Skip to content

Commit 0c8376a

Browse files
marainomaraino
committed
Fix existing unit tests.
1 parent 497158d commit 0c8376a

15 files changed

+94
-58
lines changed

‎acme/common.go

+8
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ type MockProvisioner struct {
2828
MgetName func() string
2929
MauthorizeSign func(ctx context.Context, ott string) ([]provisioner.SignOption, error)
3030
MdefaultTLSCertDuration func() time.Duration
31+
MgetOptions func() *provisioner.ProvisionerOptions
3132
}
3233

3334
// GetName mock
@@ -54,6 +55,13 @@ func (m *MockProvisioner) DefaultTLSCertDuration() time.Duration {
5455
return m.Mret1.(time.Duration)
5556
}
5657

58+
func (m *MockProvisioner) GetOptions() *provisioner.ProvisionerOptions {
59+
if m.MgetOptions != nil {
60+
return m.MgetOptions()
61+
}
62+
return m.Mret1.(*provisioner.ProvisionerOptions)
63+
}
64+
5765
// ContextKey is the key type for storing and searching for ACME request
5866
// essentials in the context of a request.
5967
type ContextKey string

‎acme/order_test.go

+10-10
Original file line numberDiff line numberDiff line change
@@ -1066,13 +1066,13 @@ func TestOrderFinalize(t *testing.T) {
10661066
Subject: pkix.Name{
10671067
CommonName: "",
10681068
},
1069-
DNSNames: []string{"acme.example.com", "step.example.com"},
1069+
// DNSNames: []string{"acme.example.com", "step.example.com"},
10701070
IPAddresses: []net.IP{net.ParseIP("1.1.1.1")},
10711071
}
10721072
return test{
10731073
o: o,
10741074
csr: csr,
1075-
err: BadCSRErr(errors.Errorf("CSR contains IP Address SANs, but should only contain DNS Names")),
1075+
err: BadCSRErr(errors.Errorf("CSR names do not match identifiers exactly")),
10761076
}
10771077
},
10781078
"fail/ready/no-emailAddresses": func(t *testing.T) test {
@@ -1084,13 +1084,13 @@ func TestOrderFinalize(t *testing.T) {
10841084
Subject: pkix.Name{
10851085
CommonName: "",
10861086
},
1087-
DNSNames: []string{"acme.example.com", "step.example.com"},
1087+
// DNSNames: []string{"acme.example.com", "step.example.com"},
10881088
EmailAddresses: []string{"max@smallstep.com", "mariano@smallstep.com"},
10891089
}
10901090
return test{
10911091
o: o,
10921092
csr: csr,
1093-
err: BadCSRErr(errors.Errorf("CSR contains Email Address SANs, but should only contain DNS Names")),
1093+
err: BadCSRErr(errors.Errorf("CSR names do not match identifiers exactly")),
10941094
}
10951095
},
10961096
"fail/ready/no-URIs": func(t *testing.T) test {
@@ -1104,13 +1104,13 @@ func TestOrderFinalize(t *testing.T) {
11041104
Subject: pkix.Name{
11051105
CommonName: "",
11061106
},
1107-
DNSNames: []string{"acme.example.com", "step.example.com"},
1108-
URIs: []*url.URL{u},
1107+
// DNSNames: []string{"acme.example.com", "step.example.com"},
1108+
URIs: []*url.URL{u},
11091109
}
11101110
return test{
11111111
o: o,
11121112
csr: csr,
1113-
err: BadCSRErr(errors.Errorf("CSR contains URI SANs, but should only contain DNS Names")),
1113+
err: BadCSRErr(errors.Errorf("CSR names do not match identifiers exactly")),
11141114
}
11151115
},
11161116
"fail/ready/provisioner-auth-sign-error": func(t *testing.T) test {
@@ -1263,7 +1263,7 @@ func TestOrderFinalize(t *testing.T) {
12631263
csr: csr,
12641264
sa: &mockSignAuth{
12651265
sign: func(csr *x509.CertificateRequest, pops provisioner.Options, signOps ...provisioner.SignOption) ([]*x509.Certificate, error) {
1266-
assert.Equals(t, len(signOps), 5)
1266+
assert.Equals(t, len(signOps), 6)
12671267
return []*x509.Certificate{crt, inter}, nil
12681268
},
12691269
},
@@ -1312,7 +1312,7 @@ func TestOrderFinalize(t *testing.T) {
13121312
csr: csr,
13131313
sa: &mockSignAuth{
13141314
sign: func(csr *x509.CertificateRequest, pops provisioner.Options, signOps ...provisioner.SignOption) ([]*x509.Certificate, error) {
1315-
assert.Equals(t, len(signOps), 5)
1315+
assert.Equals(t, len(signOps), 6)
13161316
return []*x509.Certificate{crt, inter}, nil
13171317
},
13181318
},
@@ -1359,7 +1359,7 @@ func TestOrderFinalize(t *testing.T) {
13591359
csr: csr,
13601360
sa: &mockSignAuth{
13611361
sign: func(csr *x509.CertificateRequest, pops provisioner.Options, signOps ...provisioner.SignOption) ([]*x509.Certificate, error) {
1362-
assert.Equals(t, len(signOps), 5)
1362+
assert.Equals(t, len(signOps), 6)
13631363
return []*x509.Certificate{crt, inter}, nil
13641364
},
13651365
},

‎authority/provisioner/aws_test.go

+6-5
Original file line numberDiff line numberDiff line change
@@ -540,11 +540,11 @@ func TestAWS_AuthorizeSign(t *testing.T) {
540540
code int
541541
wantErr bool
542542
}{
543-
{"ok", p1, args{t1, "foo.local"}, 5, http.StatusOK, false},
544-
{"ok", p2, args{t2, "instance-id"}, 9, http.StatusOK, false},
545-
{"ok", p2, args{t2Hostname, "ip-127-0-0-1.us-west-1.compute.internal"}, 9, http.StatusOK, false},
546-
{"ok", p2, args{t2PrivateIP, "127.0.0.1"}, 9, http.StatusOK, false},
547-
{"ok", p1, args{t4, "instance-id"}, 5, http.StatusOK, false},
543+
{"ok", p1, args{t1, "foo.local"}, 6, http.StatusOK, false},
544+
{"ok", p2, args{t2, "instance-id"}, 10, http.StatusOK, false},
545+
{"ok", p2, args{t2Hostname, "ip-127-0-0-1.us-west-1.compute.internal"}, 10, http.StatusOK, false},
546+
{"ok", p2, args{t2PrivateIP, "127.0.0.1"}, 10, http.StatusOK, false},
547+
{"ok", p1, args{t4, "instance-id"}, 6, http.StatusOK, false},
548548
{"fail account", p3, args{token: t3}, 0, http.StatusUnauthorized, true},
549549
{"fail token", p1, args{token: "token"}, 0, http.StatusUnauthorized, true},
550550
{"fail subject", p1, args{token: failSubject}, 0, http.StatusUnauthorized, true},
@@ -574,6 +574,7 @@ func TestAWS_AuthorizeSign(t *testing.T) {
574574
assert.Len(t, tt.wantLen, got)
575575
for _, o := range got {
576576
switch v := o.(type) {
577+
case certificateOptionsFunc:
577578
case *provisionerExtensionOption:
578579
assert.Equals(t, v.Type, int(TypeAWS))
579580
assert.Equals(t, v.Name, tt.aws.GetName())

‎authority/provisioner/azure_test.go

+4-3
Original file line numberDiff line numberDiff line change
@@ -431,9 +431,9 @@ func TestAzure_AuthorizeSign(t *testing.T) {
431431
code int
432432
wantErr bool
433433
}{
434-
{"ok", p1, args{t1}, 4, http.StatusOK, false},
435-
{"ok", p2, args{t2}, 9, http.StatusOK, false},
436-
{"ok", p1, args{t11}, 4, http.StatusOK, false},
434+
{"ok", p1, args{t1}, 5, http.StatusOK, false},
435+
{"ok", p2, args{t2}, 10, http.StatusOK, false},
436+
{"ok", p1, args{t11}, 5, http.StatusOK, false},
437437
{"fail tenant", p3, args{t3}, 0, http.StatusUnauthorized, true},
438438
{"fail resource group", p4, args{t4}, 0, http.StatusUnauthorized, true},
439439
{"fail token", p1, args{"token"}, 0, http.StatusUnauthorized, true},
@@ -458,6 +458,7 @@ func TestAzure_AuthorizeSign(t *testing.T) {
458458
assert.Len(t, tt.wantLen, got)
459459
for _, o := range got {
460460
switch v := o.(type) {
461+
case certificateOptionsFunc:
461462
case *provisionerExtensionOption:
462463
assert.Equals(t, v.Type, int(TypeAzure))
463464
assert.Equals(t, v.Name, tt.azure.GetName())

‎authority/provisioner/gcp_test.go

+4-3
Original file line numberDiff line numberDiff line change
@@ -515,9 +515,9 @@ func TestGCP_AuthorizeSign(t *testing.T) {
515515
code int
516516
wantErr bool
517517
}{
518-
{"ok", p1, args{t1}, 4, http.StatusOK, false},
519-
{"ok", p2, args{t2}, 9, http.StatusOK, false},
520-
{"ok", p3, args{t3}, 4, http.StatusOK, false},
518+
{"ok", p1, args{t1}, 5, http.StatusOK, false},
519+
{"ok", p2, args{t2}, 10, http.StatusOK, false},
520+
{"ok", p3, args{t3}, 5, http.StatusOK, false},
521521
{"fail token", p1, args{"token"}, 0, http.StatusUnauthorized, true},
522522
{"fail key", p1, args{failKey}, 0, http.StatusUnauthorized, true},
523523
{"fail iss", p1, args{failIss}, 0, http.StatusUnauthorized, true},
@@ -547,6 +547,7 @@ func TestGCP_AuthorizeSign(t *testing.T) {
547547
assert.Len(t, tt.wantLen, got)
548548
for _, o := range got {
549549
switch v := o.(type) {
550+
case certificateOptionsFunc:
550551
case *provisionerExtensionOption:
551552
assert.Equals(t, v.Type, int(TypeGCP))
552553
assert.Equals(t, v.Name, tt.gcp.GetName())

‎authority/provisioner/jwk_test.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -295,9 +295,10 @@ func TestJWK_AuthorizeSign(t *testing.T) {
295295
}
296296
} else {
297297
if assert.NotNil(t, got) {
298-
assert.Len(t, 6, got)
298+
assert.Len(t, 7, got)
299299
for _, o := range got {
300300
switch v := o.(type) {
301+
case certificateOptionsFunc:
301302
case *provisionerExtensionOption:
302303
assert.Equals(t, v.Type, int(TypeJWK))
303304
assert.Equals(t, v.Name, tt.prov.GetName())

‎authority/provisioner/k8sSA_test.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,7 @@ func TestK8sSA_AuthorizeSign(t *testing.T) {
274274
tot := 0
275275
for _, o := range opts {
276276
switch v := o.(type) {
277+
case certificateOptionsFunc:
277278
case *provisionerExtensionOption:
278279
assert.Equals(t, v.Type, int(TypeK8sSA))
279280
assert.Equals(t, v.Name, tc.p.GetName())
@@ -290,7 +291,7 @@ func TestK8sSA_AuthorizeSign(t *testing.T) {
290291
}
291292
tot++
292293
}
293-
assert.Equals(t, tot, 4)
294+
assert.Equals(t, tot, 5)
294295
}
295296
}
296297
}

‎authority/provisioner/oidc.go

+4
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,10 @@ func (o *OIDC) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption
363363
if err != nil {
364364
return nil, errs.Wrap(http.StatusInternalServerError, err, "oidc.AuthorizeSSHSign")
365365
}
366+
// Enforce an email claim
367+
if claims.Email == "" {
368+
return nil, errs.Unauthorized("oidc.AuthorizeSSHSign: failed to validate oidc token payload: email not found")
369+
}
366370
signOptions := []SignOption{
367371
// set the key id to the token email
368372
sshCertKeyIDModifier(claims.Email),

‎authority/provisioner/oidc_test.go

+11-9
Original file line numberDiff line numberDiff line change
@@ -179,12 +179,12 @@ func TestOIDC_authorizeToken(t *testing.T) {
179179
assert.FatalError(t, err)
180180
t4, err := generateToken("subject", issuer, p3.ClientID, "foo@smallstep.com", []string{}, time.Now(), &keys.Keys[2])
181181
assert.FatalError(t, err)
182-
// Invalid email
183-
failEmail, err := generateToken("subject", issuer, p3.ClientID, "", []string{}, time.Now(), &keys.Keys[2])
182+
t5, err := generateToken("subject", issuer, p3.ClientID, "", []string{}, time.Now(), &keys.Keys[2])
184183
assert.FatalError(t, err)
184+
185+
// Invalid email
185186
failDomain, err := generateToken("subject", issuer, p3.ClientID, "name@example.com", []string{}, time.Now(), &keys.Keys[2])
186187
assert.FatalError(t, err)
187-
188188
// Invalid tokens
189189
parts := strings.Split(t1, ".")
190190
key, err := generateJSONWebKey()
@@ -226,7 +226,7 @@ func TestOIDC_authorizeToken(t *testing.T) {
226226
{"ok tenantid", p2, args{t2}, http.StatusOK, tenantIssuer, false},
227227
{"ok admin", p3, args{t3}, http.StatusOK, issuer, false},
228228
{"ok domain", p3, args{t4}, http.StatusOK, issuer, false},
229-
{"fail-email", p3, args{failEmail}, http.StatusUnauthorized, "", true},
229+
{"ok no email", p3, args{t5}, http.StatusOK, issuer, false},
230230
{"fail-domain", p3, args{failDomain}, http.StatusUnauthorized, "", true},
231231
{"fail-key", p1, args{failKey}, http.StatusUnauthorized, "", true},
232232
{"fail-token", p1, args{failTok}, http.StatusUnauthorized, "", true},
@@ -290,8 +290,8 @@ func TestOIDC_AuthorizeSign(t *testing.T) {
290290
// Admin email not in domains
291291
okAdmin, err := generateToken("subject", "the-issuer", p3.ClientID, "root@example.com", []string{"test.smallstep.com"}, time.Now(), &keys.Keys[0])
292292
assert.FatalError(t, err)
293-
// Invalid email
294-
failEmail, err := generateToken("subject", "the-issuer", p3.ClientID, "", []string{}, time.Now(), &keys.Keys[0])
293+
// No email
294+
noEmail, err := generateToken("subject", "the-issuer", p3.ClientID, "", []string{}, time.Now(), &keys.Keys[0])
295295
assert.FatalError(t, err)
296296

297297
type args struct {
@@ -306,7 +306,8 @@ func TestOIDC_AuthorizeSign(t *testing.T) {
306306
}{
307307
{"ok1", p1, args{t1}, http.StatusOK, false},
308308
{"admin", p3, args{okAdmin}, http.StatusOK, false},
309-
{"fail-email", p3, args{failEmail}, http.StatusUnauthorized, true},
309+
{"no-email", p3, args{noEmail}, http.StatusOK, false},
310+
{"bad-token", p3, args{"foobar"}, http.StatusUnauthorized, true},
310311
}
311312
for _, tt := range tests {
312313
t.Run(tt.name, func(t *testing.T) {
@@ -323,12 +324,13 @@ func TestOIDC_AuthorizeSign(t *testing.T) {
323324
} else {
324325
if assert.NotNil(t, got) {
325326
if tt.name == "admin" {
326-
assert.Len(t, 4, got)
327+
assert.Len(t, 5, got)
327328
} else {
328329
assert.Len(t, 5, got)
329330
}
330331
for _, o := range got {
331332
switch v := o.(type) {
333+
case certificateOptionsFunc:
332334
case *provisionerExtensionOption:
333335
assert.Equals(t, v.Type, int(TypeOIDC))
334336
assert.Equals(t, v.Name, tt.prov.GetName())
@@ -514,7 +516,7 @@ func TestOIDC_AuthorizeSSHSign(t *testing.T) {
514516
// Admin email not in domains
515517
okAdmin, err := generateToken("subject", "the-issuer", p3.ClientID, "root@example.com", []string{}, time.Now(), &keys.Keys[0])
516518
assert.FatalError(t, err)
517-
// Invalid email
519+
// Empty email
518520
failEmail, err := generateToken("subject", "the-issuer", p3.ClientID, "", []string{}, time.Now(), &keys.Keys[0])
519521
assert.FatalError(t, err)
520522

‎authority/provisioner/options.go

+1
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ func (fn certificateOptionsFunc) Options(so Options) []x509util.Option {
2222

2323
// ProvisionerOptions are a collection of custom options that can be added to
2424
// each provisioner.
25+
// nolint:golint
2526
type ProvisionerOptions struct {
2627
// Template contains a X.509 certificate template. It can be a JSON template
2728
// escaped in a string or it can be also encoded in base64.

‎authority/provisioner/sign_options.go

+8-1
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@ import (
1717
"golang.org/x/crypto/ed25519"
1818
)
1919

20+
// DefaultCertValidity is the default validity for a certificate if none is specified.
21+
const DefaultCertValidity = 24 * time.Hour
22+
2023
// Options contains the options that can be passed to the Sign method. Backdate
2124
// is automatically filled and can only be configured in the CA.
2225
type Options struct {
@@ -277,7 +280,11 @@ func (v profileDefaultDuration) Modify(cert *x509.Certificate, so Options) error
277280
}
278281
notAfter := so.NotAfter.RelativeTime(notBefore)
279282
if notAfter.IsZero() {
280-
notAfter = notBefore.Add(time.Duration(v))
283+
if v != 0 {
284+
notAfter = notBefore.Add(time.Duration(v))
285+
} else {
286+
notAfter = notBefore.Add(DefaultCertValidity)
287+
}
281288
}
282289

283290
cert.NotBefore = notBefore.Add(backdate)

‎authority/provisioner/sign_options_test.go

+9-17
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import (
1313
"github.com/pkg/errors"
1414
"github.com/smallstep/assert"
1515
"github.com/smallstep/cli/crypto/pemutil"
16-
"github.com/smallstep/cli/crypto/x509util"
1716
)
1817

1918
func Test_emailOnlyIdentity_Valid(t *testing.T) {
@@ -562,15 +561,13 @@ func Test_forceCN_Option(t *testing.T) {
562561
for name, run := range tests {
563562
t.Run(name, func(t *testing.T) {
564563
tt := run()
565-
prof := &x509util.Leaf{}
566-
prof.SetSubject(tt.cert)
567-
if err := tt.fcn.Option(tt.so)(prof); err != nil {
564+
if err := tt.fcn.Modify(tt.cert, tt.so); err != nil {
568565
if assert.NotNil(t, tt.err) {
569566
assert.HasPrefix(t, err.Error(), tt.err.Error())
570567
}
571568
} else {
572569
if assert.Nil(t, tt.err) {
573-
tt.valid(prof.Subject())
570+
tt.valid(tt.cert)
574571
}
575572
}
576573
})
@@ -661,10 +658,9 @@ func Test_profileDefaultDuration_Option(t *testing.T) {
661658
for name, run := range tests {
662659
t.Run(name, func(t *testing.T) {
663660
tt := run()
664-
prof := &x509util.Leaf{}
665-
prof.SetSubject(tt.cert)
666-
assert.FatalError(t, tt.pdd.Option(tt.so)(prof), "unexpected error")
667-
tt.valid(prof.Subject())
661+
assert.FatalError(t, tt.pdd.Modify(tt.cert, tt.so), "unexpected error")
662+
time.Sleep(1 * time.Nanosecond)
663+
tt.valid(tt.cert)
668664
})
669665
}
670666
}
@@ -702,10 +698,8 @@ func Test_newProvisionerExtension_Option(t *testing.T) {
702698
for name, run := range tests {
703699
t.Run(name, func(t *testing.T) {
704700
tt := run()
705-
prof := &x509util.Leaf{}
706-
prof.SetSubject(tt.cert)
707-
assert.FatalError(t, newProvisionerExtensionOption(TypeJWK, "foo", "bar", "baz", "zap").Option(Options{})(prof))
708-
tt.valid(prof.Subject())
701+
assert.FatalError(t, newProvisionerExtensionOption(TypeJWK, "foo", "bar", "baz", "zap").Modify(tt.cert, Options{}))
702+
tt.valid(tt.cert)
709703
})
710704
}
711705
}
@@ -803,15 +797,13 @@ func Test_profileLimitDuration_Option(t *testing.T) {
803797
for name, run := range tests {
804798
t.Run(name, func(t *testing.T) {
805799
tt := run()
806-
prof := &x509util.Leaf{}
807-
prof.SetSubject(tt.cert)
808-
if err := tt.pld.Option(tt.so)(prof); err != nil {
800+
if err := tt.pld.Modify(tt.cert, tt.so); err != nil {
809801
if assert.NotNil(t, tt.err) {
810802
assert.HasPrefix(t, err.Error(), tt.err.Error())
811803
}
812804
} else {
813805
if assert.Nil(t, tt.err) {
814-
tt.valid(prof.Subject())
806+
tt.valid(tt.cert)
815807
}
816808
}
817809
})

‎authority/provisioner/x5c_test.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -463,9 +463,10 @@ func TestX5C_AuthorizeSign(t *testing.T) {
463463
} else {
464464
if assert.Nil(t, tc.err) {
465465
if assert.NotNil(t, opts) {
466-
assert.Equals(t, len(opts), 6)
466+
assert.Equals(t, len(opts), 7)
467467
for _, o := range opts {
468468
switch v := o.(type) {
469+
case certificateOptionsFunc:
469470
case *provisionerExtensionOption:
470471
assert.Equals(t, v.Type, int(TypeX5C))
471472
assert.Equals(t, v.Name, tc.p.GetName())

0 commit comments

Comments
 (0)