Support aud claim verification for arrays #188
Conversation
…ings. Add tests to help ensure backward compatibility, and compliance with RFC 7519 Section 4.1. https://tools.ietf.org/html/rfc7519#section-4.1
|
So the build fail is on Go 1.3 it's due to a poor implementation of the Go 1.3 pkg/crypto/subtle/constant_time.go// ConstantTimeCompare returns 1 iff the two equal length slices, x
// and y, have equal contents. The time taken is a function of the length of
// the slices and is independent of the contents.
func ConstantTimeCompare(x, y []byte) int {
if len(x) != len(y) {
panic("subtle: slices have different lengths")
}
...Fixed in Go 1.4 pkg/crypto/subtle/constant_time.go// ConstantTimeCompare returns 1 iff the two slices, x
// and y, have equal contents. The time taken is a function of the length of
// the slices and is independent of the contents.
func ConstantTimeCompare(x, y []byte) int {
if len(x) != len(y) {
return 0
}
...It would be nice to have a more robust suite of tests around claim validation - this issue should cause panics in 1.3 if a JWT claim for |
|
Might make sense to figure out sunsetting 1.3 support. How do you think that interacts with semver? |
| } else { | ||
| return false | ||
|
|
||
| for _, a := range aud { |
There was a problem hiding this comment.
There's some information leaked here, since an early match will take less time. I'm not sure if that presents an actual security risk, though.
There was a problem hiding this comment.
Yeah that's a good point- we can run the comparison on all items, and return true if any of them matched. On a related note- the way ConstantTimeCompare() is written can leak information on the length of the string since when the strings are not equal in length it takes less time.
| aud := m["aud"] | ||
|
|
||
| switch aud.(type) { | ||
| default: |
There was a problem hiding this comment.
Does this work with the default case first? I thought it had to be last...
There was a problem hiding this comment.
It works first, I saw this first at https://golang.org/doc/effective_go.html#type_switch . I agree it probably reads better last though.
I'm not sure exactly about what to do with Semver for dropping a supported language version. I think it just needs to be communicated clearly. My initial thought is since there are no breaking API changes, this is definitely not a major release. Dropping Go 1.3 is notable enough that you may want it to be a minor point release (3.1). |
kekoav
changed the title
|
@dgrijalva any feedback on this PR? I'm happy to improve this PR based on your feedback. |
| matchesAny := false | ||
| for _, a := range aud { | ||
| if subtle.ConstantTimeCompare([]byte(a), []byte(cmp)) != 0 { | ||
| matchesAny = true |
There was a problem hiding this comment.
You can break here, or simply return true
| func verifyAud(aud string, cmp string, required bool) bool { | ||
| if aud == "" { | ||
| func verifyAud(aud []string, cmp string, required bool) bool { | ||
| if len(aud) <= 0 || aud[0] == "" { |
|
A modified version of this has landed in the v4 branch. |
This references issue #184 .
The goal is to support compliance with RFC 7519 Section 4.1.
JWT audience claims
audshould support an array of validaudvalues in the token. When validating a token, a singleaudvalue is provided and compared against the array of aud claims.Care was taken to maintain backward compatibility with the
stringversion ofaudas it works today. This is also part of the spec, so this should not be a breaking change at all. A single string is supported and is treated as a list of one item.The library already supports creation of JWTs with this structure using the
MapClaims, but obviously theStandardClaimsstruct is too rigid for this use case.Please give me your feedback- I'm happy to improve this pull request as needed.