-
-
Notifications
You must be signed in to change notification settings - Fork 364
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
fix: [#630] Use the correct algorithm for at_hash and c_hash #659
Conversation
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.
Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)
handler/openid/helper.go
Outdated
} | ||
|
||
var hash hash.Hash | ||
if hashSize == 384 { |
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.
I think it would make sense to set hash directly, without the intermediate hashSize variable :)
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.
Hmm, could you elaborate on this please? The alg
may or may not be present in the header, so the first step here is to compute the hash size.
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.
I guess something along this pseudo-code:
hash := sha256.New()
if alg, ok := sess.IDTokenHeaders().Get("alg").(string); ok {
hashSize, _ := strconv.Atoi(alg[2:]) // 0 per default
switch hashSize {
case 384
hash = sha512.New()
// ...
}
}
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.
@aeneasr Made the change.
@aeneasr Pushed in the change based on your review comments. Hope this works. |
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.
Awesome, thank you for your contribution!
Related Issue or Design Document
The defect is described in #630 along with the discussion.
at_hash
: https://openid.net/specs/openid-connect-core-1_0.html#CodeIDTokenc_hash
: https://openid.net/specs/openid-connect-core-1_0.html#HybridIDTokenChecklist
and signed the CLA.
introduces a new feature.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
works.
appropriate).
Further comments
There are two key changes:
ComputeHash
that extracts the algorithm, if available, from the openid.Session header and computes the hash. It falls back to 256 key length to retain backward compatibility.