Closed joshuatcasey closed 2 months ago
Attention: Patch coverage is 88.82682%
with 20 lines
in your changes missing coverage. Please review.
Project coverage is 30.68%. Comparing base (
5d6dbe1
) to head (f7f32f2
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Does this new configuration option allow the user to constrain the ciphers so much that everything stops working? For example, if they only allow one cipher, and that cipher is not an elliptic curve cipher, then will that break everything (because all of our auto-generated TLS certs use EC)?
Does this new configuration option allow the user to constrain the ciphers so much that everything stops working? For example, if they only allow one cipher, and that cipher is not an elliptic curve cipher, then will that break everything (because all of our auto-generated TLS certs use EC)?
This is a good point. Something outside of the Pinniped code ultimately decides which ciphers to use that even further constrain the user-allowed list. At the moment this would not impede application startup but the endpoints would never complete a TLS handshake. This needs more thought.
What if we reserved at least one elliptic cipher for Pinniped use? That way at least the healthcheck would work. If the user has provided incompatible certificates and cipher suites there's not much we can do about that.
Another option is we require at least one elliptic and one RSA cipher?
What if we reserved at least one elliptic cipher for Pinniped use? That way at least the healthcheck would work. If the user > has provided incompatible certificates and cipher suites there's not much we can do about that.
Another option is we require at least one elliptic and one RSA cipher?
We discussed on a call and I think we decided that the documentation for the new option is sufficient. We cannot know what combination of cipher will work because it is outside our control, for example we don't know how the Kubernetes API server's ciphers were configured.
Limit TLS ciphers for tls1.2.
Fixes #1605.
Replaces #1927.