Validate hostnames when using TLS in Cassandra #11365
Conversation
| } | ||
|
|
||
| tlsConfig, err := parsedCertBundle.GetTLSConfig(certutil.TLSClient) | ||
| if err != nil || tlsConfig == nil { |
There was a problem hiding this comment.
Looking at GetTLSConfig(), it doesn't seem that we can get a nil tlsConfig if the error is nil so a check on err != nil should be sufficient.
There was a problem hiding this comment.
Good catch, fixed.
| @@ -269,6 +230,50 @@ func (c *cassandraConnectionProducer) createSession(ctx context.Context) (*gocql | |||
| return session, nil | |||
| } | |||
|
|
|||
| func getSslOpts(certBundle *certutil.CertBundle, minTLSVersion string, insecureSkipVerify bool) (*gocql.SslOptions, error) { | |||
| var tlsConfig *tls.Config | |||
There was a problem hiding this comment.
Can we start off with tlsConfig := &tls.Config{} if this is never expected to be nil?
There was a problem hiding this comment.
Funnily enough we just fixed this in another DB: https://github.com/hashicorp/vault/pull/10899/files
| defer cleanup() | ||
|
|
||
| // This is fine if it panics - the connection URL should include the port and if it doesn't, this should fail | ||
| port := strings.Split(connURL, ":")[1] |
There was a problem hiding this comment.
Should we handle this failure a bit more gracefully by doing a length check first instead of letting it panic?
There was a problem hiding this comment.
I don't think an additional check is valuable here because it's in a test and it needs to fail, so having it be a test failure rather than a panic doesn't really give us much. It's an irrecoverable error due to the test construct failing which seems like a panic is well suited for.
There was a problem hiding this comment.
Further investigation revealed that panicking in a test does not behave the same as running t.Fatal. It will stop all tests that are running in the package. I didn't realize this was the behavior. I've updated this so the PrepareTestContainer function instead returns a new Host type that contains the hostname, the port, and has a function to build the connection URL. It also performs the equivalent check in PrepareTestContainer rather than forcing the callers to make any additional checks.
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/gocql/gocql" | ||
| "github.com/hashicorp/vault/helper/testhelpers/docker" | ||
| ) | ||
|
|
||
| func PrepareTestContainer(t *testing.T, version string) (func(), string) { | ||
| type containerConfig struct { |
There was a problem hiding this comment.
Thanks! I figured since I needed to add another optional field that it would be better to switch to the functional arguments pattern for ease of use downstream.
| return nil, fmt.Errorf("failed to parse certificate bundle: %w", err) | ||
| } | ||
|
|
||
| tlsConfig, err := parsedCertBundle.GetTLSConfig(certutil.TLSClient) |
There was a problem hiding this comment.
Should this be using = instead of := when assigning to tlsConfig? I think using := here would result in a nil pointer dereference down on L256.
There was a problem hiding this comment.
Nice catch! It won't be a pointer dereference because of tlsConfig = &tls.Config{} on the else, but it won't set the fields from GetTLSConfig since tlsConfig is scoped to this conditional.
There was a problem hiding this comment.
Ya, was thinking it'd be nil pointer dereference if we don't enter that else branch (since certBundle != nil). In any case, I agree that the correct tlsConfig won't be set due to the scoping.
There was a problem hiding this comment.
I think you're right. Since both variables exist before this line they are shadowed by this call when using :=. Or at least that's what my IDE is telling me.
Updates Cassandra database engine and physical backend to validate hostnames when configured to use TLS.
This also updates the gocql library to the latest version.