Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TrustedKeys to TLS Config #10803

Conversation

sinkingpoint
Copy link

Description

When using client certificates, it's sometimes useful to use publicly signed certificates to avoid
having to run your own CA infrastructure. This leaves you open though, because if you trust a public cert then any public cert will work.

This alleviates this by allowing specifying a list of "Trusted Keys". This is an array of SHA256
fingerprints of certs that are trusted, allowing
us to reject any certs that are not the exact one we expect.

Link to tracking issue

Fixes #10523

Testing

Added two tests where we create a server that trusts a specific key, and then asserts that that key passes verification whereas the other cert doesn't.

Documentation

Added new option to README, along with an example

@sinkingpoint sinkingpoint requested review from a team and mx-psi August 6, 2024 01:48
Copy link

codecov bot commented Aug 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.65%. Comparing base (93cbae4) to head (259770f).
Report is 230 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10803      +/-   ##
==========================================
- Coverage   91.71%   91.65%   -0.07%     
==========================================
  Files         406      406              
  Lines       18947    19019      +72     
==========================================
+ Hits        17378    17432      +54     
- Misses       1209     1227      +18     
  Partials      360      360              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sinkingpoint sinkingpoint force-pushed the sinkingpoint/trusted-keys branch 4 times, most recently from 772fd3a to 231cc73 Compare August 6, 2024 05:02
When using client certificates, it's sometimes useful
to use publically signed certificates to avoid
having to run your own CA infrastructure. This leaves
you open though, because if you trust a public cert
then _any_ public cert will work.

This alleviates this by allowing specifying a list
of "Trusted Keys". This is an array of SHA256
fingerprints of certs that are trusted, allowing
us to reject any certs that are not the exact one we expect.

Signed-off-by: sinkingpoint <[email protected]>
@sinkingpoint
Copy link
Author

@mx-psi Could I get your eyes on this when you get a sec?

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, sorry for the delay. I am interested in discussing two points:

  1. Are you taking inspiration from a different application that does this? If so, how do they do it?
  2. How are we going to support a different hashing algorithm in the future?

@@ -72,6 +72,14 @@ Additionally certificates may be reloaded by setting the below configuration.
Accepts a [duration string](https://pkg.go.dev/time#ParseDuration),
valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".

- `trusted_keys` (optional) : TrustedKeys allows specifying a list of SHA256 hashes to trust when verifying client certificates. This allows only trusting a specific set of keys, rather than all the keys signed by a given CA. Note: The client certificate still has to pass standard TLS verification, so you must still set the required CA values.
Copy link
Member

@mx-psi mx-psi Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't necessarily need to implement this now (if it doesn't lead to a breaking change), but if we want to change the hashing algorithm used here, how do you see that transition happening? I guess we could have something like this:

tls:
  trusted_keys:
    hashing_function: sha256
    hashes:
      - "29:9D:39:8E:26:E7:72:88:D7:5D:2D:34:FE:8C:41:FA:9F:F1:C3:D7"

or instead each element of tls::trusted_keys could be something like this:

tls:
  trusted_keys:
    - sha256: "29:9D:39:8E:26:E7:72:88:D7:5D:2D:34:FE:8C:41:FA:9F:F1:C3:D7"

What do you think is the best approach? I am leaning towards the latter option (it's a bit more flexible)

Copy link
Contributor

github-actions bot commented Sep 4, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Sep 4, 2024
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Sep 18, 2024
@namco1992
Copy link

Hello @mx-psi, we'd like to see this implemented, is there anything we can do to get this change merged? I can take over and implement the approach 2 you proposed if needed.

@mx-psi
Copy link
Member

mx-psi commented Oct 18, 2024

Hey @namco1992 , feel free to open a PR. I still would want to explore if other applications do this and what's their UX like to make sure we are aligned with them if it makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support trusting only a set of certificate hashes
3 participants