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

Fix support for RSA CA keys in Connect. #6638

Merged
merged 6 commits into from
Nov 1, 2019
Merged

Fix support for RSA CA keys in Connect. #6638

merged 6 commits into from
Nov 1, 2019

Conversation

banks
Copy link
Member

@banks banks commented Oct 17, 2019

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

  • Document the CA root key fields added in 1.6.0 but not mentioned in docs
  • Document the CA root key config added in 1.6.0 but not added to docs
  • Add explicit unit tests for both CA providers that they can sign a CA CSR with different key type just in case secondaries are configured with a different key type to primaries.

@banks banks added the theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies label Oct 17, 2019
@banks banks added this to the 1.7.0 milestone Oct 17, 2019
@banks banks requested a review from a team October 17, 2019 13:30
agent/connect/csr.go Outdated Show resolved Hide resolved
agent/connect/testing_ca.go Show resolved Hide resolved
agent/connect/testing_ca.go Show resolved Hide resolved
agent/connect/testing_ca.go Outdated Show resolved Hide resolved
agent/connect/testing_ca_test.go Outdated Show resolved Hide resolved
agent/consul/leader_connect_test.go Outdated Show resolved Hide resolved
agent/consul/leader_connect_test.go Outdated Show resolved Hide resolved
agent/structs/connect_ca.go Outdated Show resolved Hide resolved
agent/structs/connect_ca.go Show resolved Hide resolved
agent/structs/connect_ca.go Outdated Show resolved Hide resolved
@banks
Copy link
Member Author

banks commented Oct 22, 2019

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.

@banks
Copy link
Member Author

banks commented Oct 22, 2019

@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!

Copy link
Member

@rboyer rboyer left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants