Ensure RSA keys are at least 2048 bits in length#17911
Conversation
jm96441n
added
pr/no-docs
|
|
||
| // Envoy will silently reject any keys that are less than 2048 bytes long | ||
| // https://github.com/envoyproxy/envoy/blob/main/source/extensions/transport_sockets/tls/context_impl.cc#L238 | ||
| const MinKeyLength = 2048 |
There was a problem hiding this comment.
given the FIPS work on enterprise, wondering based on the conditional a couple lines below the linked envoy line:
if (rsa_key_length != 2048 && rsa_key_length != 3072 && rsa_key_length != 4096) {
throw EnvoyException(
fmt::format("Failed to load certificate chain from {}, only RSA certificates with "
"2048-bit, 3072-bit or 4096-bit keys are supported in FIPS mode",
ctx.cert_chain_file_path_));
} We may want to call:
Line 5 in b76c4d7
and do some additional constraint checks and only allow you to create InlineCertificates with those exact bit lengths if built with FIPS... since in the FIPS-compliant envoy build (and due to the movement towards packaging envoy FIPS alongside dataplane we should be able to detect since we require the whole stack including the Consul servers to be FIPS-compliant at that point) any other key length will result in the same broken envoy instances... WDYT?
There was a problem hiding this comment.
that's a good call out! lemme modify this a bit
There was a problem hiding this comment.
One other thing to call out is that you'll want to also do this in the k8s side of things on the gateway validation itself, somewhere like this:
Since we don't do any sort of status syncing to Kubernetes secret objects, invalid certificates referenced by a gateway get statuses reflected back only on the gateway. Rather than attempting a sync and having it be silently rejected by Consul, we should just skip the attempt at InlineCertificate creation and set a status on the Gateway object in Kubernetes saying, "the cert you're referencing has an invalid length" to inform the user.
There was a problem hiding this comment.
yeah I've got some code over in the ParseCertificates function currently that I'm gonna push up after this merges so implementations are similar-ish and will have mostly been code reviewed already
andrewstucki
left a comment
andrewstucki
left a comment
There was a problem hiding this comment.
One small nit change for readability and I think this is good, also called out where you'll want to add some changes for the k8s code to propagate error statuses on certificates back to users, but that's separate from this.
andrewstucki
left a comment
andrewstucki
left a comment
There was a problem hiding this comment.
Approving with this one change that's problematic and causing issues/the linter to fail.
* Ensure RSA keys are at least 2048 bits in length * Add changelog * update key length check for FIPS compliance * Fix no new variables error and failing to return when error exists from validating * clean up code for better readability * actually return value
…ease/1.16.x (#17935) * backport of commit 93ccfe4 * Ensure RSA keys are at least 2048 bits in length (#17911) * Ensure RSA keys are at least 2048 bits in length * Add changelog * update key length check for FIPS compliance * Fix no new variables error and failing to return when error exists from validating * clean up code for better readability * actually return value --------- Co-authored-by: jm96441n <[email protected]>
Description
Envoy will silently reject RSA keys that are less than 2048 bits in length. The only way to find a key has been rejected is to go deep into the envoy config to see why. This introduces a change to reject invalid keys earlier and in a more user friendly way.
Testing & Reproduction steps
Links
PR Checklist