diff --git a/authority/provisioner/aws_test.go b/authority/provisioner/aws_test.go index 4c4dbae4..94365982 100644 --- a/authority/provisioner/aws_test.go +++ b/authority/provisioner/aws_test.go @@ -708,6 +708,7 @@ func TestAWS_AuthorizeSSHSign(t *testing.T) { } else if assert.NotNil(t, got) { cert, err := signSSHCertificate(tt.args.key, tt.args.sshOpts, got, signer.Key.(crypto.Signer)) if (err != nil) != tt.wantSignErr { + t.Errorf("SignSSH error = %v, wantSignErr %v", err, tt.wantSignErr) } else { if tt.wantSignErr { diff --git a/authority/provisioner/jwk_test.go b/authority/provisioner/jwk_test.go index 2048bc64..61f66953 100644 --- a/authority/provisioner/jwk_test.go +++ b/authority/provisioner/jwk_test.go @@ -512,10 +512,6 @@ func TestJWK_AuthorizeSign_SSHOptions(t *testing.T) { }{ {"ok-user", p1, args{sub, iss, aud, iat, &SignSSHOptions{CertType: "user", Principals: []string{"name"}}, &SignSSHOptions{}, jwk}, expectedUserOptions, false, false}, {"ok-host", p1, args{sub, iss, aud, iat, &SignSSHOptions{CertType: "host", Principals: []string{"smallstep.com"}}, &SignSSHOptions{}, jwk}, expectedHostOptions, false, false}, - {"ok-user-opts", p1, args{sub, iss, aud, iat, &SignSSHOptions{}, &SignSSHOptions{CertType: "user", Principals: []string{"name"}}, jwk}, expectedUserOptions, false, false}, - {"ok-host-opts", p1, args{sub, iss, aud, iat, &SignSSHOptions{}, &SignSSHOptions{CertType: "host", Principals: []string{"smallstep.com"}}, jwk}, expectedHostOptions, false, false}, - {"ok-user-mixed", p1, args{sub, iss, aud, iat, &SignSSHOptions{CertType: "user"}, &SignSSHOptions{Principals: []string{"name"}}, jwk}, expectedUserOptions, false, false}, - {"ok-host-mixed", p1, args{sub, iss, aud, iat, &SignSSHOptions{Principals: []string{"smallstep.com"}}, &SignSSHOptions{CertType: "host"}, jwk}, expectedHostOptions, false, false}, {"ok-user-validAfter", p1, args{sub, iss, aud, iat, &SignSSHOptions{ CertType: "user", Principals: []string{"name"}, }, &SignSSHOptions{ diff --git a/authority/provisioner/k8sSA_test.go b/authority/provisioner/k8sSA_test.go index 16a57aaf..9c731ae4 100644 --- a/authority/provisioner/k8sSA_test.go +++ b/authority/provisioner/k8sSA_test.go @@ -361,9 +361,9 @@ func TestK8sSA_AuthorizeSSHSign(t *testing.T) { tot := 0 for _, o := range opts { switch v := o.(type) { - case sshCertDefaultsModifier: - assert.Equals(t, v.CertType, SSHUserCert) - case *sshDefaultExtensionModifier: + case sshCertificateOptionsFunc: + case *sshCertOptionsRequireValidator: + assert.Equals(t, v, &sshCertOptionsRequireValidator{CertType: true, KeyID: true, Principals: true}) case *sshCertValidityValidator: assert.Equals(t, v.Claimer, tc.p.claimer) case *sshDefaultPublicKeyValidator: diff --git a/authority/provisioner/oidc_test.go b/authority/provisioner/oidc_test.go index 55093a91..cb830246 100644 --- a/authority/provisioner/oidc_test.go +++ b/authority/provisioner/oidc_test.go @@ -565,33 +565,34 @@ func TestOIDC_AuthorizeSSHSign(t *testing.T) { {"ok-rsa2048", p1, args{t1, SignSSHOptions{}, rsa2048.Public()}, expectedUserOptions, http.StatusOK, false, false}, {"ok-user", p1, args{t1, SignSSHOptions{CertType: "user"}, pub}, expectedUserOptions, http.StatusOK, false, false}, {"ok-principals", p1, args{t1, SignSSHOptions{Principals: []string{"name"}}, pub}, - &SignSSHOptions{CertType: "user", Principals: []string{"name"}, + &SignSSHOptions{CertType: "user", Principals: []string{"name", "name@smallstep.com"}, ValidAfter: NewTimeDuration(tm), ValidBefore: NewTimeDuration(tm.Add(userDuration))}, http.StatusOK, false, false}, {"ok-principals-getIdentity", p4, args{okGetIdentityToken, SignSSHOptions{Principals: []string{"mariano"}}, pub}, - &SignSSHOptions{CertType: "user", Principals: []string{"mariano"}, + &SignSSHOptions{CertType: "user", Principals: []string{"max", "mariano"}, ValidAfter: NewTimeDuration(tm), ValidBefore: NewTimeDuration(tm.Add(userDuration))}, http.StatusOK, false, false}, {"ok-emptyPrincipals-getIdentity", p4, args{okGetIdentityToken, SignSSHOptions{}, pub}, &SignSSHOptions{CertType: "user", Principals: []string{"max", "mariano"}, ValidAfter: NewTimeDuration(tm), ValidBefore: NewTimeDuration(tm.Add(userDuration))}, http.StatusOK, false, false}, {"ok-options", p1, args{t1, SignSSHOptions{CertType: "user", Principals: []string{"name"}}, pub}, - &SignSSHOptions{CertType: "user", Principals: []string{"name"}, + &SignSSHOptions{CertType: "user", Principals: []string{"name", "name@smallstep.com"}, ValidAfter: NewTimeDuration(tm), ValidBefore: NewTimeDuration(tm.Add(userDuration))}, http.StatusOK, false, false}, - {"admin", p3, args{okAdmin, SignSSHOptions{}, pub}, expectedAdminOptions, http.StatusOK, false, false}, - {"admin-user", p3, args{okAdmin, SignSSHOptions{CertType: "user"}, pub}, expectedAdminOptions, http.StatusOK, false, false}, - {"admin-principals", p3, args{okAdmin, SignSSHOptions{Principals: []string{"root"}}, pub}, - &SignSSHOptions{CertType: "user", Principals: []string{"root"}, - ValidAfter: NewTimeDuration(tm), ValidBefore: NewTimeDuration(tm.Add(userDuration))}, http.StatusOK, false, false}, - {"admin-options", p3, args{okAdmin, SignSSHOptions{CertType: "user", Principals: []string{"name"}}, pub}, - &SignSSHOptions{CertType: "user", Principals: []string{"name"}, - ValidAfter: NewTimeDuration(tm), ValidBefore: NewTimeDuration(tm.Add(userDuration))}, http.StatusOK, false, false}, - {"admin-host", p3, args{okAdmin, SignSSHOptions{CertType: "host", Principals: []string{"smallstep.com"}}, pub}, + {"admin-user", p3, args{okAdmin, SignSSHOptions{CertType: "user", KeyID: "root@example.com", Principals: []string{"root", "root@example.com"}}, pub}, + expectedAdminOptions, http.StatusOK, false, false}, + {"admin-host", p3, args{okAdmin, SignSSHOptions{CertType: "host", KeyID: "smallstep.com", Principals: []string{"smallstep.com"}}, pub}, expectedHostOptions, http.StatusOK, false, false}, + {"admin-options", p3, args{okAdmin, SignSSHOptions{CertType: "user", KeyID: "name", Principals: []string{"name"}}, pub}, + &SignSSHOptions{CertType: "user", Principals: []string{"name"}, + ValidAfter: NewTimeDuration(tm), ValidBefore: NewTimeDuration(tm.Add(userDuration))}, http.StatusOK, false, false}, {"fail-rsa1024", p1, args{t1, SignSSHOptions{}, rsa1024.Public()}, expectedUserOptions, http.StatusOK, false, true}, {"fail-user-host", p1, args{t1, SignSSHOptions{CertType: "host"}, pub}, nil, http.StatusOK, false, true}, {"fail-user-principals", p1, args{t1, SignSSHOptions{Principals: []string{"root"}}, pub}, nil, http.StatusOK, false, true}, {"fail-email", p3, args{failEmail, SignSSHOptions{}, pub}, nil, http.StatusUnauthorized, true, false}, {"fail-getIdentity", p5, args{failGetIdentityToken, SignSSHOptions{}, pub}, nil, http.StatusInternalServerError, true, false}, {"fail-sshCA-disabled", p6, args{"foo", SignSSHOptions{}, pub}, nil, http.StatusUnauthorized, true, false}, + // Missing parametrs + {"fail-admin-type", p3, args{okAdmin, SignSSHOptions{KeyID: "root@example.com", Principals: []string{"root@example.com"}}, pub}, nil, http.StatusUnauthorized, false, true}, + {"fail-admin-key-id", p3, args{okAdmin, SignSSHOptions{CertType: "user", Principals: []string{"root@example.com"}}, pub}, nil, http.StatusUnauthorized, false, true}, + {"fail-admin-principals", p3, args{okAdmin, SignSSHOptions{CertType: "user", KeyID: "root@example.com"}, pub}, nil, http.StatusUnauthorized, false, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/authority/provisioner/sign_ssh_options.go b/authority/provisioner/sign_ssh_options.go index 279c2ae1..e9d5c727 100644 --- a/authority/provisioner/sign_ssh_options.go +++ b/authority/provisioner/sign_ssh_options.go @@ -47,7 +47,7 @@ type SignSSHOptions struct { Principals []string `json:"principals"` ValidAfter TimeDuration `json:"validAfter,omitempty"` ValidBefore TimeDuration `json:"validBefore,omitempty"` - TemplateData json.RawMessage `json:"templateData"` + TemplateData json.RawMessage `json:"templateData,omitempty"` Backdate time.Duration `json:"-"` } diff --git a/authority/provisioner/sign_ssh_options_test.go b/authority/provisioner/sign_ssh_options_test.go index 3c0d9bb5..9ab72a51 100644 --- a/authority/provisioner/sign_ssh_options_test.go +++ b/authority/provisioner/sign_ssh_options_test.go @@ -525,11 +525,6 @@ func Test_sshCertDefaultValidator_Valid(t *testing.T) { &ssh.Certificate{Nonce: []byte("foo"), Key: sshPub, Serial: 1, CertType: 1}, errors.New("ssh certificate key id cannot be empty"), }, - { - "fail/empty-valid-principals", - &ssh.Certificate{Nonce: []byte("foo"), Key: sshPub, Serial: 1, CertType: 1, KeyId: "foo"}, - errors.New("ssh certificate valid principals cannot be empty"), - }, { "fail/zero-validAfter", &ssh.Certificate{ @@ -571,20 +566,6 @@ func Test_sshCertDefaultValidator_Valid(t *testing.T) { }, errors.New("ssh certificate validBefore cannot be before validAfter"), }, - { - "fail/empty-extensions", - &ssh.Certificate{ - Nonce: []byte("foo"), - Key: sshPub, - Serial: 1, - CertType: 1, - KeyId: "foo", - ValidPrincipals: []string{"foo"}, - ValidAfter: uint64(time.Now().Unix()), - ValidBefore: uint64(time.Now().Add(10 * time.Minute).Unix()), - }, - errors.New("ssh certificate extensions cannot be empty"), - }, { "fail/nil-signature-key", &ssh.Certificate{ @@ -655,6 +636,38 @@ func Test_sshCertDefaultValidator_Valid(t *testing.T) { }, nil, }, + { + "ok/emptyPrincipals", + &ssh.Certificate{ + Nonce: []byte("foo"), + Key: sshPub, + Serial: 1, + CertType: 1, + KeyId: "foo", + ValidPrincipals: []string{}, + ValidAfter: uint64(time.Now().Unix()), + ValidBefore: uint64(time.Now().Add(10 * time.Minute).Unix()), + SignatureKey: sshPub, + Signature: &ssh.Signature{}, + }, + nil, + }, + { + "ok/empty-extensions", + &ssh.Certificate{ + Nonce: []byte("foo"), + Key: sshPub, + Serial: 1, + CertType: 1, + KeyId: "foo", + ValidPrincipals: []string{}, + ValidAfter: uint64(time.Now().Unix()), + ValidBefore: uint64(time.Now().Add(10 * time.Minute).Unix()), + SignatureKey: sshPub, + Signature: &ssh.Signature{}, + }, + nil, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/authority/provisioner/ssh_test.go b/authority/provisioner/ssh_test.go index 5511e6cd..3c8f7118 100644 --- a/authority/provisioner/ssh_test.go +++ b/authority/provisioner/ssh_test.go @@ -2,11 +2,13 @@ package provisioner import ( "crypto" - "crypto/rand" "fmt" + "net/http" "reflect" "time" + "github.com/smallstep/certificates/errs" + "github.com/smallstep/certificates/sshutil" "golang.org/x/crypto/ssh" ) @@ -46,10 +48,14 @@ func signSSHCertificate(key crypto.PublicKey, opts SignSSHOptions, signOpts []Si } var mods []SSHCertModifier + var certOptions []sshutil.Option var validators []SSHCertValidator for _, op := range signOpts { switch o := op.(type) { + // add options to NewCertificate + case SSHCertificateOptions: + certOptions = append(certOptions, o.Options(opts)...) // modify the ssh.Certificate case SSHCertModifier: mods = append(mods, o) @@ -66,22 +72,39 @@ func signSSHCertificate(key crypto.PublicKey, opts SignSSHOptions, signOpts []Si } } - // Build base certificate with the key and some random values - cert := &ssh.Certificate{ - Nonce: []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 0}, - Key: pub, - Serial: 1234567890, + // Simulated certificate request with request options. + cr := sshutil.CertificateRequest{ + Type: opts.CertType, + KeyID: opts.KeyID, + Principals: opts.Principals, + Key: pub, } - // Use opts to modify the certificate - if err := opts.Modify(cert, opts); err != nil { - return nil, err + // Create certificate from template. + certificate, err := sshutil.NewCertificate(cr, certOptions...) + if err != nil { + if _, ok := err.(*sshutil.TemplateError); ok { + return nil, errs.NewErr(http.StatusBadRequest, err, + errs.WithMessage(err.Error()), + errs.WithKeyVal("signOptions", signOpts), + ) + } + return nil, errs.Wrap(http.StatusInternalServerError, err, "authority.SignSSH") } - // Use provisioner modifiers + // Get actual *ssh.Certificate and continue with provisioner modifiers. + cert := certificate.GetCertificate() + + // Use SignSSHOptions to modify the certificate validity. It will be later + // checked or set if not defined. + if err := opts.ModifyValidity(cert); err != nil { + return nil, errs.Wrap(http.StatusBadRequest, err, "authority.SignSSH") + } + + // Use provisioner modifiers. for _, m := range mods { if err := m.Modify(cert, opts); err != nil { - return nil, err + return nil, errs.Wrap(http.StatusForbidden, err, "authority.SignSSH") } } @@ -98,23 +121,17 @@ func signSSHCertificate(key crypto.PublicKey, opts SignSSHOptions, signOpts []Si if err != nil { return nil, err } - cert.SignatureKey = signer.PublicKey() - // Get bytes for signing trailing the signature length. - data := cert.Marshal() - data = data[:len(data)-4] - - // Sign the certificate - sig, err := signer.Sign(rand.Reader, data) + // Sign certificate. + cert, err = sshutil.CreateCertificate(cert, signer) if err != nil { - return nil, err + return nil, errs.Wrap(http.StatusInternalServerError, err, "authority.SignSSH: error signing certificate") } - cert.Signature = sig - // User provisioners validators + // User provisioners validators. for _, v := range validators { if err := v.Valid(cert, opts); err != nil { - return nil, err + return nil, errs.Wrap(http.StatusForbidden, err, "authority.SignSSH") } } diff --git a/authority/provisioner/x5c_test.go b/authority/provisioner/x5c_test.go index 73a3ba9f..fac8e60e 100644 --- a/authority/provisioner/x5c_test.go +++ b/authority/provisioner/x5c_test.go @@ -698,6 +698,7 @@ func TestX5C_AuthorizeSSHSign(t *testing.T) { }, Step: &stepPayload{SSH: &SignSSHOptions{ CertType: SSHHostCert, + KeyID: "foo", Principals: []string{"max", "mariano", "alan"}, ValidAfter: TimeDuration{d: 5 * time.Minute}, ValidBefore: TimeDuration{d: 10 * time.Minute}, @@ -753,19 +754,19 @@ func TestX5C_AuthorizeSSHSign(t *testing.T) { if assert.Nil(t, tc.err) { if assert.NotNil(t, opts) { tot := 0 + firstValidator := true nw := now() for _, o := range opts { switch v := o.(type) { case sshCertOptionsValidator: tc.claims.Step.SSH.ValidAfter.t = time.Time{} tc.claims.Step.SSH.ValidBefore.t = time.Time{} - assert.Equals(t, SignSSHOptions(v), *tc.claims.Step.SSH) - case sshCertKeyIDModifier: - assert.Equals(t, string(v), "foo") - case sshCertTypeModifier: - assert.Equals(t, string(v), tc.claims.Step.SSH.CertType) - case sshCertPrincipalsModifier: - assert.Equals(t, []string(v), tc.claims.Step.SSH.Principals) + if firstValidator { + assert.Equals(t, SignSSHOptions(v), *tc.claims.Step.SSH) + } else { + assert.Equals(t, SignSSHOptions(v), SignSSHOptions{KeyID: tc.claims.Subject}) + } + firstValidator = false case sshCertValidAfterModifier: assert.Equals(t, int64(v), tc.claims.Step.SSH.ValidAfter.RelativeTime(nw).Unix()) case sshCertValidBeforeModifier: @@ -777,19 +778,16 @@ func TestX5C_AuthorizeSSHSign(t *testing.T) { assert.Equals(t, v.NotAfter, x5cCerts[0].NotAfter) case *sshCertValidityValidator: assert.Equals(t, v.Claimer, tc.p.claimer) - case *sshDefaultExtensionModifier, *sshDefaultPublicKeyValidator, - *sshCertDefaultValidator: - case sshCertKeyIDValidator: - assert.Equals(t, string(v), "foo") + case *sshDefaultPublicKeyValidator, *sshCertDefaultValidator, sshCertificateOptionsFunc: default: assert.FatalError(t, errors.Errorf("unexpected sign option of type %T", v)) } tot++ } if len(tc.claims.Step.SSH.CertType) > 0 { - assert.Equals(t, tot, 13) - } else { assert.Equals(t, tot, 9) + } else { + assert.Equals(t, tot, 7) } } }