From 82785c70eb98def7205795e271d0fe5ffd3ba83f Mon Sep 17 00:00:00 2001 From: Aydin Mercan Date: Wed, 3 Nov 2021 20:41:03 +0300 Subject: [PATCH] internal/publickey: move from public jwk add tests Moving algorithm specific jwk code to an internal module. Not caring about it should be fine(?) to the end user as long as the library remains vigilantic about it. Also, the crypto.PublicKey in JWK isn't meant to be used directly anyway. --- README.md | 1 + go.sum | 6 ++ {jwk => internal/publickey}/ecdsa.go | 3 +- internal/publickey/ecdsa_test.go | 83 ++++++++++++++++++++++++++++ {jwk => internal/publickey}/eddsa.go | 2 +- internal/publickey/jwk.go | 3 + {jwk => internal/publickey}/rsa.go | 7 ++- internal/publickey/rsa_test.go | 56 +++++++++++++++++++ jwk/jwk.go | 7 ++- jwk/jwk_test.go | 4 +- 10 files changed, 163 insertions(+), 9 deletions(-) rename {jwk => internal/publickey}/ecdsa.go (98%) create mode 100644 internal/publickey/ecdsa_test.go rename {jwk => internal/publickey}/eddsa.go (97%) create mode 100644 internal/publickey/jwk.go rename {jwk => internal/publickey}/rsa.go (88%) create mode 100644 internal/publickey/rsa_test.go diff --git a/README.md b/README.md index c014cba..1b26112 100644 --- a/README.md +++ b/README.md @@ -17,6 +17,7 @@ Likewise, you shouldn't need me to tell you that you shouldn't use this library. * Just enough JWT for people to speak commonly encountered OAuth 2.0 and alike. * Don't allow for any of the sharp edges. * Allow for binding domain parameters as much as possible to the public keys. +* Extensive test coverage even if a particular case seems pedantic, guaranteed to be handled properly and/or improbable to be problematic. ## Non-Goals diff --git a/go.sum b/go.sum index cbb0e79..814b1a3 100644 --- a/go.sum +++ b/go.sum @@ -1,2 +1,8 @@ golang.org/x/crypto v0.0.0-20210921155107-089bfa567519 h1:7I4JAnoQBe7ZtJcBaYHi5UtiO8tQHbUSXxL+pnGRANg= golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= +golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= +golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= +golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= +golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= diff --git a/jwk/ecdsa.go b/internal/publickey/ecdsa.go similarity index 98% rename from jwk/ecdsa.go rename to internal/publickey/ecdsa.go index e921c70..595a1e0 100644 --- a/jwk/ecdsa.go +++ b/internal/publickey/ecdsa.go @@ -1,4 +1,4 @@ -package jwk +package publickey import ( "crypto/ecdsa" @@ -68,5 +68,4 @@ func ParseECDSAPublicKey(data []byte) (*ecdsa.PublicKey, error) { } return key, nil - } diff --git a/internal/publickey/ecdsa_test.go b/internal/publickey/ecdsa_test.go new file mode 100644 index 0000000..11f8665 --- /dev/null +++ b/internal/publickey/ecdsa_test.go @@ -0,0 +1,83 @@ +package publickey_test + +import ( + "crypto/elliptic" + "mercan.dev/dumb-jose/internal/publickey" + "testing" +) + +const ( + ValidECDSAPublicKey = `{ + "crv": "P-256", + "x": "MKBCTNIcKUSDii11ySs3526iDZ8AiTo7Tu6KPAqv7D4", + "y": "4Etl6SRW2YiLUrN5vfvVHuhp7x8PxltmWWlbbM4IFyM" + }` + + InvalidCurveType = `{ + "crv": "p-256", + "x": "MKBCTNIcKUSDii11ySs3526iDZ8AiTo7Tu6KPAqv7D4", + "y": "4Etl6SRW2YiLUrN5vfvVHuhp7x8PxltmWWlbbM4IFyM" + }` + + InvalidCurvePoint = `{ + "crv": "P-521", + "x": "f83OJ3D2xF1Bg8vub9tLe1gHMzV76e8Tus9uPHvRVEU", + "y": "x_FEzRu9m36HLN_tue659LNpXW6pCyStikYjKIWI5a0" + }` +) + +var ( + IncompleteECDSAPublicKeyPermutation = []string{ + `{"crv": "P-256", "x": "MKBCTNIcKUSDii11ySs3526iDZ8AiTo7Tu6KPAqv7D4"}`, + `{"crv": "P-256", "y": "4Etl6SRW2YiLUrN5vfvVHuhp7x8PxltmWWlbbM4IFyM"}`, + `{"x": "f83OJ3D2xF1Bg8vub9tLe1gHMzV76e8Tus9uPHvRVEU", "y": "x_FEzRu9m36HLN_tue659LNpXW6pCyStikYjKIWI5a0"}`, + } + + MalformedKeyJSON = []string{ + `Wait this isn't even JSON!`, + `{"crv": "P-521", "x": 1234567890, "y": "4Etl6SRW2YiLUrN5vfvVHuhp7x8PxltmWWlbbM4IFyM}"`, + } +) + +func TestValidCurvePoint(t *testing.T) { + key, err := publickey.ParseECDSAPublicKey([]byte(ValidECDSAPublicKey)) + if err != nil { + t.Fatalf("Expected pass while parsing, found %v", err) + } + + if key.Curve != elliptic.P256() { + t.Fatalf("Expected P-256 curve found %s", key.Params().Name) + } +} + +func TestInvalidCurveTypeDenial(t *testing.T) { + _, err := publickey.ParseECDSAPublicKey([]byte(InvalidCurveType)) + if err == nil { + t.Errorf("Expected failure for curve type but passed") + } +} + +func TestInvalidCurvePointDenial(t *testing.T) { + _, err := publickey.ParseECDSAPublicKey([]byte(InvalidCurvePoint)) + if err != publickey.ErrInvalidCurvePoint { + t.Errorf("Expected invalid curve point failure, found: %v", err) + } +} + +func TestIncompleteHeaderDenial(t *testing.T) { + for _, key := range IncompleteECDSAPublicKeyPermutation { + _, err := publickey.ParseECDSAPublicKey([]byte(key)) + if err == nil { + t.Errorf("Expected error") + } + } +} + +func TestMalformedHeaderDenial(t *testing.T) { + for _, key := range MalformedKeyJSON { + _, err := publickey.ParseECDSAPublicKey([]byte(key)) + if err == nil { + t.Errorf("Expected to fail but didn't") + } + } +} diff --git a/jwk/eddsa.go b/internal/publickey/eddsa.go similarity index 97% rename from jwk/eddsa.go rename to internal/publickey/eddsa.go index 557454e..d64c679 100644 --- a/jwk/eddsa.go +++ b/internal/publickey/eddsa.go @@ -1,4 +1,4 @@ -package jwk +package publickey import ( "encoding/base64" diff --git a/internal/publickey/jwk.go b/internal/publickey/jwk.go new file mode 100644 index 0000000..7650b68 --- /dev/null +++ b/internal/publickey/jwk.go @@ -0,0 +1,3 @@ +package publickey + +type JWKPublicKeyHeader interface{} diff --git a/jwk/rsa.go b/internal/publickey/rsa.go similarity index 88% rename from jwk/rsa.go rename to internal/publickey/rsa.go index 1c5958d..748e1d9 100644 --- a/jwk/rsa.go +++ b/internal/publickey/rsa.go @@ -1,10 +1,11 @@ -package jwk +package publickey import ( "crypto/rsa" "encoding/base64" "encoding/json" "errors" + "fmt" "math/big" ) @@ -26,6 +27,10 @@ func ParseRSAPublicKey(data []byte) (*rsa.PublicKey, error) { return nil, err } + if header.Modulus == "" { + return nil, fmt.Errorf("Empty N") + } + rawN, err := base64.RawURLEncoding.DecodeString(header.Modulus) if err != nil { return nil, err diff --git a/internal/publickey/rsa_test.go b/internal/publickey/rsa_test.go new file mode 100644 index 0000000..952e406 --- /dev/null +++ b/internal/publickey/rsa_test.go @@ -0,0 +1,56 @@ +package publickey_test + +import ( + "mercan.dev/dumb-jose/internal/publickey" + "testing" +) + +const ( + ValidRSAPublicKey = `{ + "e": "AQAB", + "n": "0vx7agoebGcQSuuPiLJXZptN9nndrQmbXEps2aiAFbWhM78LhWx4cbbfAAtVT86zwu1RK7aPFFxuhDR1L6tSoc_BJECPebWKRXjBZCiFV4n3oknjhMstn64tZ_2W-5JsGY4Hc5n9yBXArwl93lqt7_RN5w6Cf0h4QyQ5v-65YGjQR0_FDW2QvzqY368QQMicAtaSqzs8KJZgnYb9c7d0zgdAZHzu6qMQvRL5hajrn1n91CbOpbISD08qNLyrdkt-bFTWhAI4vMQFh6WeZu0fM4lFd2NcRwr3XPksINHaQ-G_xBniIqbw0Ls1jF44-csFCur-kEgU8awapJzKnqDKgw" + }` + + InvalidExponent = `{ + "e": "Aw", + "n": "0vx7agoebGcQSuuPiLJXZptN9nndrQmbXEps2aiAFbWhM78LhWx4cbbfAAtVT86zwu1RK7aPFFxuhDR1L6tSoc_BJECPebWKRXjBZCiFV4n3oknjhMstn64tZ_2W-5JsGY4Hc5n9yBXArwl93lqt7_RN5w6Cf0h4QyQ5v-65YGjQR0_FDW2QvzqY368QQMicAtaSqzs8KJZgnYb9c7d0zgdAZHzu6qMQvRL5hajrn1n91CbOpbISD08qNLyrdkt-bFTWhAI4vMQFh6WeZu0fM4lFd2NcRwr3XPksINHaQ-G_xBniIqbw0Ls1jF44-csFCur-kEgU8awapJzKnqDKgw" + }` +) + +var ( + IncompleteRSAPublicKeyPermutation = []string{ + `{ "e": "AQAB" }`, + `{ "n": "0vx7agoebGcQSuuPiLJXZptN9nndrQmbXEps2aiAFbWhM78LhWx4cbbfAAtVT86zwu1RK7aPFFxuhDR1L6tSoc_BJECPebWKRXjBZCiFV4n3oknjhMstn64tZ_2W-5JsGY4Hc5n9yBXArwl93lqt7_RN5w6Cf0h4QyQ5v-65YGjQR0_FDW2QvzqY368QQMicAtaSqzs8KJZgnYb9c7d0zgdAZHzu6qMQvRL5hajrn1n91CbOpbISD08qNLyrdkt-bFTWhAI4vMQFh6WeZu0fM4lFd2NcRwr3XPksINHaQ-G_xBniIqbw0Ls1jF44-csFCur-kEgU8awapJzKnqDKgw" }`, + } + + MalformedRSAPublicKey = []string{ + `Wait this isn't even JSON!`, + } +) + +func TestRSAValidPublicKey(t *testing.T) { + key, err := publickey.ParseRSAPublicKey([]byte(ValidRSAPublicKey)) + if err != nil { + t.Fatalf("Expected no errors, found %v", err) + } + + if key.E != 65537 { + t.Fatalf("Expected e to be 65537, found %d", key.E) + } +} + +func TestRSAInvalidExponentDenial(t *testing.T) { + _, err := publickey.ParseRSAPublicKey([]byte(InvalidExponent)) + if err != publickey.ErrUnsupportedPublicExponent { + t.Fatalf("Expected errors, found none") + } +} + +func TestRSAIncompleteHeaderDenial(t *testing.T) { + for _, key := range IncompleteRSAPublicKeyPermutation { + _, err := publickey.ParseRSAPublicKey([]byte(key)) + if err == nil { + t.Errorf("Expected to fail for %s but didn't", key) + } + } +} diff --git a/jwk/jwk.go b/jwk/jwk.go index 15cb7f3..ee32f55 100644 --- a/jwk/jwk.go +++ b/jwk/jwk.go @@ -4,6 +4,7 @@ import ( "crypto" "encoding/json" "fmt" + "mercan.dev/dumb-jose/internal/publickey" ) type PublicKeyHeader interface{} @@ -48,19 +49,19 @@ func ParseJWKKeysFromSet(data []byte) ([]JWK, error) { return nil, fmt.Errorf("Insuitable key type %s for algorithm %s", jwk.KeyType, jwk.Algorithm) } - jwk.PublicKey, err = ParseECDSAPublicKey(key) + jwk.PublicKey, err = publickey.ParseECDSAPublicKey(key) case "RS256", "RS384", "RS512", "PS256", "PS384", "PS512": if jwk.KeyType != "RSA" { return nil, fmt.Errorf("Insuitable key type %s for algorithm %s", jwk.KeyType, jwk.Algorithm) } - jwk.PublicKey, err = ParseRSAPublicKey(key) + jwk.PublicKey, err = publickey.ParseRSAPublicKey(key) case "EdDSA": if jwk.KeyType != "OKP" { return nil, fmt.Errorf("Insuitable key type %s for algorithm %s", jwk.KeyType, jwk.Algorithm) } - jwk.PublicKey, err = ParseEdDSAPublicKey(key) + jwk.PublicKey, err = publickey.ParseEdDSAPublicKey(key) default: return nil, fmt.Errorf("Invalid/Unsupported JWK Algorithm %s", jwk.Algorithm) } diff --git a/jwk/jwk_test.go b/jwk/jwk_test.go index afaba30..36745c9 100644 --- a/jwk/jwk_test.go +++ b/jwk/jwk_test.go @@ -40,7 +40,7 @@ func TestCorrectJwkKeySet(t *testing.T) { for i, kid := range []string{"bbd2ac7c4c5eb8adc8eeffbc8f5a2dd6cf7545e4", "85828c59284a69b54b27483e487c3bd46cd2a2b3"} { if set[i].KeyID != kid { - t.Errorf("hhhhh") + t.Errorf("Key ID Mismatch (%s vs %s)", set[i].KeyID, kid) t.Fail() } } @@ -54,7 +54,7 @@ func TestCorrectJwkKeySet(t *testing.T) { func TestInvalidRSAExponent(t *testing.T) { _, err := jwk.ParseJWKKeysFromSet([]byte(InvalidExponent)) - if err != jwk.ErrUnsupportedPublicExponent { + if err == nil { t.Errorf("Expected error not returned for unsupported public exponent, found \"%v\"", err) } }