Skip to content

Commit 992ff69

Browse files
marainomaraino
authored
Merge pull request #2491 from smallstep/mariano/update
Improve validation in authorization path
2 parents 8e76e29 + 9d79c59 commit 992ff69

31 files changed

+464
-108
lines changed

‎acme/challenge.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -792,6 +792,9 @@ type attestationObject struct {
792792

793793
// TODO(bweeks): move attestation verification to a shared package.
794794
func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSONWebKey, payload []byte) error {
795+
// Update challenge with the payload
796+
ch.Payload = payload
797+
795798
// Load authorization to store the key fingerprint.
796799
az, err := db.GetAuthorization(ctx, ch.AuthorizationID)
797800
if err != nil {
@@ -946,7 +949,6 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
946949
ch.Status = StatusValid
947950
ch.Error = nil
948951
ch.ValidatedAt = clock.Now().Format(time.RFC3339)
949-
ch.Payload = payload
950952
ch.PayloadFormat = format
951953

952954
// Store the fingerprint in the authorization.

‎acme/challenge_test.go‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -878,7 +878,7 @@ MCowBQYDK2VwAyEA5c+4NKZSNQcR1T8qN6SjwgdPZQ0Ge12Ylx/YeGAJ35k=
878878
assert.Equal(t, StatusInvalid, updch.Status)
879879
assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
880880
assert.Equal(t, "12345678", updch.Value)
881-
assert.Nil(t, updch.Payload)
881+
assert.Equal(t, payload, updch.Payload)
882882
assert.Empty(t, updch.PayloadFormat)
883883

884884
err := NewError(ErrorRejectedIdentifierType, "payload contained error: an error")
@@ -4077,7 +4077,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
40774077
assert.Equal(t, StatusInvalid, updch.Status)
40784078
assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
40794079
assert.Equal(t, "12345678", updch.Value)
4080-
assert.Nil(t, updch.Payload)
4080+
assert.Equal(t, errorPayload, updch.Payload)
40814081
assert.Empty(t, updch.PayloadFormat)
40824082

40834083
err := NewError(ErrorRejectedIdentifierType, "payload contained error: an error")
@@ -4117,7 +4117,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
41174117
assert.Equal(t, StatusInvalid, updch.Status)
41184118
assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
41194119
assert.Equal(t, "12345678", updch.Value)
4120-
assert.Nil(t, updch.Payload)
4120+
assert.Equal(t, errorBase64Payload, updch.Payload)
41214121
assert.Empty(t, updch.PayloadFormat)
41224122

41234123
err := NewDetailedError(ErrorBadAttestationStatementType, "failed base64 decoding attObj %q", "?!")
@@ -4157,7 +4157,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
41574157
assert.Equal(t, StatusInvalid, updch.Status)
41584158
assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
41594159
assert.Equal(t, "12345678", updch.Value)
4160-
assert.Nil(t, updch.Payload)
4160+
assert.Equal(t, emptyPayload, updch.Payload)
41614161
assert.Empty(t, updch.PayloadFormat)
41624162

41634163
err := NewDetailedError(ErrorBadAttestationStatementType, "attObj must not be empty")
@@ -4197,7 +4197,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
41974197
assert.Equal(t, StatusInvalid, updch.Status)
41984198
assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
41994199
assert.Equal(t, "12345678", updch.Value)
4200-
assert.Nil(t, updch.Payload)
4200+
assert.Equal(t, emptyObjectPayload, updch.Payload)
42014201
assert.Empty(t, updch.PayloadFormat)
42024202

42034203
err := NewDetailedError(ErrorBadAttestationStatementType, "attObj must not be empty")
@@ -4237,7 +4237,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
42374237
assert.Equal(t, StatusInvalid, updch.Status)
42384238
assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
42394239
assert.Equal(t, "12345678", updch.Value)
4240-
assert.Nil(t, updch.Payload)
4240+
assert.Equal(t, errorNonWellformedCBORPayload, updch.Payload)
42414241
assert.Empty(t, updch.PayloadFormat)
42424242

42434243
err := NewDetailedError(ErrorBadAttestationStatementType, "attObj is not well formed CBOR: unexpected EOF")
@@ -4279,7 +4279,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
42794279
assert.Equal(t, StatusInvalid, updch.Status)
42804280
assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
42814281
assert.Equal(t, "12345678", updch.Value)
4282-
assert.Nil(t, updch.Payload)
4282+
assert.Equal(t, errorUnsupportedFormat, updch.Payload)
42834283
assert.Empty(t, updch.PayloadFormat)
42844284

42854285
err := NewDetailedError(ErrorBadAttestationStatementType, "unsupported attestation object format %q", "unsupported-format")
@@ -4326,7 +4326,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
43264326
assert.Equal(t, StatusInvalid, updch.Status)
43274327
assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
43284328
assert.Equal(t, "12345678", updch.Value)
4329-
assert.Nil(t, updch.Payload)
4329+
assert.Equal(t, payload, updch.Payload)
43304330
assert.Empty(t, updch.PayloadFormat)
43314331

43324332
err := NewError(ErrorBadAttestationStatementType, "attestation format %q is not enabled", "step")
@@ -4383,7 +4383,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
43834383
assert.Equal(t, StatusInvalid, updch.Status)
43844384
assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
43854385
assert.Equal(t, "12345678", updch.Value)
4386-
assert.Nil(t, updch.Payload)
4386+
assert.Equal(t, payload, updch.Payload)
43874387
assert.Empty(t, updch.PayloadFormat)
43884388

43894389
err := NewDetailedError(ErrorBadAttestationStatementType, "x5c not present")
@@ -4432,7 +4432,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
44324432
assert.Equal(t, StatusInvalid, updch.Status)
44334433
assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
44344434
assert.Equal(t, "serial-number", updch.Value)
4435-
assert.Nil(t, updch.Payload)
4435+
assert.Equal(t, payload, updch.Payload)
44364436
assert.Empty(t, updch.PayloadFormat)
44374437

44384438
err := NewDetailedError(ErrorBadAttestationStatementType, "challenge token does not match")
@@ -4480,7 +4480,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
44804480
assert.Equal(t, StatusInvalid, updch.Status)
44814481
assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
44824482
assert.Equal(t, "non-matching-value", updch.Value)
4483-
assert.Nil(t, updch.Payload)
4483+
assert.Equal(t, payload, updch.Payload)
44844484
assert.Empty(t, updch.PayloadFormat)
44854485

44864486
subproblem := NewSubproblemWithIdentifier(
@@ -4560,7 +4560,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
45604560
assert.Equal(t, StatusInvalid, updch.Status)
45614561
assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
45624562
assert.Equal(t, "12345678", updch.Value)
4563-
assert.Nil(t, updch.Payload)
4563+
assert.Equal(t, payload, updch.Payload)
45644564
assert.Empty(t, updch.PayloadFormat)
45654565

45664566
err := NewDetailedError(ErrorBadAttestationStatementType, "x5c not present")
@@ -4616,7 +4616,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
46164616
assert.Equal(t, StatusInvalid, updch.Status)
46174617
assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
46184618
assert.Equal(t, "12345678", updch.Value)
4619-
assert.Nil(t, updch.Payload)
4619+
assert.Equal(t, payload, updch.Payload)
46204620
assert.Empty(t, updch.PayloadFormat)
46214621

46224622
err := NewDetailedError(ErrorBadAttestationStatementType, "permanent identifier does not match").
@@ -4713,7 +4713,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
47134713
assert.Equal(t, StatusInvalid, updch.Status)
47144714
assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
47154715
assert.Equal(t, "12345678", updch.Value)
4716-
assert.Nil(t, updch.Payload)
4716+
assert.Equal(t, payload, updch.Payload)
47174717
assert.Empty(t, updch.PayloadFormat)
47184718

47194719
err := NewDetailedError(ErrorBadAttestationStatementType, `unsupported attestation object format "bogus-format"`)

‎api/renew.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77

88
"github.com/smallstep/certificates/api/render"
99
"github.com/smallstep/certificates/authority"
10+
"github.com/smallstep/certificates/authority/provisioner"
1011
"github.com/smallstep/certificates/errs"
1112
)
1213

@@ -60,7 +61,7 @@ func getPeerCertificate(r *http.Request) (*x509.Certificate, string, error) {
6061
}
6162
if s := r.Header.Get(authorizationHeader); s != "" {
6263
if parts := strings.SplitN(s, bearerScheme+" ", 2); len(parts) == 2 {
63-
ctx := r.Context()
64+
ctx := provisioner.NewContextWithMethod(r.Context(), provisioner.RenewMethod)
6465
peer, err := mustAuthority(ctx).AuthorizeRenewToken(ctx, parts[1])
6566
return peer, parts[1], err
6667
}

‎api/revoke.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,8 +94,9 @@ func Revoke(w http.ResponseWriter, r *http.Request) {
9494
return
9595
}
9696
opts.Crt = r.TLS.PeerCertificates[0]
97-
if opts.Crt.SerialNumber.String() != opts.Serial {
98-
render.Error(w, r, errs.BadRequest("serial number in client certificate different than body"))
97+
if serialNumber := opts.Crt.SerialNumber.String(); opts.Serial != serialNumber {
98+
render.Error(w, r, errs.Forbidden(
99+
"request serial number %q and certificate serial number %q do not match", opts.Serial, serialNumber))
99100
return
100101
}
101102
// TODO: should probably be checking if the certificate was revoked here.

‎api/sshRevoke.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,14 +70,13 @@ func SSHRevoke(w http.ResponseWriter, r *http.Request) {
7070
ctx := provisioner.NewContextWithMethod(r.Context(), provisioner.SSHRevokeMethod)
7171
a := mustAuthority(ctx)
7272

73-
// A token indicates that we are using the api via a provisioner token,
74-
// otherwise it is assumed that the certificate is revoking itself over mTLS.
7573
logOtt(w, body.OTT)
7674

7775
if _, err := a.Authorize(ctx, body.OTT); err != nil {
7876
render.Error(w, r, errs.UnauthorizedErr(err))
7977
return
8078
}
79+
8180
opts.OTT = body.OTT
8281

8382
if err := a.Revoke(ctx, opts); err != nil {

‎authority/authority_test.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,10 @@ func testAuthority(t *testing.T, opts ...Option) *Authority {
8080
EnableSSHCA: &enableSSHCA,
8181
},
8282
},
83+
&provisioner.ACME{
84+
Name: "acme",
85+
Type: "ACME",
86+
},
8387
&provisioner.JWK{
8488
Name: "uninitialized",
8589
Type: "JWK",

‎authority/authorize.go‎

Lines changed: 31 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ func (a *Authority) authorizeToken(ctx context.Context, token string) (provision
9393
// Store the token to protect against reuse unless it's skipped.
9494
// If we cannot get a token id from the provisioner, just hash the token.
9595
if !SkipTokenReuseFromContext(ctx) {
96-
if err := a.UseToken(token, p); err != nil {
96+
if err := a.UseToken(ctx, token, p); err != nil {
9797
return nil, err
9898
}
9999
}
@@ -138,7 +138,7 @@ func (a *Authority) AuthorizeAdminToken(r *http.Request, token string) (*linkedc
138138
}
139139

140140
// Check that the token has not been used.
141-
if err := a.UseToken(token, prov); err != nil {
141+
if err := a.UseToken(r.Context(), token, prov); err != nil {
142142
return nil, admin.WrapError(admin.ErrorUnauthorizedType, err, "adminHandler.authorizeToken; error with reuse token")
143143
}
144144

@@ -193,22 +193,35 @@ func (a *Authority) AuthorizeAdminToken(r *http.Request, token string) (*linkedc
193193

194194
// UseToken stores the token to protect against reuse.
195195
//
196-
// This method currently ignores any error coming from the GetTokenID, but it
197-
// should specifically ignore the error provisioner.ErrAllowTokenReuse.
198-
func (a *Authority) UseToken(token string, prov provisioner.Interface) error {
199-
if reuseKey, err := prov.GetTokenID(token); err == nil {
200-
if reuseKey == "" {
201-
sum := sha256.Sum256([]byte(token))
202-
reuseKey = strings.ToLower(hex.EncodeToString(sum[:]))
203-
}
204-
ok, err := a.db.UseToken(reuseKey, token)
205-
if err != nil {
206-
return errs.Wrap(http.StatusInternalServerError, err, "failed when attempting to store token")
207-
}
208-
if !ok {
209-
return errs.Unauthorized("token already used")
196+
// This method currently ignores most errors coming from the GetTokenID because
197+
// the token is already validated. But it should specifically ignore the errors
198+
// provisioner.ErrAllowTokenReuse, provisioner.ErrNotImplemented, and
199+
// provisioner.ErrTokenFlowNotSupported unless this latter one used in a renewal
200+
// flow without mTLS.
201+
func (a *Authority) UseToken(ctx context.Context, token string, prov provisioner.Interface) error {
202+
reuseKey, err := prov.GetTokenID(token)
203+
if err != nil {
204+
// Fail on ErrTokenFlowNotSupported but allow x5cInsecure renew token
205+
if errors.Is(err, provisioner.ErrTokenFlowNotSupported) && provisioner.RenewMethod != provisioner.MethodFromContext(ctx) {
206+
return errs.BadRequest("token flow is not supported")
210207
}
208+
209+
return nil
210+
}
211+
212+
if reuseKey == "" {
213+
sum := sha256.Sum256([]byte(token))
214+
reuseKey = strings.ToLower(hex.EncodeToString(sum[:]))
211215
}
216+
217+
ok, err := a.db.UseToken(reuseKey, token)
218+
if err != nil {
219+
return errs.Wrap(http.StatusInternalServerError, err, "failed when attempting to store token")
220+
}
221+
if !ok {
222+
return errs.Unauthorized("token already used")
223+
}
224+
212225
return nil
213226
}
214227

@@ -398,7 +411,7 @@ func (a *Authority) authorizeSSHRevoke(ctx context.Context, token string) error
398411

399412
// AuthorizeRenewToken validates the renew token and returns the leaf
400413
// certificate in the x5cInsecure header.
401-
func (a *Authority) AuthorizeRenewToken(_ context.Context, ott string) (*x509.Certificate, error) {
414+
func (a *Authority) AuthorizeRenewToken(ctx context.Context, ott string) (*x509.Certificate, error) {
402415
var claims jose.Claims
403416
jwt, chain, err := jose.ParseX5cInsecure(ott, a.rootX509Certs)
404417
if err != nil {
@@ -413,7 +426,7 @@ func (a *Authority) AuthorizeRenewToken(_ context.Context, ott string) (*x509.Ce
413426
if err != nil {
414427
return nil, errs.Unauthorized("error validating renew token: cannot get provisioner from certificate")
415428
}
416-
if err := a.UseToken(ott, p); err != nil {
429+
if err := a.UseToken(ctx, ott, p); err != nil {
417430
return nil, err
418431
}
419432

‎authority/authorize_test.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,25 @@ func TestAuthority_authorizeToken(t *testing.T) {
193193
code: http.StatusUnauthorized,
194194
}
195195
},
196+
"fail/token-flow-not-supported": func(t *testing.T) *authorizeTest {
197+
cl := jose.Claims{
198+
Subject: "test.smallstep.com",
199+
Issuer: validIssuer,
200+
NotBefore: jose.NewNumericDate(now),
201+
Expiry: jose.NewNumericDate(now.Add(time.Minute)),
202+
IssuedAt: jose.NewNumericDate(now),
203+
Audience: []string{"acme/acme"},
204+
ID: "45",
205+
}
206+
raw, err := jose.Signed(sig).Claims(cl).CompactSerialize()
207+
assert.FatalError(t, err)
208+
return &authorizeTest{
209+
auth: a,
210+
token: raw,
211+
err: errors.New("token flow is not supported"),
212+
code: http.StatusBadRequest,
213+
}
214+
},
196215
"ok/simpledb": func(t *testing.T) *authorizeTest {
197216
cl := jose.Claims{
198217
Subject: "test.smallstep.com",

‎authority/provisioner/acme.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,9 +138,10 @@ func (p *ACME) GetIDForToken() string {
138138
return "acme/" + p.Name
139139
}
140140

141-
// GetTokenID returns the identifier of the token.
141+
// GetTokenID returns the identifier of the token. This provisioner will always
142+
// return [ErrTokenFlowNotSupported].
142143
func (p *ACME) GetTokenID(string) (string, error) {
143-
return "", errors.New("acme provisioner does not implement GetTokenID")
144+
return "", ErrTokenFlowNotSupported
144145
}
145146

146147
// GetName returns the name of the provisioner.

‎authority/provisioner/acme_test.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,9 @@ func TestACME_Getters(t *testing.T) {
8282
t.Errorf("ACME.GetEncryptedKey() = (%v, %v, %v), want (%v, %v, %v)",
8383
kid, key, ok, "", "", false)
8484
}
85+
tokenID, err := p.GetTokenID("token")
86+
assert.Empty(t, tokenID)
87+
assert.Equal(t, ErrTokenFlowNotSupported, err)
8588
}
8689

8790
func TestACME_Init(t *testing.T) {

0 commit comments

Comments
 (0)