From 1323f1fadbc969bca41fb29a57db9121601f3709 Mon Sep 17 00:00:00 2001 From: Filipe Azevedo Date: Thu, 30 Apr 2020 19:02:04 +0100 Subject: [PATCH] Fix decoding x5t parameters (#304) 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 --- .travis.yml | 2 + jwk.go | 63 ++++++++++++++++++++----- jwk_test.go | 131 ++++++++++++++++++++++++++++++++++++++++++---------- 3 files changed, 160 insertions(+), 36 deletions(-) diff --git a/.travis.yml b/.travis.yml index e8f12eb..ae69862 100644 --- a/.travis.yml +++ b/.travis.yml @@ -26,6 +26,8 @@ before_install: - go get github.com/wadey/gocovmerge - go get github.com/mattn/goveralls - go get github.com/stretchr/testify/assert +- go get github.com/stretchr/testify/require +- go get github.com/google/go-cmp/cmp - go get golang.org/x/tools/cmd/cover || true - go get code.google.com/p/go.tools/cmd/cover || true - pip install cram --user diff --git a/jwk.go b/jwk.go index 578f471..2dc6aec 100644 --- a/jwk.go +++ b/jwk.go @@ -26,6 +26,7 @@ import ( "crypto/sha256" "crypto/x509" "encoding/base64" + "encoding/hex" "errors" "fmt" "math/big" @@ -61,10 +62,10 @@ type rawJSONWebKey struct { Dq *byteBuffer `json:"dq,omitempty"` Qi *byteBuffer `json:"qi,omitempty"` // Certificates - X5c []string `json:"x5c,omitempty"` - X5u *url.URL `json:"x5u,omitempty"` - X5tSHA1 *byteBuffer `json:"x5t,omitempty"` - X5tSHA256 *byteBuffer `json:"x5t#S256,omitempty"` + X5c []string `json:"x5c,omitempty"` + X5u *url.URL `json:"x5u,omitempty"` + X5tSHA1 string `json:"x5t,omitempty"` + X5tSHA256 string `json:"x5t#S256,omitempty"` } // JSONWebKey represents a public or private key in JWK format. @@ -130,13 +131,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) } 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) } // If cert chain is attached (as opposed to being behind a URL), check the @@ -146,9 +147,12 @@ func (k JSONWebKey) MarshalJSON() ([]byte, error) { if len(k.Certificates) > 0 { expectedSHA1 := sha1.Sum(k.Certificates[0].Raw) expectedSHA256 := sha256.Sum256(k.Certificates[0].Raw) - if !bytes.Equal(k.CertificateThumbprintSHA1, expectedSHA1[:]) || - !bytes.Equal(k.CertificateThumbprintSHA256, expectedSHA256[:]) { - return nil, errors.New("square/go-jose: invalid SHA-1 or SHA-256 thumbprint, does not match cert chain") + + if len(k.CertificateThumbprintSHA1) > 0 && !bytes.Equal(k.CertificateThumbprintSHA1, expectedSHA1[:]) { + return nil, errors.New("square/go-jose: invalid SHA-1 thumbprint, does not match cert chain") + } + if len(k.CertificateThumbprintSHA256) > 0 && !bytes.Equal(k.CertificateThumbprintSHA256, expectedSHA256[:]) { + return nil, errors.New("square/go-jose: invalid or SHA-256 thumbprint, does not match cert chain") } } @@ -241,8 +245,43 @@ func (k *JSONWebKey) UnmarshalJSON(data []byte) (err error) { *k = JSONWebKey{Key: key, KeyID: raw.Kid, Algorithm: raw.Alg, Use: raw.Use, Certificates: certs} k.CertificatesURL = raw.X5u - k.CertificateThumbprintSHA1 = raw.X5tSHA1.bytes() - k.CertificateThumbprintSHA256 = raw.X5tSHA256.bytes() + + // x5t parameters are base64url-encoded SHA thumbprints + // See RFC 7517, Section 4.8, https://tools.ietf.org/html/rfc7517#section-4.8 + x5tSHA1bytes, err := base64.RawURLEncoding.DecodeString(raw.X5tSHA1) + if err != nil { + return errors.New("square/go-jose: invalid JWK, x5t header has invalid encoding") + } + + // RFC 7517, Section 4.8 is ambiguous as to whether the digest output should be byte or hex, + // for this reason, after base64 decoding, if the size is sha1.Size it's likely that the value is a byte encoded + // 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 { + hx, err := hex.DecodeString(string(x5tSHA1bytes)) + if err != nil { + return fmt.Errorf("square/go-jose: invalid JWK, unable to hex decode x5t: %v", err) + + } + x5tSHA1bytes = hx + } + + k.CertificateThumbprintSHA1 = x5tSHA1bytes + + x5tSHA256bytes, err := base64.RawURLEncoding.DecodeString(raw.X5tSHA256) + if err != nil { + return errors.New("square/go-jose: invalid JWK, x5t#S256 header has invalid encoding") + } + + if len(x5tSHA256bytes) == 2*sha256.Size { + hx256, err := hex.DecodeString(string(x5tSHA256bytes)) + if err != nil { + return fmt.Errorf("square/go-jose: invalid JWK, unable to hex decode x5t#S256: %v", err) + } + x5tSHA256bytes = hx256 + } + + k.CertificateThumbprintSHA256 = x5tSHA256bytes x5tSHA1Len := len(k.CertificateThumbprintSHA1) x5tSHA256Len := len(k.CertificateThumbprintSHA256) @@ -250,7 +289,7 @@ func (k *JSONWebKey) UnmarshalJSON(data []byte) (err error) { return errors.New("square/go-jose: invalid JWK, x5t header is of incorrect size") } if x5tSHA256Len > 0 && x5tSHA256Len != sha256.Size { - return errors.New("square/go-jose: invalid JWK, x5t header is of incorrect size") + return errors.New("square/go-jose: invalid JWK, x5t#S256 header is of incorrect size") } // If certificate chain *and* thumbprints are set, verify correctness. diff --git a/jwk_test.go b/jwk_test.go index 23bcef7..0f92879 100644 --- a/jwk_test.go +++ b/jwk_test.go @@ -31,7 +31,10 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/crypto/ed25519" "gopkg.in/square/go-jose.v2/json" @@ -246,37 +249,117 @@ func TestRoundtripX509(t *testing.T) { x5tSHA1 := sha1.Sum(testCertificates[0].Raw) x5tSHA256 := sha256.Sum256(testCertificates[0].Raw) - jwk := JSONWebKey{ - Key: testCertificates[0].PublicKey, - KeyID: "bar", - Algorithm: "foo", - Certificates: testCertificates, - CertificateThumbprintSHA1: x5tSHA1[:], - CertificateThumbprintSHA256: x5tSHA256[:], + cases := []struct { + name string + jwk JSONWebKey + }{ + { + name: "all fields", + jwk: JSONWebKey{ + Key: testCertificates[0].PublicKey, + KeyID: "bar", + Algorithm: "foo", + Certificates: testCertificates, + CertificateThumbprintSHA1: x5tSHA1[:], + CertificateThumbprintSHA256: x5tSHA256[:], + }, + }, + { + name: "no optional x5ts", + jwk: JSONWebKey{ + Key: testCertificates[0].PublicKey, + KeyID: "bar", + Algorithm: "foo", + Certificates: testCertificates, + }, + }, + { + name: "no x5t", + jwk: JSONWebKey{ + Key: testCertificates[0].PublicKey, + KeyID: "bar", + Algorithm: "foo", + Certificates: testCertificates, + CertificateThumbprintSHA256: x5tSHA256[:], + }, + }, + { + name: "no x5t#S256", + jwk: JSONWebKey{ + Key: testCertificates[0].PublicKey, + KeyID: "bar", + Algorithm: "foo", + Certificates: testCertificates, + CertificateThumbprintSHA1: x5tSHA1[:], + }, + }, } - jsonbar, err := jwk.MarshalJSON() - if err != nil { - t.Error("problem marshaling", err) + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + jsonbar, err := c.jwk.MarshalJSON() + require.NoError(t, err) + + var jwk2 JSONWebKey + err = jwk2.UnmarshalJSON(jsonbar) + require.NoError(t, err) + + if !reflect.DeepEqual(testCertificates, jwk2.Certificates) { + t.Error("Certificates not equal", c.jwk.Certificates, jwk2.Certificates) + } + + jsonbar2, err := jwk2.MarshalJSON() + require.NoError(t, err) + + require.Empty(t, cmp.Diff(jsonbar, jsonbar2)) + if !bytes.Equal(jsonbar, jsonbar2) { + t.Error("roundtrip should not lose information") + } + }) } +} + +func TestRoundtripX509Hex(t *testing.T) { + var hexJWK = `{ + "kty":"RSA", + "kid":"bar", + "alg":"foo", + "n":"u7LUr30Mhrh8N79-H4rKiHQ123q6xaBZPYbf1nV4GM19rizSnbEfyebG1kpfCv-XY6c499XiM6lOvcPL-0goTOcfW6Lg7AAR895GbnMeXEmnxICaI8rAZHK6t1WPmiWp82y_qhK2F_pYUaT3GSuiTFiMGq_GNwdpWuMlsInnnMNv1nxFbxtDPwzmCp0fEBxbH5d1EtXZwTPOHMyj8rfa-NIA5Nl4h_5RrbOWveKwBr26_CDAratJgOWh9xcd5g0ot_uDGcMoAgB6xeTuYklfaxCPptvu49kvoxw1J71fp6nKW_ZuhDRAp2F_BQ9inKpTo05sPLJg8tPTdjaeouOuJQ", + "e":"AQAB", + "x5c":[ + "MIIDfDCCAmSgAwIBAgIJANWAkzF7PA8/MA0GCSqGSIb3DQEBCwUAMFUxCzAJBgNVBAYTAlVTMQswCQYDVQQIEwJDQTEQMA4GA1UEChMHY2VydGlnbzEQMA4GA1UECxMHZXhhbXBsZTEVMBMGA1UEAxMMZXhhbXBsZS1sZWFmMB4XDTE2MDYxMDIyMTQxMVoXDTIzMDQxNTIyMTQxMVowVTELMAkGA1UEBhMCVVMxCzAJBgNVBAgTAkNBMRAwDgYDVQQKEwdjZXJ0aWdvMRAwDgYDVQQLEwdleGFtcGxlMRUwEwYDVQQDEwxleGFtcGxlLWxlYWYwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQC7stSvfQyGuHw3v34fisqIdDXberrFoFk9ht/WdXgYzX2uLNKdsR/J5sbWSl8K/5djpzj31eIzqU69w8v7SChM5x9bouDsABHz3kZucx5cSafEgJojysBkcrq3VY+aJanzbL+qErYX+lhRpPcZK6JMWIwar8Y3B2la4yWwieecw2/WfEVvG0M/DOYKnR8QHFsfl3US1dnBM84czKPyt9r40gDk2XiH/lGts5a94rAGvbr8IMCtq0mA5aH3Fx3mDSi3+4MZwygCAHrF5O5iSV9rEI+m2+7j2S+jHDUnvV+nqcpb9m6ENECnYX8FD2KcqlOjTmw8smDy09N2Np6i464lAgMBAAGjTzBNMB0GA1UdJQQWMBQGCCsGAQUFBwMCBggrBgEFBQcDATAsBgNVHREEJTAjhwR/AAABhxAAAAAAAAAAAAAAAAAAAAABgglsb2NhbGhvc3QwDQYJKoZIhvcNAQELBQADggEBAGM4aa/qrURUweZBIwZYv8O9b2+r4l0HjGAh982/B9sMlM05kojyDCUGvj86z18Lm8mKr4/y+i0nJ+vDIksEvfDuzw5ALAXGcBzPJKtICUf7LstA/n9NNpshWz0kld9ylnB5mbUzSFDncVyeXkEf5sGQXdIIZT9ChRBoiloSaa7dvBVCcsX1LGP2LWqKtD+7nUnw5qCwtyAVT8pthEUxFTpywoiJS5ZdzeEx8MNGvUeLFj2kleqPF78EioEQlSOxViCuctEtnQuPcDLHNFr10byTZY9roObiqdsJLMVvb2XliJjAqaPa9AkYwGE6xHw2ispwg64Rse0+AtKups19WIU=", + "MIIDUzCCAjugAwIBAgIJAKg+LQlirffwMA0GCSqGSIb3DQEBCwUAMFUxCzAJBgNVBAYTAlVTMQswCQYDVQQIEwJDQTEQMA4GA1UEChMHY2VydGlnbzEQMA4GA1UECxMHZXhhbXBsZTEVMBMGA1UEAxMMZXhhbXBsZS1yb290MB4XDTE2MDYxMDIyMTQxMVoXDTIzMDQxNTIyMTQxMVowVTELMAkGA1UEBhMCVVMxCzAJBgNVBAgTAkNBMRAwDgYDVQQKEwdjZXJ0aWdvMRAwDgYDVQQLEwdleGFtcGxlMRUwEwYDVQQDEwxleGFtcGxlLXJvb3QwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDKOEoSiNjMQ8/zUFcQW89LWw+UeTXKGwNDSpGjyi8jBKZ1lWPbnMmrjI6DZ9ReevHHzqBdKZt+9NFPFEz7djDMRByIuJhRvzhfFBflaIdSeNk2+NpUaFuUUUd6IIePu0AdRveJ8ZGHXRwCeEDIVCZS4oBYPHOhX/zMWDg8vSO4pSxTjGc7I8fHxaUSkVzUBbeO9T/1eFk0m2uxs3UziUck2X/8YqRd+p/EaBED78nXvKRALAguKAzqxIgk3ccPK0SVQFNFq+eV1/qo8coueQuqMpCAvwVkfpVKhneyC2NlMrfzlcZZbfG/irlSjQn5+ExZX4Isy1pCUbOiVfSrsCdtAgMBAAGjJjAkMA4GA1UdDwEB/wQEAwICBDASBgNVHRMBAf8ECDAGAQH/AgEAMA0GCSqGSIb3DQEBCwUAA4IBAQCLEJU65vTU+oLbNHLOCR6fALrbjK7xsi6SFDpSXBMm74MWsy3myDBmXpOcN8hCYgsgivUXTQz9ynXP/pzOj4b83zzlaOfPtLTAmMhKWVV4Q85mrDQz+HzG4lKXM78eTsD8PyrocA/tSE7mVEJ0Jal4E2KI/Z9/fqpYFLB6LFlx5n83ehXM/egA0l4OeCC9nBKCeNUN3sIQO85lljyzAJdtWnsdoWogJs6qjcV8n2U5xjZxN5ZFdclYLjq6g2cjEXXMQxb8b7ZhHjLWFdjHP85UvXHK3DpK3JmUg8bYS7t1DJffDQNjawhlsMycKZN+r0ND0Um4m7AjGqxbKT/M2yKF" + ], + "x5t": "MDYxMjU0ZmRmNzIwZjJjMGU0YmQzZjMzMzlhMmZlNTM1MGExNWRlMQ", + "x5t#S256": "MjAzMjRhNGI5MmYxMjI2OGVmOWFlMDI1ZmQ1Yzc5ZDE1OGZmNzQ1NzQwMDkyMTk2ZTgzNTNjMDAzMTUxNzUxMQ" +}` + + // json output + var output = `{ + "kty":"RSA", + "kid":"bar", + "alg":"foo", + "n":"u7LUr30Mhrh8N79-H4rKiHQ123q6xaBZPYbf1nV4GM19rizSnbEfyebG1kpfCv-XY6c499XiM6lOvcPL-0goTOcfW6Lg7AAR895GbnMeXEmnxICaI8rAZHK6t1WPmiWp82y_qhK2F_pYUaT3GSuiTFiMGq_GNwdpWuMlsInnnMNv1nxFbxtDPwzmCp0fEBxbH5d1EtXZwTPOHMyj8rfa-NIA5Nl4h_5RrbOWveKwBr26_CDAratJgOWh9xcd5g0ot_uDGcMoAgB6xeTuYklfaxCPptvu49kvoxw1J71fp6nKW_ZuhDRAp2F_BQ9inKpTo05sPLJg8tPTdjaeouOuJQ", + "e":"AQAB", + "x5c":[ + "MIIDfDCCAmSgAwIBAgIJANWAkzF7PA8/MA0GCSqGSIb3DQEBCwUAMFUxCzAJBgNVBAYTAlVTMQswCQYDVQQIEwJDQTEQMA4GA1UEChMHY2VydGlnbzEQMA4GA1UECxMHZXhhbXBsZTEVMBMGA1UEAxMMZXhhbXBsZS1sZWFmMB4XDTE2MDYxMDIyMTQxMVoXDTIzMDQxNTIyMTQxMVowVTELMAkGA1UEBhMCVVMxCzAJBgNVBAgTAkNBMRAwDgYDVQQKEwdjZXJ0aWdvMRAwDgYDVQQLEwdleGFtcGxlMRUwEwYDVQQDEwxleGFtcGxlLWxlYWYwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQC7stSvfQyGuHw3v34fisqIdDXberrFoFk9ht/WdXgYzX2uLNKdsR/J5sbWSl8K/5djpzj31eIzqU69w8v7SChM5x9bouDsABHz3kZucx5cSafEgJojysBkcrq3VY+aJanzbL+qErYX+lhRpPcZK6JMWIwar8Y3B2la4yWwieecw2/WfEVvG0M/DOYKnR8QHFsfl3US1dnBM84czKPyt9r40gDk2XiH/lGts5a94rAGvbr8IMCtq0mA5aH3Fx3mDSi3+4MZwygCAHrF5O5iSV9rEI+m2+7j2S+jHDUnvV+nqcpb9m6ENECnYX8FD2KcqlOjTmw8smDy09N2Np6i464lAgMBAAGjTzBNMB0GA1UdJQQWMBQGCCsGAQUFBwMCBggrBgEFBQcDATAsBgNVHREEJTAjhwR/AAABhxAAAAAAAAAAAAAAAAAAAAABgglsb2NhbGhvc3QwDQYJKoZIhvcNAQELBQADggEBAGM4aa/qrURUweZBIwZYv8O9b2+r4l0HjGAh982/B9sMlM05kojyDCUGvj86z18Lm8mKr4/y+i0nJ+vDIksEvfDuzw5ALAXGcBzPJKtICUf7LstA/n9NNpshWz0kld9ylnB5mbUzSFDncVyeXkEf5sGQXdIIZT9ChRBoiloSaa7dvBVCcsX1LGP2LWqKtD+7nUnw5qCwtyAVT8pthEUxFTpywoiJS5ZdzeEx8MNGvUeLFj2kleqPF78EioEQlSOxViCuctEtnQuPcDLHNFr10byTZY9roObiqdsJLMVvb2XliJjAqaPa9AkYwGE6xHw2ispwg64Rse0+AtKups19WIU=", + "MIIDUzCCAjugAwIBAgIJAKg+LQlirffwMA0GCSqGSIb3DQEBCwUAMFUxCzAJBgNVBAYTAlVTMQswCQYDVQQIEwJDQTEQMA4GA1UEChMHY2VydGlnbzEQMA4GA1UECxMHZXhhbXBsZTEVMBMGA1UEAxMMZXhhbXBsZS1yb290MB4XDTE2MDYxMDIyMTQxMVoXDTIzMDQxNTIyMTQxMVowVTELMAkGA1UEBhMCVVMxCzAJBgNVBAgTAkNBMRAwDgYDVQQKEwdjZXJ0aWdvMRAwDgYDVQQLEwdleGFtcGxlMRUwEwYDVQQDEwxleGFtcGxlLXJvb3QwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDKOEoSiNjMQ8/zUFcQW89LWw+UeTXKGwNDSpGjyi8jBKZ1lWPbnMmrjI6DZ9ReevHHzqBdKZt+9NFPFEz7djDMRByIuJhRvzhfFBflaIdSeNk2+NpUaFuUUUd6IIePu0AdRveJ8ZGHXRwCeEDIVCZS4oBYPHOhX/zMWDg8vSO4pSxTjGc7I8fHxaUSkVzUBbeO9T/1eFk0m2uxs3UziUck2X/8YqRd+p/EaBED78nXvKRALAguKAzqxIgk3ccPK0SVQFNFq+eV1/qo8coueQuqMpCAvwVkfpVKhneyC2NlMrfzlcZZbfG/irlSjQn5+ExZX4Isy1pCUbOiVfSrsCdtAgMBAAGjJjAkMA4GA1UdDwEB/wQEAwICBDASBgNVHRMBAf8ECDAGAQH/AgEAMA0GCSqGSIb3DQEBCwUAA4IBAQCLEJU65vTU+oLbNHLOCR6fALrbjK7xsi6SFDpSXBMm74MWsy3myDBmXpOcN8hCYgsgivUXTQz9ynXP/pzOj4b83zzlaOfPtLTAmMhKWVV4Q85mrDQz+HzG4lKXM78eTsD8PyrocA/tSE7mVEJ0Jal4E2KI/Z9/fqpYFLB6LFlx5n83ehXM/egA0l4OeCC9nBKCeNUN3sIQO85lljyzAJdtWnsdoWogJs6qjcV8n2U5xjZxN5ZFdclYLjq6g2cjEXXMQxb8b7ZhHjLWFdjHP85UvXHK3DpK3JmUg8bYS7t1DJffDQNjawhlsMycKZN+r0ND0Um4m7AjGqxbKT/M2yKF" + ], + "x5t":"BhJU_fcg8sDkvT8zOaL-U1ChXeE", + "x5t#S256":"IDJKS5LxImjvmuAl_Vx50Vj_dFdACSGW6DU8ADFRdRE" + }` var jwk2 JSONWebKey - err = jwk2.UnmarshalJSON(jsonbar) - if err != nil { - t.Fatal("problem unmarshalling", err) - } + err := jwk2.UnmarshalJSON([]byte(hexJWK)) + require.NoError(t, err) - if !reflect.DeepEqual(testCertificates, jwk2.Certificates) { - t.Error("Certificates not equal", jwk.Certificates, jwk2.Certificates) - } + js, err := jwk2.MarshalJSON() + require.NoError(t, err) - jsonbar2, err := jwk2.MarshalJSON() - if err != nil { - t.Error("problem marshaling", err) - } - if !bytes.Equal(jsonbar, jsonbar2) { - t.Error("roundtrip should not lose information") - } + var j1, j2 map[string]interface{} + require.NoError(t, json.Unmarshal(js, &j1)) + require.NoError(t, json.Unmarshal([]byte(output), &j2)) + require.Empty(t, cmp.Diff(j1, j2)) } func TestInvalidThumbprintsX509(t *testing.T) {