-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Fix support for RSA CA keys in Connect. #6638
Conversation
Realised when adding tests that Vault doesn't actually support cross-signing CA certs with a different key type currently. I filed hashicorp/vault#7709 to see if that's something they might change but it might mean we should for now not bother trying to enforce that full matrix of CA key type compatibility and just document that all DCs need the same CA key type configured. This is likely more future proof for additional CA integrations which may well also not allow signing with different key types too. I think I'm going to back down on this requirement and instead document the limitation so we can ship this. The bad part is that even if all DCs have the same key type for a Vault-based CA, rotating to a different CA provider with a different key type will fail since we can't cross sign the keys. The up-coming work to allow forcing no cross signing for CAs that don't support that at all will be a reasonable workaround for now but it would be nice for Vault to actually support cross signing CAs with different key type. |
Co-Authored-By: R.B. Boyer <rb@hashicorp.com>
@rboyer thanks for the review - I've addressed all the points and finished the TODO items here with some commentary above. Should be ready to go when you get a chance to look! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Some history is required here:
Design Decisions
Rather than allowing Connect to generate leaf keys that match the CA key type, I've instead kept all leaf keys as EC P256 but allowed for all current CAs to support signing CSR with a different key type.
It's not especially hard to keep key types following CA type and we can change to that approach later but this should work since TLS1.2 relaxed TLS1.1's requirement to use ECDSA when signing a EC certificate. Both Consul and Vault CAs allow signing an EC cert with an RSA key and all of Golang, openssl and Envoy validate these chains correctly (all three are verified by tests in this PR).
For now this seems marginally nicer than having a legacy CA root that needs to be RSA 2048 for compatibility reasons hold back Connect from using more modern crypto. But if we do hit compat issues in the future we can always change to following key types or even allow separate config of CA and leaf key types.
In this current setup any new CA provider will need to be explicitly tested with both key types for the ability to sign EC leaf CSRs. They also need to be able to sign CA CSRs from secondary DCs with a full cross-product. Typically secondary DCs will be configured to use the same CA key type we assume but they may be configured differently which means that all types of CA root need to be able to sign all types of CA CSR and have this tested (see TODO below).
Testing
I've tried to add a lot of tests to validate the various permutations that can now happen.
The cert gen tests that use openssl to independently verify our chains are now table-powered to verify more permutations of key type etc.
Both CA providers now have tests that cover them signing a leaf with an EC key with both EC and RSA CA keys. All tests also validate the whole chain afterwards too.
The "unit" tests should be sufficient to cover all behaviour including secondary CA init, but I added an end-to-end integration test using Envoy as a sanity check and in the process found a bug that I'd missed testing at unit test level: the secondary DC CSR would use the wrong signature algorithm when configured with an RSA key. That is fixed and a unit test that would have caught that case was added to by making the test a table test with different key type configs as input.
All the tests also sanity check that the root ca generated actually does have the key type configured too because it was very easy to misconfigure or not catch a config/initialization bug that meant we actually ignored the key type.
Finally, I fixed the issue that we were only populating
CARoots.PrivateKey{Type|Bits}
with the ca config values which were almost always empty but could actually be different from the CA key if it was provided externally. Now we populate that exclusively from looking at the actual key in the CA cert we are adding so it should always be correct. I added table tests to verify some of this parsing and found a few other issues I'll deal with separately (like our use if uint64 for serial which will truncate longer serials from external providers and make them meaningless).TODO