-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
database/postgres: add inline certificate authentication fields #28024
Conversation
da01aee
to
83748a8
Compare
CI Results: |
@@ -62,15 +58,3 @@ func GetCloudSQLAuthOptions(credentials string, usePrivateIP bool) ([]cloudsqlco | |||
|
|||
return opts, nil | |||
} | |||
|
|||
func ValidateAuthType(authType string) bool { |
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.
moved to sql.go
Build Results: |
83748a8
to
889ed2f
Compare
// Deprecated: OpenPostgres will be removed in a future version of the Vault SDK. | ||
func OpenPostgres(driverName, connString string) (*sql.DB, error) { | ||
// Deprecated: openPostgres will be removed in a future version of the Vault SDK. | ||
func openPostgres(driverName, connString string) (*sql.DB, error) { |
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.
This was only exported for tests but I decided we should not export this so the test usage was removed. This code path is tested however because it is called indirectly by our tests.
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! Had 1 question, but non-blocking
} | ||
tlsConfig.Certificates = []tls.Certificate{cert} | ||
p.TLSConfig = tlsConfig | ||
} else { |
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.
Are sslcert
and sslkey
always required to have a TLS config? Should it be possible for the user to only set the sslrootcert
? If not, wondering if it is worth returning an error to the user saying that the TLS Cert and Private Key need to be added
This is for the case if the user sets the sslrootcert
but does not set either the sslcert
or sslkey
, then p.TLSConfig
will be nil
, and we would not use any info decoded in L112-L119. Since it would be under the hood I was wondering if it would be worth surfacing that we will ignore the root cert because of missing info in the config
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.
Great question! I considered this briefly as I was writing this code but never came back to it.
Setting only sslrootcert
is a legit use case. This is useful when you want the client to verify the server certificate. See https://www.postgresql.org/docs/16/libpq-ssl.html#LIBQ-SSL-CERTIFICATES
I will update this flow to allow for this use case. 👍
This PR adds support for inline TLS configuration.
We add new fields to the plugin's config endpoint:
tls_certificate
,tls_private_key
andtls_ca
. These fields will take certificate data as a string. This allows Vault Operators and/or development teams to configure TLS when they do not have access to the Vault Server.Here is an example of configuring TLS inline via the new fields on the plugin’s config endpoint:
The
@
in the CLI argument indicates a path to a file on disk to be read.