require a TLS client certificate by default #453
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit changes the `ClientAuth` type from `RequestClientCert` to `RequireAnyClientCert` by default. In general, a KES server should demand a client certificate. Otherwise, a client (the HTTP/TLS stack) may choose to not send a client certificate - even if one is available. For example, the HTTP stack may try to be smart and not send a client certificate if it determines that cannot be validated since its self-signed. Instead, the KES server's TLS should abort the handshake if the client does not send a certificate. However, in some cases we cannot enforce this. In particular, when some APIs should be accessible without TLS authentication, like `/v1/metrics`. In these cases, we have to make it optional for clients to send a certificate. However, disabling auth for some APIs is an advanced use case intended only for users who are aware of the implications. Signed-off-by: Andreas Auernhammer <[email protected]>
ravindk89
approved these changes
Mar 13, 2024
Comment on lines
+308
to
+320
| // If mTLS authentication is disabled for at least one API, | ||
| // we must no longer require that a client sends a certificate. | ||
| // However, this may cause authentication errors when a client | ||
| // (the client's HTTP/TLS stack) does not send a certificate | ||
| // for an API that requires authentication. | ||
| if api.InsecureSkipAuth.Value { | ||
| if clientAuth == tls.RequireAnyClientCert { | ||
| clientAuth = tls.RequestClientCert | ||
| } | ||
| if clientAuth == tls.RequireAndVerifyClientCert { | ||
| clientAuth = tls.VerifyClientCertIfGiven | ||
| } | ||
| } |
There was a problem hiding this comment.
@aead just to confirm, how exactly would someone disable auth for an API? I don't recall this in the server config options off the top of my head.
There was a problem hiding this comment.
harshavardhana
approved these changes
Mar 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit changes the
ClientAuthtype fromRequestClientCerttoRequireAnyClientCertby default.In general, a KES server should demand a client certificate. Otherwise, a client (the HTTP/TLS stack) may choose to not send a client certificate - even if one is available. For example, the HTTP stack may try to be smart and not send a client certificate if it determines that cannot be validated since its self-signed.
Instead, the KES server's TLS should abort the handshake if the client does not send a certificate. However, in some cases we cannot enforce this. In particular, when some APIs should be accessible without TLS authentication, like
/v1/metrics. In these cases, we have to make it optional for clients to send a certificate. However, disabling auth for some APIs is an advanced use case intended only for users who are aware of the implications.