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

tls: require RSA certificates with 2048-bit or larger keys. #5318

Merged
merged 3 commits into from
Dec 17, 2018

Conversation

PiotrSikora
Copy link
Contributor

@PiotrSikora PiotrSikora commented Dec 16, 2018

RSA certificates with keys smaller than 2048-bits are disallowed by NIST,
Internet migrated to 2048-bit keys in 2013, and no CAs issue certificates
with smaller keys, so let's prevent users from accidentally using such
certificates.

Signed-off-by: Piotr Sikora piotrsikora@google.com

RSA certificates with keys smaller than 2048-bits are disallowed by NIST,
Internet migrated to 2048-bit keys in 2013, and no CAs issue certificates
with smaller keys, so we let's prevent users from accidentally using such
certificates.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Contributor Author

@agl @davidben I originally started this as FIPS-only requirement, but it seems that noone should be using 1024-bit RSA anymore, and since Envoy got a bit opinionated recently (i.e. allowing only P-256 ECDSA certificates), I think that it's sane to enforce this as well. Let me know if you disagree!

cc @mattklein123 @htuch

htuch
htuch previously approved these changes Dec 16, 2018
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

This works from a Google perspective, so LGTM. Will wait for others to comment before merging.

RSA* rsa_public_key = EVP_PKEY_get0_RSA(public_key.get());
// Since we checked the key type above, this should be valid.
ASSERT(rsa_public_key != nullptr);
int rsa_key_length = RSA_size(rsa_public_key);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: const int (maybe switch to unsigned for later arithmetic..).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

@htuch htuch self-assigned this Dec 16, 2018
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@agl
Copy link
Contributor

agl commented Dec 16, 2018

I think this is fine. Issues that I would expect would come from people doing dummy setups with keys generated by openssl genrsa which, in older versions, defaulted to 1024 bits. (But I guess putting a speed-bump there is the intent.)

As for FIPS, as far as BoringSSL is concerned, FIPS RSA keys are either exactly 2048 or 3072 bits. I believe that's true of any FIPS approved module given FIPS 186-4 section 5.1:

This Standard specifies three choices for the length of the modulus (i.e., nlen): 1024, 2048 and
3072 bits.

However, imposing FIPS requirements more generally is probably not appropriate. For example, it's probably not reasonable for Envoy to disallow 4096-bit RSA by default.

@mattklein123
Copy link
Member

What are the chances that someone is using a 1024-bit key in production? I don't have any sense of the likelihood of this. If it's basically 0% this is fine with me. If higher perhaps we should config guard this and default to on? Thoughts?

@PiotrSikora
Copy link
Contributor Author

@agl this PR is specifically about rejecting RSA certificates with keys smaller than 2048 bits in all builds (FIPS and non-FIPS). We can (should?) add more requirements the FIPS PR and guard them with #ifdef BORINGSSL_FIPS. I agree that enforcing all FIPS requirements in non-FIPS builds is a bad idea.

@mattklein123 there is 0% chance of this happening on the edge / public Internet, since no browsers accept such certificates and no CAs issue them... However, if there are people using self-signed certificates (i.e. produced using openssl genrsa and not any CA-like tool) inside their service mesh, then this might affect them. IMHO, this should affect close to 0% of production deployments, but it's impossible to tell, really...

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

We don't need to be perfectionist on being non-breaking for test/dev setups, so if production is close to zero likelihood, LGTM.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit 924d2d5 into envoyproxy:master Dec 17, 2018
fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019
…xy#5318)

RSA certificates with keys smaller than 2048-bits are disallowed by NIST,
Internet migrated to 2048-bit keys in 2013, and no CAs issue certificates
with smaller keys, so we let's prevent users from accidentally using such
certificates.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants