From d6ea8b13abba17f384ba446af0d54c4b09b53dd6 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 17 Dec 2020 13:34:50 -0800 Subject: [PATCH 1/4] Upgrade crypto. Related to #435 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 6ab8ffc8..5887ac49 100644 --- a/go.mod +++ b/go.mod @@ -20,7 +20,7 @@ require ( github.com/smallstep/nosql v0.3.0 github.com/urfave/cli v1.22.4 go.step.sm/cli-utils v0.1.0 - go.step.sm/crypto v0.7.1 + go.step.sm/crypto v0.7.2 golang.org/x/crypto v0.0.0-20201016220609-9e8e0b390897 golang.org/x/net v0.0.0-20201021035429-f5854403a974 google.golang.org/api v0.33.0 diff --git a/go.sum b/go.sum index cc71da4b..8d68502a 100644 --- a/go.sum +++ b/go.sum @@ -293,8 +293,8 @@ go.step.sm/cli-utils v0.1.0 h1:uuQ73MuAh5P5Eg+3zfqwrtlTLx5DWSfGqGCrSSjYqdk= go.step.sm/cli-utils v0.1.0/go.mod h1:+t4qCp5NO+080DdGkJxEh3xL5S4TcYC2JTPLMM72b6Y= go.step.sm/crypto v0.6.1 h1:nJoRFGrGNf/mKVVMdWnfLbBfIFt/z4NdJlSL5nipQMQ= go.step.sm/crypto v0.6.1/go.mod h1:AKS4yMZVZD4EGjpSkY4eibuMenrvKCscb+BpWMet8c0= -go.step.sm/crypto v0.7.1 h1:AX9rCGDvSHxfYy117+i7TqNkjhRxuybI584woKqP54c= -go.step.sm/crypto v0.7.1/go.mod h1:AKS4yMZVZD4EGjpSkY4eibuMenrvKCscb+BpWMet8c0= +go.step.sm/crypto v0.7.2 h1:4EFVTDmdLchlw1Z8AgMl+UiExb1nsocbm/7aArEG/SA= +go.step.sm/crypto v0.7.2/go.mod h1:AKS4yMZVZD4EGjpSkY4eibuMenrvKCscb+BpWMet8c0= golang.org/x/crypto v0.0.0-20181203042331-505ab145d0a9/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20190510104115-cbcb75029529/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= From 0cf594a00389a326ed5ef88e4390a19f6ca4a298 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 17 Dec 2020 13:35:14 -0800 Subject: [PATCH 2/4] Validate payload ID. Related to #435 --- authority/provisioner/aws.go | 7 +++ authority/provisioner/aws_test.go | 70 ++++++++++++++++----------- authority/provisioner/options_test.go | 4 +- authority/provisioner/utils_test.go | 7 ++- 4 files changed, 58 insertions(+), 30 deletions(-) diff --git a/authority/provisioner/aws.go b/authority/provisioner/aws.go index ac5ef003..2d07d2da 100644 --- a/authority/provisioner/aws.go +++ b/authority/provisioner/aws.go @@ -631,6 +631,13 @@ func (p *AWS) authorizeToken(token string) (*awsPayload, error) { return nil, errs.Unauthorized("aws.authorizeToken; aws identity document region cannot be empty") } + // Recalculate and validate payload.ID + unique := fmt.Sprintf("%s.%s", p.GetID(), doc.InstanceID) + sum := sha256.Sum256([]byte(unique)) + if payload.ID != strings.ToLower(hex.EncodeToString(sum[:])) { + return nil, errs.Unauthorized("aws.authorizeToken; invalid token id") + } + // According to "rfc7519 JSON Web Token" acceptable skew should be no // more than a few minutes. now := time.Now().UTC() diff --git a/authority/provisioner/aws_test.go b/authority/provisioner/aws_test.go index 063aa666..3feeed1f 100644 --- a/authority/provisioner/aws_test.go +++ b/authority/provisioner/aws_test.go @@ -332,7 +332,7 @@ func TestAWS_authorizeToken(t *testing.T) { p, err := generateAWS() assert.FatalError(t, err) tok, err := generateAWSToken( - "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", + p, "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", "127.0.0.1", "us-west-1", time.Now(), badKey) assert.FatalError(t, err) return test{ @@ -346,7 +346,7 @@ func TestAWS_authorizeToken(t *testing.T) { p, err := generateAWS() assert.FatalError(t, err) tok, err := generateAWSToken( - "instance-id", awsIssuer, p.GetID(), "", "instance-id", + p, "instance-id", awsIssuer, p.GetID(), "", "instance-id", "127.0.0.1", "us-west-1", time.Now(), key) assert.FatalError(t, err) return test{ @@ -360,7 +360,7 @@ func TestAWS_authorizeToken(t *testing.T) { p, err := generateAWS() assert.FatalError(t, err) tok, err := generateAWSToken( - "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "", + p, "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "", "127.0.0.1", "us-west-1", time.Now(), key) assert.FatalError(t, err) return test{ @@ -374,7 +374,7 @@ func TestAWS_authorizeToken(t *testing.T) { p, err := generateAWS() assert.FatalError(t, err) tok, err := generateAWSToken( - "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", + p, "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", "", "us-west-1", time.Now(), key) assert.FatalError(t, err) return test{ @@ -388,7 +388,7 @@ func TestAWS_authorizeToken(t *testing.T) { p, err := generateAWS() assert.FatalError(t, err) tok, err := generateAWSToken( - "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", + p, "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", "127.0.0.1", "", time.Now(), key) assert.FatalError(t, err) return test{ @@ -402,7 +402,7 @@ func TestAWS_authorizeToken(t *testing.T) { p, err := generateAWS() assert.FatalError(t, err) tok, err := generateAWSToken( - "instance-id", "bad-issuer", p.GetID(), p.Accounts[0], "instance-id", + p, "instance-id", "bad-issuer", p.GetID(), p.Accounts[0], "instance-id", "127.0.0.1", "us-west-1", time.Now(), key) assert.FatalError(t, err) return test{ @@ -416,7 +416,7 @@ func TestAWS_authorizeToken(t *testing.T) { p, err := generateAWS() assert.FatalError(t, err) tok, err := generateAWSToken( - "instance-id", awsIssuer, "bad-audience", p.Accounts[0], "instance-id", + p, "instance-id", awsIssuer, "bad-audience", p.Accounts[0], "instance-id", "127.0.0.1", "us-west-1", time.Now(), key) assert.FatalError(t, err) return test{ @@ -431,7 +431,7 @@ func TestAWS_authorizeToken(t *testing.T) { assert.FatalError(t, err) p.DisableCustomSANs = true tok, err := generateAWSToken( - "foo", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", + p, "foo", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", "127.0.0.1", "us-west-1", time.Now(), key) assert.FatalError(t, err) return test{ @@ -445,7 +445,7 @@ func TestAWS_authorizeToken(t *testing.T) { p, err := generateAWS() assert.FatalError(t, err) tok, err := generateAWSToken( - "instance-id", awsIssuer, p.GetID(), "foo", "instance-id", + p, "instance-id", awsIssuer, p.GetID(), "foo", "instance-id", "127.0.0.1", "us-west-1", time.Now(), key) assert.FatalError(t, err) return test{ @@ -460,7 +460,7 @@ func TestAWS_authorizeToken(t *testing.T) { assert.FatalError(t, err) p.InstanceAge = Duration{1 * time.Minute} tok, err := generateAWSToken( - "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", + p, "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", "127.0.0.1", "us-west-1", time.Now().Add(-1*time.Minute), key) assert.FatalError(t, err) return test{ @@ -470,24 +470,27 @@ func TestAWS_authorizeToken(t *testing.T) { err: errors.New("aws.authorizeToken; aws identity document pendingTime is too old"), } }, - "fail/identityCert": func(t *testing.T) test { + "fail/payloadId": func(t *testing.T) test { p, err := generateAWS() - p.IIDRoots = "testdata/certs/aws.crt" + assert.FatalError(t, err) + p2, err := generateAWS() assert.FatalError(t, err) tok, err := generateAWSToken( - "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", + p2, "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", "127.0.0.1", "us-west-1", time.Now(), key) assert.FatalError(t, err) return test{ p: p, token: tok, + code: http.StatusUnauthorized, + err: errors.New("aws.authorizeToken; invalid token id"), } }, "ok": func(t *testing.T) test { p, err := generateAWS() assert.FatalError(t, err) tok, err := generateAWSToken( - "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", + p, "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", "127.0.0.1", "us-west-1", time.Now(), key) assert.FatalError(t, err) return test{ @@ -500,7 +503,20 @@ func TestAWS_authorizeToken(t *testing.T) { p.IIDRoots = "testdata/certs/aws-test.crt" assert.FatalError(t, err) tok, err := generateAWSToken( - "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", + p, "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", + "127.0.0.1", "us-west-1", time.Now(), key) + assert.FatalError(t, err) + return test{ + p: p, + token: tok, + } + }, + "ok/identityCert2": func(t *testing.T) test { + p, err := generateAWS() + p.IIDRoots = "testdata/certs/aws.crt" + assert.FatalError(t, err) + tok, err := generateAWSToken( + p, "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", "127.0.0.1", "us-west-1", time.Now(), key) assert.FatalError(t, err) return test{ @@ -575,51 +591,51 @@ func TestAWS_AuthorizeSign(t *testing.T) { assert.FatalError(t, err) t4, err := generateAWSToken( - "instance-id", awsIssuer, p1.GetID(), p1.Accounts[0], "instance-id", + p1, "instance-id", awsIssuer, p1.GetID(), p1.Accounts[0], "instance-id", "127.0.0.1", "us-west-1", time.Now(), key) assert.FatalError(t, err) failSubject, err := generateAWSToken( - "bad-subject", awsIssuer, p2.GetID(), p2.Accounts[0], "instance-id", + p2, "bad-subject", awsIssuer, p2.GetID(), p2.Accounts[0], "instance-id", "127.0.0.1", "us-west-1", time.Now(), key) assert.FatalError(t, err) failIssuer, err := generateAWSToken( - "instance-id", "bad-issuer", p1.GetID(), p1.Accounts[0], "instance-id", + p1, "instance-id", "bad-issuer", p1.GetID(), p1.Accounts[0], "instance-id", "127.0.0.1", "us-west-1", time.Now(), key) assert.FatalError(t, err) failAudience, err := generateAWSToken( - "instance-id", awsIssuer, "bad-audience", p1.Accounts[0], "instance-id", + p1, "instance-id", awsIssuer, "bad-audience", p1.Accounts[0], "instance-id", "127.0.0.1", "us-west-1", time.Now(), key) assert.FatalError(t, err) failAccount, err := generateAWSToken( - "instance-id", awsIssuer, p1.GetID(), "", "instance-id", + p1, "instance-id", awsIssuer, p1.GetID(), "", "instance-id", "127.0.0.1", "us-west-1", time.Now(), key) assert.FatalError(t, err) failInstanceID, err := generateAWSToken( - "instance-id", awsIssuer, p1.GetID(), p1.Accounts[0], "", + p1, "instance-id", awsIssuer, p1.GetID(), p1.Accounts[0], "", "127.0.0.1", "us-west-1", time.Now(), key) assert.FatalError(t, err) failPrivateIP, err := generateAWSToken( - "instance-id", awsIssuer, p1.GetID(), p1.Accounts[0], "instance-id", + p1, "instance-id", awsIssuer, p1.GetID(), p1.Accounts[0], "instance-id", "", "us-west-1", time.Now(), key) assert.FatalError(t, err) failRegion, err := generateAWSToken( - "instance-id", awsIssuer, p1.GetID(), p1.Accounts[0], "instance-id", + p1, "instance-id", awsIssuer, p1.GetID(), p1.Accounts[0], "instance-id", "127.0.0.1", "", time.Now(), key) assert.FatalError(t, err) failExp, err := generateAWSToken( - "instance-id", awsIssuer, p1.GetID(), p1.Accounts[0], "instance-id", + p1, "instance-id", awsIssuer, p1.GetID(), p1.Accounts[0], "instance-id", "127.0.0.1", "us-west-1", time.Now().Add(-360*time.Second), key) assert.FatalError(t, err) failNbf, err := generateAWSToken( - "instance-id", awsIssuer, p1.GetID(), p1.Accounts[0], "instance-id", + p1, "instance-id", awsIssuer, p1.GetID(), p1.Accounts[0], "instance-id", "127.0.0.1", "us-west-1", time.Now().Add(360*time.Second), key) assert.FatalError(t, err) failKey, err := generateAWSToken( - "instance-id", awsIssuer, p1.GetID(), p1.Accounts[0], "instance-id", + p1, "instance-id", awsIssuer, p1.GetID(), p1.Accounts[0], "instance-id", "127.0.0.1", "us-west-1", time.Now(), badKey) assert.FatalError(t, err) failInstanceAge, err := generateAWSToken( - "instance-id", awsIssuer, p2.GetID(), p2.Accounts[0], "instance-id", + p2, "instance-id", awsIssuer, p2.GetID(), p2.Accounts[0], "instance-id", "127.0.0.1", "us-west-1", time.Now().Add(-1*time.Minute), key) assert.FatalError(t, err) diff --git a/authority/provisioner/options_test.go b/authority/provisioner/options_test.go index ced9626f..8f411aca 100644 --- a/authority/provisioner/options_test.go +++ b/authority/provisioner/options_test.go @@ -127,7 +127,7 @@ func TestTemplateOptions(t *testing.T) { }`)}, false}, {"okCustomTemplate", args{&Options{X509: &X509Options{Template: x509util.DefaultIIDLeafTemplate}}, data}, x509util.Options{ CertBuffer: bytes.NewBufferString(`{ - "subject": {"commonName":"foo"}, + "subject": {"commonName": "foo"}, "sans": [{"type":"dns","value":"foo.com"}], "keyUsage": ["digitalSignature"], "extKeyUsage": ["serverAuth", "clientAuth"] @@ -189,7 +189,7 @@ func TestCustomTemplateOptions(t *testing.T) { }`)}, false}, {"okIID", args{nil, data, x509util.DefaultIIDLeafTemplate, SignOptions{}}, x509util.Options{ CertBuffer: bytes.NewBufferString(`{ - "subject": {"commonName":"foo"}, + "subject": {"commonName": "foo"}, "sans": [{"type":"dns","value":"foo.com"}], "keyUsage": ["digitalSignature"], "extKeyUsage": ["serverAuth", "clientAuth"] diff --git a/authority/provisioner/utils_test.go b/authority/provisioner/utils_test.go index d4f5cc80..fb0eb9e7 100644 --- a/authority/provisioner/utils_test.go +++ b/authority/provisioner/utils_test.go @@ -13,6 +13,7 @@ import ( "io/ioutil" "net/http" "net/http/httptest" + "strings" "time" "github.com/pkg/errors" @@ -910,7 +911,7 @@ func generateGCPToken(sub, iss, aud, instanceID, instanceName, projectID, zone s return jose.Signed(sig).Claims(claims).CompactSerialize() } -func generateAWSToken(sub, iss, aud, accountID, instanceID, privateIP, region string, iat time.Time, key crypto.Signer) (string, error) { +func generateAWSToken(p *AWS, sub, iss, aud, accountID, instanceID, privateIP, region string, iat time.Time, key crypto.Signer) (string, error) { doc, err := json.MarshalIndent(awsInstanceIdentityDocument{ AccountID: accountID, Architecture: "x86_64", @@ -946,8 +947,12 @@ func generateAWSToken(sub, iss, aud, accountID, instanceID, privateIP, region st return "", err } + unique := fmt.Sprintf("%s.%s", p.GetID(), instanceID) + sum = sha256.Sum256([]byte(unique)) + claims := awsPayload{ Claims: jose.Claims{ + ID: strings.ToLower(hex.EncodeToString(sum[:])), Subject: sub, Issuer: iss, IssuedAt: jose.NewNumericDate(iat), From 86c947babcc7e2d8a608d82e5db5849543438ff4 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 17 Dec 2020 14:17:08 -0800 Subject: [PATCH 3/4] Upgrade crypto and fix test. --- authority/provisioner/ssh_options_test.go | 4 ++-- go.mod | 2 +- go.sum | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/authority/provisioner/ssh_options_test.go b/authority/provisioner/ssh_options_test.go index cbd67d85..6f45d5ac 100644 --- a/authority/provisioner/ssh_options_test.go +++ b/authority/provisioner/ssh_options_test.go @@ -39,8 +39,8 @@ func TestCustomSSHTemplateOptions(t *testing.T) { }, false}, {"okNoData", args{nil, nil, sshutil.DefaultTemplate, SignSSHOptions{}}, sshutil.Options{ CertBuffer: bytes.NewBufferString(`{ - "type": "", - "keyId": "", + "type": null, + "keyId": null, "principals": null, "extensions": null, "criticalOptions": null diff --git a/go.mod b/go.mod index 5887ac49..7c8f6582 100644 --- a/go.mod +++ b/go.mod @@ -20,7 +20,7 @@ require ( github.com/smallstep/nosql v0.3.0 github.com/urfave/cli v1.22.4 go.step.sm/cli-utils v0.1.0 - go.step.sm/crypto v0.7.2 + go.step.sm/crypto v0.7.3 golang.org/x/crypto v0.0.0-20201016220609-9e8e0b390897 golang.org/x/net v0.0.0-20201021035429-f5854403a974 google.golang.org/api v0.33.0 diff --git a/go.sum b/go.sum index 8d68502a..fdafcd66 100644 --- a/go.sum +++ b/go.sum @@ -293,8 +293,8 @@ go.step.sm/cli-utils v0.1.0 h1:uuQ73MuAh5P5Eg+3zfqwrtlTLx5DWSfGqGCrSSjYqdk= go.step.sm/cli-utils v0.1.0/go.mod h1:+t4qCp5NO+080DdGkJxEh3xL5S4TcYC2JTPLMM72b6Y= go.step.sm/crypto v0.6.1 h1:nJoRFGrGNf/mKVVMdWnfLbBfIFt/z4NdJlSL5nipQMQ= go.step.sm/crypto v0.6.1/go.mod h1:AKS4yMZVZD4EGjpSkY4eibuMenrvKCscb+BpWMet8c0= -go.step.sm/crypto v0.7.2 h1:4EFVTDmdLchlw1Z8AgMl+UiExb1nsocbm/7aArEG/SA= -go.step.sm/crypto v0.7.2/go.mod h1:AKS4yMZVZD4EGjpSkY4eibuMenrvKCscb+BpWMet8c0= +go.step.sm/crypto v0.7.3 h1:uWkT0vsaZVixgn5x6Ojqittry9PiyVn2ihEYG/qOxV8= +go.step.sm/crypto v0.7.3/go.mod h1:AKS4yMZVZD4EGjpSkY4eibuMenrvKCscb+BpWMet8c0= golang.org/x/crypto v0.0.0-20181203042331-505ab145d0a9/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20190510104115-cbcb75029529/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= From 5017b7d21f49e282cfd0545299a445f5f049634c Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 17 Dec 2020 14:52:34 -0800 Subject: [PATCH 4/4] Recalculate token id instead of validating it. --- authority/provisioner/aws.go | 13 +++++-------- authority/provisioner/aws_test.go | 16 ---------------- 2 files changed, 5 insertions(+), 24 deletions(-) diff --git a/authority/provisioner/aws.go b/authority/provisioner/aws.go index 2d07d2da..75115154 100644 --- a/authority/provisioner/aws.go +++ b/authority/provisioner/aws.go @@ -284,7 +284,11 @@ func (p *AWS) GetTokenID(token string) (string, error) { sum := sha256.Sum256([]byte(token)) return strings.ToLower(hex.EncodeToString(sum[:])), nil } - return payload.ID, nil + + // Use provisioner + instance-id as the identifier. + unique := fmt.Sprintf("%s.%s", p.GetID(), payload.document.InstanceID) + sum := sha256.Sum256([]byte(unique)) + return strings.ToLower(hex.EncodeToString(sum[:])), nil } // GetName returns the name of the provisioner. @@ -631,13 +635,6 @@ func (p *AWS) authorizeToken(token string) (*awsPayload, error) { return nil, errs.Unauthorized("aws.authorizeToken; aws identity document region cannot be empty") } - // Recalculate and validate payload.ID - unique := fmt.Sprintf("%s.%s", p.GetID(), doc.InstanceID) - sum := sha256.Sum256([]byte(unique)) - if payload.ID != strings.ToLower(hex.EncodeToString(sum[:])) { - return nil, errs.Unauthorized("aws.authorizeToken; invalid token id") - } - // According to "rfc7519 JSON Web Token" acceptable skew should be no // more than a few minutes. now := time.Now().UTC() diff --git a/authority/provisioner/aws_test.go b/authority/provisioner/aws_test.go index 3feeed1f..dadf1f17 100644 --- a/authority/provisioner/aws_test.go +++ b/authority/provisioner/aws_test.go @@ -470,22 +470,6 @@ func TestAWS_authorizeToken(t *testing.T) { err: errors.New("aws.authorizeToken; aws identity document pendingTime is too old"), } }, - "fail/payloadId": func(t *testing.T) test { - p, err := generateAWS() - assert.FatalError(t, err) - p2, err := generateAWS() - assert.FatalError(t, err) - tok, err := generateAWSToken( - p2, "instance-id", awsIssuer, p.GetID(), p.Accounts[0], "instance-id", - "127.0.0.1", "us-west-1", time.Now(), key) - assert.FatalError(t, err) - return test{ - p: p, - token: tok, - code: http.StatusUnauthorized, - err: errors.New("aws.authorizeToken; invalid token id"), - } - }, "ok": func(t *testing.T) test { p, err := generateAWS() assert.FatalError(t, err)