Cassandra: Refactor PEM parsing logic #11861
Conversation
The ParsePEMBundle and ParsePKIJSON functions in the certutil package assumes both a client certificate and a custom CA are specified. Cassandra needs to allow for either a client certificate, a custom CA, or both. This revamps the parsing of pem_json and pem_bundle to accomodate for any of these configurations
| @@ -83,38 +82,46 @@ func (c *cassandraConnectionProducer) Initialize(ctx context.Context, req dbplug | |||
| return fmt.Errorf("username cannot be empty") | |||
| case len(c.Password) == 0: | |||
| return fmt.Errorf("password cannot be empty") | |||
| case len(c.PemJSON) > 0 && len(c.PemBundle) > 0: | |||
| return fmt.Errorf("cannot specify both pem_json and pem_bundle") | |||
There was a problem hiding this comment.
I think that this is good to do, but note that we previously allowed both to be specified at the same time, with preference on pem_json so it's technically a breaking change. Would be good to get the docs updated as well to indicate that this is a one of between the two parameters.
There was a problem hiding this comment.
True, this is a breaking change. I'm not a fan of allowing for both as it can create confusion ("which one is being used?"). I'll update the docs to describe this change unless anyone has any objections to making this change.
| caChain := []string{} | ||
|
|
||
| count := 0 | ||
| for len(pemBytes) > 0 { |
There was a problem hiding this comment.
Does the ordering while parsing matter? For example, should we check/ensure that there's no private key exists in the middle of a chain?
There was a problem hiding this comment.
As far as I can tell there is no required ordering down stream. The current ParsePEMBundle function also doesn't enforce any ordering (other than the implicit assertion that the client certificate is before the CA cert). We can enforce ordering if we want, but the current implementation doesn't require it.
There was a problem hiding this comment.
I noticed that ParsePEMBundle performs validity checks around the client certs and the CA chain, via parsedBundle.Verify(). Wondering if we should do the same.
There was a problem hiding this comment.
I looked into the Verify function and it is specific to mTLS so we can't do the exact same validation. I'm suspicious that we don't need to do much of what it's doing in that function as it's probably handled downstream in the TLS code, but I'll try to double check that.
|
|
||
| - `pem_json` `(string: "")` – Specifies JSON containing a certificate and | ||
| private key; a certificate, private key, and issuing CA certificate; or just a | ||
| CA certificate. For convenience format is the same as the output of the | ||
| `issue` command from the `pki` secrets engine; see | ||
| [the pki documentation](/docs/secrets/pki). | ||
| [the pki documentation](/docs/secrets/pki). Only one of `pem_bundle` or `pem_json` can be specified. |
There was a problem hiding this comment.
Should we have a note that the certs/key can't have newlines?
There was a problem hiding this comment.
I've added a note that the PEM data must be properly JSON encoded.
| where Vault will not parse the CA certificate correctly. To work around this, use `pem_json` with the | ||
| following structure: `{"ca_chain": ["-----BEGIN CERTIFICATE-----\nMIIEFjC...FNYakP7I\n-----END CERTIFICATE-----"]}` | ||
| Also make sure the PEM data is properly JSON encoded with `\n` instead of newlines. | ||
| certificate; or just a CA certificate. Only one of `pem_bundle` or `pem_json` can be specified. |
There was a problem hiding this comment.
Do you think there's value in providing a sample command for constructing this concatenated bundle to minimize the chances of a malformed bundle being provided?
There was a problem hiding this comment.
I lean towards no because such a command is going to be fairly specific to my development environment.
* Refactor TLS parsing The ParsePEMBundle and ParsePKIJSON functions in the certutil package assumes both a client certificate and a custom CA are specified. Cassandra needs to allow for either a client certificate, a custom CA, or both. This revamps the parsing of pem_json and pem_bundle to accomodate for any of these configurations
* Refactor TLS parsing The ParsePEMBundle and ParsePKIJSON functions in the certutil package assumes both a client certificate and a custom CA are specified. Cassandra needs to allow for either a client certificate, a custom CA, or both. This revamps the parsing of pem_json and pem_bundle to accomodate for any of these configurations
Description
This fixes an issue in the Cassandra database plugin TLS logic where providing only a custom CA doesn't work when using
pem_bundleand partially works withpem_json. This now supports setting either a client certificate & private key, a custom CA, or both.How does this work?
The
certutil.ParsePEMBundlefunction makes assumptions about the PEM data it is parsing:Certificatefield rather thanCAChainwhich ends up not being interpreted correctly when converting to atls.Config.A new function has been written (
pemBundleToTLSConfig) that parses the PEM bundle very similarly tocertutil.ParsePEMBundlebut does not make either of those assumptions.Similarly, the
certutil.ParsePKIJSONfunction ends up having some similar assumptions, however it does parse the CA correctly and thus thetls.Configworks when connecting to Cassandra. However, the CA is also included as a client certificate when it shouldn't be. However, it appears that Cassandra will ignore a provided client certificate if not configured to use mTLS.A new function was created (
jsonBundleToTLSConfig) that parses the certificate data into atls.Configobject.Testing
Additional automated tests were added in
connection_producer_test.gothat start up a Cassandra instance using TLS with a custom CA and connects to it using the same custom CA.Also this was tested with a real Cassandra instance (also configured with a custom CA) and a real Vault instance configured to use just the custom CA when talking to Cassandra.