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

VAULT-13614 Support SCRAM-SHA-256 encrypted passwords for PostgreSQL #19616

Merged
merged 56 commits into from
Mar 21, 2023

Conversation

raymonstah
Copy link
Contributor

@raymonstah raymonstah commented Mar 17, 2023

This PR introduces the ability to encrypt passwords in Vault before sending them to PostgreSQL. Doing so will prevent plaintext passwords from showing up in the PostgreSQL logs.
To enable scram-sha-256 password encryption, set password_encryption when writing the database configuration.

    $ vault write database/config/my-postgresql-database \
        plugin_name="postgresql-database-plugin" \
        allowed_roles="my-role" \
        connection_url="postgresql://{{username}}:{{password}}@localhost:5432/database-name" \
        username="vaultuser" \
        password="vaultpass" \
        password_authentication="scram-sha-256"

It makes use of this library to perform the encryption.

@raymonstah raymonstah requested a review from yhyakuna as a code owner March 17, 2023 18:42
@raymonstah raymonstah requested review from a team and removed request for a team March 17, 2023 18:42
@raymonstah raymonstah added this to the 1.14 milestone Mar 17, 2023
raymonstah and others added 21 commits March 17, 2023 12:03
* don't error for other message events

Signed-off-by: David van der Spek <[email protected]>

* add changelog

Signed-off-by: David van der Spek <[email protected]>

* rename release note for changelog

Signed-off-by: David van der Spek <[email protected]>

---------

Signed-off-by: David van der Spek <[email protected]>
* glimmerize alert-banner

* structure for the DocLink todo: css important remove

* styling done. kind of strange, but should help in future

* clean up

* test coverage

* changelog

* address pr comments

* clean up

* amended language on banner to match most recent change.

* add return

* clean up

* modify the banner title and shorten message

* update language
* fix delete not working for ssh config

* add test

* add changelog;
* bug: correct handling of the zero int64 value

* Update changelog/18729.txt

---------

Co-authored-by: valli_0x <[email protected]>
Co-authored-by: Tom Proctor <[email protected]>
* VAULT-14215 Fix panic for non-TLS listeners during SIGHUP

* VAULT-14215 Changelog

* VAULT-14215 Godoc for test
* replace use of os.Unsetenv in test with t.Setenv and remove t.Parallel from test that rely on env being modified.

* experiment with using fromJSON function

* revert previous experiment

* including double quotes in the output value for the string ubuntu-latest

* use go run to launch gofumpt
* Un-hiding link to 1.13 upgrade guide

* Removing draft notice
* sdk: Fix fmt + add FieldType test

* Add test comment
* Add support for importing RSA-PSS keys in Transit

Signed-off-by: Alexander Scheel <[email protected]>

* Add changelog entry

Signed-off-by: Alexander Scheel <[email protected]>

---------

Signed-off-by: Alexander Scheel <[email protected]>
)

* fix data race on plugin reload

* add changelog

* add comment for posterity

* revert comment and return assignment in router.go

* rework plugin continue on error tests to use compilePlugin

* fix race condition on route entry

* add test for plugin reload and rollback race detection

* add go doc for test
* remove oracle banner

* add back extra test coverage for other banner

* add description
@raymonstah raymonstah marked this pull request as ready for review March 18, 2023 00:53
@raymonstah raymonstah changed the title Support SCRAM-SHA-256 encrypted passwords for PostgreSQL VAULT-13614 Support SCRAM-SHA-256 encrypted passwords for PostgreSQL Mar 20, 2023
Copy link
Contributor

@maxcoulombe maxcoulombe left a comment

Choose a reason for hiding this comment

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

Looks good! Honestly all my comments are cosmetic. Only real question is why did we copy the scram code instead of importing it?

plugins/database/postgresql/postgresql.go Outdated Show resolved Hide resolved
var (
PasswordEncryptionSCRAMSHA256 PasswordEncryption = "scram-sha-256"
PasswordEncryptionPlainText PasswordEncryption = "plaintext"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would PasswordEncryptionNone PasswordEncryption = "none" be a more appropriate name?
Plaintext encryption sounds a bit funky to me, curious to hear from others.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for PasswordEncryptionNone

plugins/database/postgresql/postgresql.go Outdated Show resolved Hide resolved
@@ -10,6 +10,8 @@ import (
"regexp"
"strings"

"github.com/hashicorp/vault/plugins/database/postgresql/scram"
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why we copied to code instead of importing github.com/supercaracal/scram-sha-256?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately when I last checked, it wasn't an option since all of the source code was in a main package and un-exported.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aight, surprised there are no packages that would do what we need but it's simple enough and open sourced so I think copying is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, wondering if it would be worthwhile to have the Crypto team take a look?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, tagged @hashicorp/vault-crypto

Copy link
Contributor

Choose a reason for hiding this comment

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

woops somehow forgot we have our own resident Crypto expert! Thanks for jumping in @swenson !

plugins/database/postgresql/postgresql.go Outdated Show resolved Hide resolved
website/content/api-docs/secret/databases/postgresql.mdx Outdated Show resolved Hide resolved
changelog/19616.txt Outdated Show resolved Hide resolved
@raymonstah raymonstah requested a review from a team March 20, 2023 20:19
plugins/database/postgresql/scram/scram.go Show resolved Hide resolved
return h.Sum(nil)
}

func encryptPassword(rawPassword, salt []byte, iter, keyLen int) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't encryption.

Suggested change
func encryptPassword(rawPassword, salt []byte, iter, keyLen int) string {
func hashPassword(rawPassword, salt []byte, iter, keyLen int) string {

Copy link
Contributor

Choose a reason for hiding this comment

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

Not to bikeshed, I'm fine with hash, but I had some un-submitted comments potentially suggesting crypt here as a name. IIRC, this format that's returned here is the Unix crypt format (man crypt), which is used for password hashing.

plugins/database/postgresql/scram/scram.go Outdated Show resolved Hide resolved
website/content/api-docs/secret/databases/postgresql.mdx Outdated Show resolved Hide resolved

func encodeB64(src []byte) (dst []byte) {
dst = make([]byte, base64.StdEncoding.EncodedLen(len(src)))
base64.StdEncoding.Encode(dst, src)
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why not just return []byte(base64.StdEncoding.EncodeToString(dst, src))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, most of the source code in this file is copied over from here.

Not opposed to changing it, including handling the errors you mentioned below.


func getHMACSum(key, msg []byte) []byte {
h := hmac.New(sha256.New, key)
_, _ = h.Write(msg)
Copy link
Contributor

@cipherboy cipherboy Mar 20, 2023

Choose a reason for hiding this comment

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

HMAC and SHA-256 may error with BoringCrypto fwiw.

I believe this mostly occurs with large inputs, which this shouldn't really have in general, but still worth noting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to know. The docs for the functions in the Go standard library say they will "never" return an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the Go standard library doesn't really document the BoringCrypto behavior from what I've seen, especially since its not officially supported.

Copy link
Contributor

@swenson swenson left a comment

Choose a reason for hiding this comment

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

LGTM as far as the scram stuff goes.

@@ -77,7 +79,8 @@ func new() *PostgreSQL {
type PostgreSQL struct {
*connutil.SQLConnectionProducer

usernameProducer template.StringTemplate
usernameProducer template.StringTemplate
PasswordEncryption passwordAuthentication
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is used outside of this directory, but would be good to update this to PasswordAuthenticator as well.

Copy link
Contributor

@maxcoulombe maxcoulombe left a comment

Choose a reason for hiding this comment

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

Looks good! Just a small suggestion left for the doc and 🚢 🇮🇹

@raymonstah
Copy link
Contributor Author

raymonstah commented Mar 21, 2023

This PR addresses #6910

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.