Fix x5t #304
Fix x5t #304
Conversation
|
|
| @@ -3,6 +3,7 @@ module github.com/square/go-jose/v3 | |||
| go 1.12 | |||
|
|
|||
| require ( | |||
| github.com/google/go-cmp v0.4.0 | |||
There was a problem hiding this comment.
This is more for output readability on the test but happy to drop it if you'd rather not add the dependency
There was a problem hiding this comment.
especially that Go modules knows when an import is used in tests, and even though this dep is present in go.mod, it will not bloat the actual code.
| @@ -129,13 +130,13 @@ func (k JSONWebKey) MarshalJSON() ([]byte, error) { | |||
| if x5tSHA1Len != sha1.Size { | |||
| return nil, fmt.Errorf("square/go-jose: invalid SHA-1 thumbprint (must be %d bytes, not %d)", sha1.Size, x5tSHA1Len) | |||
| } | |||
| raw.X5tSHA1 = newFixedSizeBuffer(k.CertificateThumbprintSHA1, sha1.Size) | |||
| raw.X5tSHA1 = base64.RawURLEncoding.EncodeToString(k.CertificateThumbprintSHA1) | |||
There was a problem hiding this comment.
This could be newFixedSizeBuffer(k.CertificateThumbprintSHA1, sha1.Size).base64() which I believe has additional error-checking and verifies the size.
There was a problem hiding this comment.
That causes some problems with !bytes.Equal on line 296 that's why I kept this one instead
| } | ||
| if x5tSHA256Len > 0 { | ||
| if x5tSHA256Len != sha256.Size { | ||
| return nil, fmt.Errorf("square/go-jose: invalid SHA-256 thumbprint (must be %d bytes, not %d)", sha256.Size, x5tSHA256Len) | ||
| } | ||
| raw.X5tSHA256 = newFixedSizeBuffer(k.CertificateThumbprintSHA256, sha256.Size) | ||
| raw.X5tSHA256 = base64.RawURLEncoding.EncodeToString(k.CertificateThumbprintSHA256) |
There was a problem hiding this comment.
This could be newFixedSizeBuffer(k.CertificateThumbprintSHA1, sha1.Size).base64() which I believe has additional error-checking and verifies the size.
| } | ||
| } | ||
|
|
||
| func TestRoundtripX509Hex(t *testing.T) { |
|
Looks like there's a build failure but otherwise this seems good. |
|
Build failure was the linter rule but IMO reducing complexity will hurt readability in this particular function |
|
Travis is happy now it seems :) |
jwk.go
Outdated
| // checksum so we skip this. Otherwise if the checksum was hex encoded we expect a 40 byte sized array so we'll | ||
| // try to hex decode it. When Marshalling this value we'll always use a base64 encoded version of byte format checksum. | ||
| if len(x5tSHA1bytes) == 2*sha1.Size { | ||
| if hx, err := hex.DecodeString(string(x5tSHA1bytes)); err == nil { |
There was a problem hiding this comment.
if we can't decode hex, we should error here (especially that we know the length is not a correct SHA1 length) and same for SHA256 hex
There was a problem hiding this comment.
oh I see, we compare the lengths below and then fail. Seems ok to do it this way. Maybe a small comment could help, though
There was a problem hiding this comment.
Fixed with an explicit message here
jwk_test.go
Outdated
| }, | ||
| }, | ||
| { | ||
| name: "no x5t25", |
There was a problem hiding this comment.
small nit: this should probably say no x5t?
| }, | ||
| }, | ||
| { | ||
| name: "no x5t", |
There was a problem hiding this comment.
small nit: this should probably say no x5t#S256?
There was a problem hiding this comment.
@mangas left some comments, but overall I think this is good. Appreciate your work on this! 🙌
When support for optional x5u, x5t, and x5t#S256 parameters in JWK was added in #242 (and subsequently released in 2.5.0) it actually broke parsing of JWKs which included those parameters. See #299 for detailed analysis and discussion. Cherry-picked from #304 Needed minor tweaks, since v2 doesn't use golangci linter nor Go modules. Co-authored-by: Mat Byczkowski <mbyczkowski@gmail.com>
When support for optional x5u, x5t, and x5t#S256 parameters in JWK was added in #242 (and subsequently released in 2.5.0) it actually broke parsing of JWKs which included those parameters. See #299 for detailed analysis and discussion. Cherry-picked from #304 Needed minor tweaks, since v2 doesn't use golangci linter nor Go modules. Co-authored-by: Mat Byczkowski <mbyczkowski@gmail.com>
Fixes #299