feat: Use encrypted cookie to store OAuth2 state nonce (instead of redis) #8241
Conversation
alexmt
force-pushed
the
5873-auth-state-cookie
branch
from
212b12a to
2729b19
Compare
Codecov Report
@@ Coverage Diff @@
## master #8241 +/- ##
==========================================
+ Coverage 41.63% 41.93% +0.29%
==========================================
Files 174 176 +2
Lines 22881 22992 +111
==========================================
+ Hits 9527 9641 +114
+ Misses 11990 11966 -24
- Partials 1364 1385 +21
Continue to review full report at Codecov.
|
jannfis
left a comment
jannfis
left a comment
There was a problem hiding this comment.
From the first glance, I have a few questions and challenges to discuss.
Since we should be careful with this change, I'm putting a request for change for now.
It's not dangerous per se, but we should carefully review the code, flow of data and possible pitfalls before merging. Change itself is fine to go into 2.3 IMHO. |
|
Thanks for the careful review! All comments have been addressed. PTAL |
util/crypto/crypto.go
Outdated
There was a problem hiding this comment.
Should we use bcrypt instead of sha256? It's recommended in the crypto docs for keys.
Since the key is 32 random bytes, I don't think bcrypt actually buys us much. But if someone found a flaw in the random number generator, bcrypt would give us just a tiny bit more protection against someone brute-forcing the hash to recover the key.
There was a problem hiding this comment.
Actually... why hash at all? The server token is already 32 random bytes. Couldn't it serve directly as the key? Since this hashed key disappears at GC, seems like it's functionally the same as just using the server token directly.
There was a problem hiding this comment.
(I'm assuming we don't let users set that secret to some arbitrary string.)
There was a problem hiding this comment.
🤦 I should have look at how server signature is generated. Removed hashing completelly
There was a problem hiding this comment.
I was thinking: What if we decided at some point in time to make the server's signature different from what it is now (e.g. smaller or larger, different format, etc)? Also, I think now we have made the Encrypt() and Decrypt() functions public in our package, but they are of limited use now, i.e. only usable with our server's signature. SHA256 in contrast has a constant output of 32 bytes, for any given passphrase, even if it comes at some additional computational cost.
There was a problem hiding this comment.
Sorry resolved this thread before I noticed your comment @jannfis . I think you are right. I don't want to accidentally break SSO tomorrow. I've fallback to original @crenshaw-dev 's suggestion: use scrypt to generate keys from the keyphrase (bcrypt does not support generating 32 bytes long keys). According to docs, it is actually very memory expensive so I changed the code to generate key only once when settings are loaded/updated. Now crypto package has Encrypt/Decrypt functions that can be used with any key and function that generates encryption key using server signature.
…dis) Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
…ds to separate package Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
…return url Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
alexmt
force-pushed
the
5873-auth-state-cookie
branch
3 times, most recently
from
ba437cd to
53b7a11
Compare
Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
alexmt
force-pushed
the
5873-auth-state-cookie
branch
from
53b7a11 to
a398353
Compare
|
Thanks for thorough review, everyone! |
|
I've noticed that one conversation was resolved incorrectly, unresolved it, and made some improvements :) . Please see #8241 (comment) for more information. Sorry for moving back and forth in this PR. Cryptography is definitely not my strength and it was a learning exercise for me. |
crenshaw-dev
left a comment
crenshaw-dev
left a comment
There was a problem hiding this comment.
No comments that should block merging. I have a slight preference for explicitly using a "nothing" salt if the hashed passphrase isn't actually a meaningful salt.
util/crypto/crypto.go
Outdated
There was a problem hiding this comment.
Is the salt actually helpful for this use case, or is it functionally equivalent to no-salt?
There was a problem hiding this comment.
As I understand it is equivalent of no salt. I think it would be overkill to generate and store salt in argocd-secret Secret just to derive key from server signature
There was a problem hiding this comment.
Cool, agreed. For readability, what about explicit no salt (empty slice?) or a comment clarifying why we’re using the salt value we’re using.
alexmt
force-pushed
the
5873-auth-state-cookie
branch
from
ed45e2f to
0c249c8
Compare
util/crypto/crypto.go
Outdated
…e encryption key Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
alexmt
force-pushed
the
5873-auth-state-cookie
branch
from
0c249c8 to
ba1f8d6
Compare
…dis) (argoproj#8241) feat: Use encrypted cookie to store OAuth2 state nonce (instead of redis) (argoproj#8241) Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Signed-off-by: pashavictorovich <pavel@codefresh.io>
4 participants
Signed-off-by: Alexander Matyushentsev AMatyushentsev@gmail.com
Closes #5873
PR changes SSO auth flow to store temporary nonce tokens in an encrypted cookie instead of redis