Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
VAULT-13614 Support SCRAM-SHA-256 encrypted passwords for PostgreSQL #19616
Changes from 49 commits
da0d8ae
e8db5e5
49bc306
72e8096
5a54a44
e032a37
f2b2b0b
b55c3b2
c27026d
88988e1
8185d68
72c5ce8
e8f8084
25e359f
139fad6
e4957d6
0e8ecd0
0fb9b15
3e4bc9f
edef778
068c146
915c245
e210cae
aaf2824
f0e9dea
7983f92
0102e61
5d4f9b8
28590b8
559627c
fd60cdc
9aa7fee
405f598
4c8d062
a579239
ebf5b63
8ddea0b
08231ce
97b68f5
3d4ba60
882b172
76c7871
addc7c0
1071315
6039a6c
0ad6835
ef51b7b
cb65677
3ca59d1
99e2d6e
e0f926e
b998c23
efe94fd
ef287a2
ceaba05
aac9e01
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Curious why we copied to code instead of importing
github.com/supercaracal/scram-sha-256
?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.
Unfortunately when I last checked, it wasn't an option since all of the source code was in a
main
package and un-exported.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.
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.
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.
Also, wondering if it would be worthwhile to have the Crypto team take a look?
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.
Sure, tagged @hashicorp/vault-crypto
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.
woops somehow forgot we have our own resident Crypto expert! Thanks for jumping in @swenson !
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.
Would
PasswordEncryptionNone PasswordEncryption = "none"
be a more appropriate name?Plaintext encryption sounds a bit funky to me, curious to hear from others.
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.
+1 for
PasswordEncryptionNone
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.
Out of curiosity, why not just
return []byte(base64.StdEncoding.EncodeToString(dst, src))
?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.
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.
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.
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.
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.
Good to know. The docs for the functions in the Go standard library say they will "never" return an 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.
Yeah, the Go standard library doesn't really document the BoringCrypto behavior from what I've seen, especially since its not officially supported.
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 isn't encryption.
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.
Not to bikeshed, I'm fine with
hash
, but I had some un-submitted comments potentially suggestingcrypt
here as a name. IIRC, this format that's returned here is the Unix crypt format (man crypt
), which is used for password hashing.