Migrate cookie signing to SHA256 from SHA1 #524
Conversation
NickMeves
requested review from
JoelSpeed,
steakunderscore and
loshz
as code owners
pkg/encryption/cipher.go
Outdated
| h := hmac.New(sha256.New, []byte(args[0])) | ||
| for _, arg := range args[1:] { | ||
| h.Write([]byte(arg)) | ||
| } | ||
| var b []byte | ||
| b = h.Sum(b) | ||
| return base64.URLEncoding.EncodeToString(b) | ||
| } |
There was a problem hiding this comment.
This is basically the same as the content of LegacyCookieSignature, how about extracting to a helper somehow?
func cookieSignature(args ...string) string {
return cookieSignatureFrom(sha256.New, args...)
}
func legacyCookieSignature(args ...string) string {
return cookieSignatureFrom(sha1.New, args...)
}
pkg/encryption/cipher.go
Outdated
| if checkHmac(parts[2], legacySig) { | ||
| ts, err := strconv.Atoi(parts[1]) | ||
| if err != nil { | ||
| return | ||
| } | ||
| t = time.Unix(int64(ts), 0) | ||
| if t.After(time.Now().Add(expiration*-1)) && t.Before(time.Now().Add(time.Minute*5)) { | ||
| // it's a valid cookie. now get the contents | ||
| rawValue, err := base64.URLEncoding.DecodeString(parts[0]) | ||
| if err == nil { | ||
| value = string(rawValue) | ||
| ok = true | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the same as lines 30-49, how about extracting to it's own function?
I thought the same thing this morning and wanted to log in and add unit tests -- but you beat me to it 😄 I'll try to get these tweaks added later today 👍 |
Also, cleanup the code & make the specific hashing algorithm chosen a function variable.
NickMeves
force-pushed
the
sha256-signed-cookies
branch
from
4e8aede to
b5a7bd1
Compare
|
Code Climate has analyzed commit b5a7bd1 and detected 0 issues on this pull request. View more on Code Climate. |
|
Note: long-term, when we rip the bandaid off and stop allowing legacy sessions still signed with SHA1, we can remove all the code supporting legacy sessions that creep in in PRs like this: |
Also, cleanup the code & make the specific hashing algorithm chosen a function variable. Co-authored-by: Henry Jenkins <[email protected]>
|
Manually fixed the merge conflit. GH didn't like that manually squashed the commits so didn't pick up the commit. |
This moves cookie signing from SHA1 to SHA256 in a backwards compatible way that still validates existing cookies with SHA1 signatures (to be removed later after a burn in period).
Description
Sign cookies with SHA256
Motivation and Context
Since SHA1 for signing now has multiple vulnerabilities & collision attack issues, this moves to the more cryptographically secure SHA256.
How Has This Been Tested?
Ran locally:
Confirmed existing SHA1 signed cookie sessions still were valid with the proxy.
Confirmed new cookies were signed with SHA256
UPDATE: Unit tests added
Checklist: