From 6e1f8dd7aba9f76ebcc925606f5961fb63f5fca2 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Tue, 26 Apr 2022 13:12:16 +0200 Subject: [PATCH] Refactor policy engines into container --- acme/api/order.go | 10 +- authority/authority.go | 4 +- authority/policy.go | 48 +-- authority/policy/engine.go | 114 ++++++ authority/policy_test.go | 428 +++++++++++----------- authority/provisioner/acme.go | 4 +- authority/provisioner/sign_options.go | 3 +- authority/provisioner/sign_ssh_options.go | 6 +- authority/ssh.go | 73 +--- authority/ssh_test.go | 103 +++--- authority/tls.go | 34 +- authority/tls_test.go | 26 +- policy/engine.go | 41 +-- policy/engine_test.go | 52 +-- policy/ssh.go | 2 +- policy/x509.go | 10 +- 16 files changed, 485 insertions(+), 473 deletions(-) create mode 100644 authority/policy/engine.go diff --git a/acme/api/order.go b/acme/api/order.go index 5bf35a58..c37285d2 100644 --- a/acme/api/order.go +++ b/acme/api/order.go @@ -5,7 +5,6 @@ import ( "crypto/x509" "encoding/base64" "encoding/json" - "fmt" "net" "net/http" "strings" @@ -199,14 +198,7 @@ func isIdentifierAllowed(acmePolicy policy.X509Policy, identifier acme.Identifie if acmePolicy == nil { return nil } - allowed, err := acmePolicy.AreSANsAllowed([]string{identifier.Value}) - if err != nil { - return err - } - if !allowed { - return fmt.Errorf("acme identifier '%s' not allowed", identifier.Value) - } - return nil + return acmePolicy.AreSANsAllowed([]string{identifier.Value}) } func newACMEPolicyEngine(eak *acme.ExternalAccountKey) (policy.X509Policy, error) { diff --git a/authority/authority.go b/authority/authority.go index 84864159..8a0013c0 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -81,9 +81,7 @@ type Authority struct { authorizeSSHRenewFunc provisioner.AuthorizeSSHRenewFunc // Policy engines - x509Policy policy.X509Policy - sshUserPolicy policy.UserPolicy - sshHostPolicy policy.HostPolicy + policyEngine *policy.Engine adminMutex sync.RWMutex } diff --git a/authority/policy.go b/authority/policy.go index 08a26f16..47104e0e 100644 --- a/authority/policy.go +++ b/authority/policy.go @@ -227,50 +227,19 @@ func (a *Authority) reloadPolicyEngines(ctx context.Context) error { policyOptions = a.config.AuthorityConfig.Policy } - // if no new or updated policy option is set, clear policy engines that (may have) - // been configured before and return early - if policyOptions == nil { - a.x509Policy = nil - a.sshHostPolicy = nil - a.sshUserPolicy = nil - return nil - } - - var ( - x509Policy authPolicy.X509Policy - sshHostPolicy authPolicy.HostPolicy - sshUserPolicy authPolicy.UserPolicy - ) - - // initialize the x509 allow/deny policy engine - if x509Policy, err = authPolicy.NewX509PolicyEngine(policyOptions.GetX509Options()); err != nil { - return err - } - - // initialize the SSH allow/deny policy engine for host certificates - if sshHostPolicy, err = authPolicy.NewSSHHostPolicyEngine(policyOptions.GetSSHOptions()); err != nil { - return err - } - - // initialize the SSH allow/deny policy engine for user certificates - if sshUserPolicy, err = authPolicy.NewSSHUserPolicyEngine(policyOptions.GetSSHOptions()); err != nil { + engine, err := authPolicy.New(policyOptions) + if err != nil { return err } - // set all policy engines; all or nothing - a.x509Policy = x509Policy - a.sshHostPolicy = sshHostPolicy - a.sshUserPolicy = sshUserPolicy + // only update the policy engine when no error was returned + a.policyEngine = engine return nil } func isAllowed(engine authPolicy.X509Policy, sans []string) error { - var ( - allowed bool - err error - ) - if allowed, err = engine.AreSANsAllowed(sans); err != nil { + if err := engine.AreSANsAllowed(sans); err != nil { var policyErr *policy.NamePolicyError isNamePolicyError := errors.As(err, &policyErr) if isNamePolicyError && policyErr.Reason == policy.NotAllowed { @@ -285,13 +254,6 @@ func isAllowed(engine authPolicy.X509Policy, sans []string) error { } } - if !allowed { - return &PolicyError{ - Typ: AdminLockOut, - Err: fmt.Errorf("the provided policy would lock out %s from the CA. Please update your policy to include %s as an allowed name", sans, sans), - } - } - return nil } diff --git a/authority/policy/engine.go b/authority/policy/engine.go new file mode 100644 index 00000000..4b21f66b --- /dev/null +++ b/authority/policy/engine.go @@ -0,0 +1,114 @@ +package policy + +import ( + "crypto/x509" + "errors" + "fmt" + + "golang.org/x/crypto/ssh" +) + +// Engine is a container for multiple policies. +type Engine struct { + x509Policy X509Policy + sshUserPolicy UserPolicy + sshHostPolicy HostPolicy +} + +// New returns a new Engine using Options. +func New(options *Options) (*Engine, error) { + + // if no options provided, return early + if options == nil { + return nil, nil + } + + var ( + x509Policy X509Policy + sshHostPolicy HostPolicy + sshUserPolicy UserPolicy + err error + ) + + // initialize the x509 allow/deny policy engine + if x509Policy, err = NewX509PolicyEngine(options.GetX509Options()); err != nil { + return nil, err + } + + // initialize the SSH allow/deny policy engine for host certificates + if sshHostPolicy, err = NewSSHHostPolicyEngine(options.GetSSHOptions()); err != nil { + return nil, err + } + + // initialize the SSH allow/deny policy engine for user certificates + if sshUserPolicy, err = NewSSHUserPolicyEngine(options.GetSSHOptions()); err != nil { + return nil, err + } + + return &Engine{ + x509Policy: x509Policy, + sshHostPolicy: sshHostPolicy, + sshUserPolicy: sshUserPolicy, + }, nil +} + +// IsX509CertificateAllowed evaluates an X.509 certificate against +// the X.509 policy (if available) and returns an error if one of the +// names in the certificate is not allowed. +func (e *Engine) IsX509CertificateAllowed(cert *x509.Certificate) error { + + // return early if there's no policy to evaluate + if e == nil || e.x509Policy == nil { + return nil + } + + // return result of X.509 policy evaluation + return e.x509Policy.IsX509CertificateAllowed(cert) +} + +// AreSANsAllowed evaluates the slice of SANs against the X.509 policy +// (if available) and returns an error if one of the SANs is not allowed. +func (e *Engine) AreSANsAllowed(sans []string) error { + + // return early if there's no policy to evaluate + if e == nil || e.x509Policy == nil { + return nil + } + + // return result of X.509 policy evaluation + return e.x509Policy.AreSANsAllowed(sans) +} + +// IsSSHCertificateAllowed evaluates an SSH certificate against the +// user or host policy (if configured) and returns an error if one of the +// principals in the certificate is not allowed. +func (e *Engine) IsSSHCertificateAllowed(cert *ssh.Certificate) error { + + // return early if there's no policy to evaluate + if e == nil || (e.sshHostPolicy == nil && e.sshUserPolicy == nil) { + return nil + } + + switch cert.CertType { + case ssh.HostCert: + // when no host policy engine is configured, but a user policy engine is + // configured, the host certificate is denied. + if e.sshHostPolicy == nil && e.sshUserPolicy != nil { + return errors.New("authority not allowed to sign ssh host certificates") + } + + // return result of SSH host policy evaluation + return e.sshHostPolicy.IsSSHCertificateAllowed(cert) + case ssh.UserCert: + // when no user policy engine is configured, but a host policy engine is + // configured, the user certificate is denied. + if e.sshUserPolicy == nil && e.sshHostPolicy != nil { + return errors.New("authority not allowed to sign ssh user certificates") + } + + // return result of SSH user policy evaluation + return e.sshUserPolicy.IsSSHCertificateAllowed(cert) + default: + return fmt.Errorf("unexpected ssh certificate type %q", cert.CertType) + } +} diff --git a/authority/policy_test.go b/authority/policy_test.go index f444a7f0..40af879a 100644 --- a/authority/policy_test.go +++ b/authority/policy_test.go @@ -330,105 +330,193 @@ func Test_policyToCertificates(t *testing.T) { } } +func mustPolicyEngine(t *testing.T, options *policy.Options) *policy.Engine { + engine, err := policy.New(options) + if err != nil { + t.Fatal(err) + } + return engine +} + func TestAuthority_reloadPolicyEngines(t *testing.T) { - existingX509PolicyEngine, err := policy.NewX509PolicyEngine(&policy.X509PolicyOptions{ - AllowedNames: &policy.X509NameOptions{ - DNSDomains: []string{"*.hosts.example.com"}, + existingPolicyEngine, err := policy.New(&policy.Options{ + X509: &policy.X509PolicyOptions{ + AllowedNames: &policy.X509NameOptions{ + DNSDomains: []string{"*.hosts.example.com"}, + }, + }, + SSH: &policy.SSHPolicyOptions{ + Host: &policy.SSHHostCertificateOptions{ + AllowedNames: &policy.SSHNameOptions{ + DNSDomains: []string{"*.hosts.example.com"}, + }, + }, + User: &policy.SSHUserCertificateOptions{ + AllowedNames: &policy.SSHNameOptions{ + EmailAddresses: []string{"@mails.example.com"}, + }, + }, }, }) assert.NoError(t, err) - existingSSHHostPolicyEngine, err := policy.NewSSHHostPolicyEngine(&policy.SSHPolicyOptions{ - Host: &policy.SSHHostCertificateOptions{ - AllowedNames: &policy.SSHNameOptions{ - DNSDomains: []string{"*.hosts.example.com"}, + newX509Options := &policy.Options{ + X509: &policy.X509PolicyOptions{ + AllowedNames: &policy.X509NameOptions{ + DNSDomains: []string{"*.local"}, }, + DeniedNames: &policy.X509NameOptions{ + DNSDomains: []string{"badhost.local"}, + }, + AllowWildcardLiteral: true, + DisableCommonNameVerification: false, }, - }) - assert.NoError(t, err) + } - existingSSHUserPolicyEngine, err := policy.NewSSHUserPolicyEngine(&policy.SSHPolicyOptions{ - User: &policy.SSHUserCertificateOptions{ - AllowedNames: &policy.SSHNameOptions{ - EmailAddresses: []string{"@mails.example.com"}, + newSSHHostOptions := &policy.Options{ + SSH: &policy.SSHPolicyOptions{ + Host: &policy.SSHHostCertificateOptions{ + AllowedNames: &policy.SSHNameOptions{ + DNSDomains: []string{"*.local"}, + }, + DeniedNames: &policy.SSHNameOptions{ + DNSDomains: []string{"badhost.local"}, + }, }, }, - }) - assert.NoError(t, err) + } - newX509PolicyEngine, err := policy.NewX509PolicyEngine(&policy.X509PolicyOptions{ - AllowedNames: &policy.X509NameOptions{ - DNSDomains: []string{"*.local"}, + newSSHUserOptions := &policy.Options{ + SSH: &policy.SSHPolicyOptions{ + User: &policy.SSHUserCertificateOptions{ + AllowedNames: &policy.SSHNameOptions{ + Principals: []string{"*"}, + }, + DeniedNames: &policy.SSHNameOptions{ + Principals: []string{"root"}, + }, + }, }, - DeniedNames: &policy.X509NameOptions{ - DNSDomains: []string{"badhost.local"}, + } + + newSSHOptions := &policy.Options{ + SSH: &policy.SSHPolicyOptions{ + Host: &policy.SSHHostCertificateOptions{ + AllowedNames: &policy.SSHNameOptions{ + DNSDomains: []string{"*.local"}, + }, + DeniedNames: &policy.SSHNameOptions{ + DNSDomains: []string{"badhost.local"}, + }, + }, + User: &policy.SSHUserCertificateOptions{ + AllowedNames: &policy.SSHNameOptions{ + Principals: []string{"*"}, + }, + DeniedNames: &policy.SSHNameOptions{ + Principals: []string{"root"}, + }, + }, }, - AllowWildcardLiteral: true, - DisableCommonNameVerification: false, - }) - assert.NoError(t, err) + } - newSSHHostPolicyEngine, err := policy.NewSSHHostPolicyEngine(&policy.SSHPolicyOptions{ - Host: &policy.SSHHostCertificateOptions{ - AllowedNames: &policy.SSHNameOptions{ + newOptions := &policy.Options{ + X509: &policy.X509PolicyOptions{ + AllowedNames: &policy.X509NameOptions{ DNSDomains: []string{"*.local"}, }, - DeniedNames: &policy.SSHNameOptions{ + DeniedNames: &policy.X509NameOptions{ DNSDomains: []string{"badhost.local"}, }, + AllowWildcardLiteral: true, + DisableCommonNameVerification: false, }, - }) - assert.NoError(t, err) - - newSSHUserPolicyEngine, err := policy.NewSSHUserPolicyEngine(&policy.SSHPolicyOptions{ - User: &policy.SSHUserCertificateOptions{ - AllowedNames: &policy.SSHNameOptions{ - Principals: []string{"*"}, + SSH: &policy.SSHPolicyOptions{ + Host: &policy.SSHHostCertificateOptions{ + AllowedNames: &policy.SSHNameOptions{ + DNSDomains: []string{"*.local"}, + }, + DeniedNames: &policy.SSHNameOptions{ + DNSDomains: []string{"badhost.local"}, + }, }, - DeniedNames: &policy.SSHNameOptions{ - Principals: []string{"root"}, + User: &policy.SSHUserCertificateOptions{ + AllowedNames: &policy.SSHNameOptions{ + Principals: []string{"*"}, + }, + DeniedNames: &policy.SSHNameOptions{ + Principals: []string{"root"}, + }, }, }, - }) - assert.NoError(t, err) + } - newAdminX509PolicyEngine, err := policy.NewX509PolicyEngine(&policy.X509PolicyOptions{ - AllowedNames: &policy.X509NameOptions{ - DNSDomains: []string{"*.local"}, + newAdminX509Options := &policy.Options{ + X509: &policy.X509PolicyOptions{ + AllowedNames: &policy.X509NameOptions{ + DNSDomains: []string{"*.local"}, + }, }, - }) - assert.NoError(t, err) + } - newAdminSSHHostPolicyEngine, err := policy.NewSSHHostPolicyEngine(&policy.SSHPolicyOptions{ - Host: &policy.SSHHostCertificateOptions{ - AllowedNames: &policy.SSHNameOptions{ - DNSDomains: []string{"*.local"}, + newAdminSSHHostOptions := &policy.Options{ + SSH: &policy.SSHPolicyOptions{ + Host: &policy.SSHHostCertificateOptions{ + AllowedNames: &policy.SSHNameOptions{ + DNSDomains: []string{"*.local"}, + }, }, }, - }) - assert.NoError(t, err) + } - newAdminSSHUserPolicyEngine, err := policy.NewSSHUserPolicyEngine(&policy.SSHPolicyOptions{ - User: &policy.SSHUserCertificateOptions{ - AllowedNames: &policy.SSHNameOptions{ - EmailAddresses: []string{"@example.com"}, + newAdminSSHUserOptions := &policy.Options{ + SSH: &policy.SSHPolicyOptions{ + User: &policy.SSHUserCertificateOptions{ + AllowedNames: &policy.SSHNameOptions{ + EmailAddresses: []string{"@example.com"}, + }, }, }, - }) - assert.NoError(t, err) + } - type expected struct { - x509Policy policy.X509Policy - sshUserPolicy policy.UserPolicy - sshHostPolicy policy.HostPolicy + newAdminOptions := &policy.Options{ + X509: &policy.X509PolicyOptions{ + AllowedNames: &policy.X509NameOptions{ + DNSDomains: []string{"*.local"}, + }, + DeniedNames: &policy.X509NameOptions{ + DNSDomains: []string{"badhost.local"}, + }, + AllowWildcardLiteral: true, + DisableCommonNameVerification: false, + }, + SSH: &policy.SSHPolicyOptions{ + Host: &policy.SSHHostCertificateOptions{ + AllowedNames: &policy.SSHNameOptions{ + DNSDomains: []string{"*.local"}, + }, + DeniedNames: &policy.SSHNameOptions{ + DNSDomains: []string{"badhost.local"}, + }, + }, + User: &policy.SSHUserCertificateOptions{ + AllowedNames: &policy.SSHNameOptions{ + EmailAddresses: []string{"@example.com"}, + }, + DeniedNames: &policy.SSHNameOptions{ + EmailAddresses: []string{"baduser@example.com"}, + }, + }, + }, } + tests := []struct { name string config *config.Config adminDB admin.DB ctx context.Context - expected *expected + expected *policy.Engine wantErr bool }{ { @@ -445,13 +533,9 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { }, }, }, - ctx: context.Background(), - wantErr: true, - expected: &expected{ - x509Policy: existingX509PolicyEngine, - sshUserPolicy: existingSSHUserPolicyEngine, - sshHostPolicy: existingSSHHostPolicyEngine, - }, + ctx: context.Background(), + wantErr: true, + expected: existingPolicyEngine, }, { name: "fail/standalone-ssh-host-policy", @@ -469,13 +553,9 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { }, }, }, - ctx: context.Background(), - wantErr: true, - expected: &expected{ - x509Policy: existingX509PolicyEngine, - sshUserPolicy: existingSSHUserPolicyEngine, - sshHostPolicy: existingSSHHostPolicyEngine, - }, + ctx: context.Background(), + wantErr: true, + expected: existingPolicyEngine, }, { name: "fail/standalone-ssh-user-policy", @@ -493,13 +573,9 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { }, }, }, - ctx: context.Background(), - wantErr: true, - expected: &expected{ - x509Policy: existingX509PolicyEngine, - sshUserPolicy: existingSSHUserPolicyEngine, - sshHostPolicy: existingSSHHostPolicyEngine, - }, + ctx: context.Background(), + wantErr: true, + expected: existingPolicyEngine, }, { name: "fail/adminDB.GetAuthorityPolicy-error", @@ -513,13 +589,9 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { return nil, errors.New("force") }, }, - ctx: context.Background(), - wantErr: true, - expected: &expected{ - x509Policy: existingX509PolicyEngine, - sshUserPolicy: existingSSHUserPolicyEngine, - sshHostPolicy: existingSSHHostPolicyEngine, - }, + ctx: context.Background(), + wantErr: true, + expected: existingPolicyEngine, }, { name: "fail/admin-x509-policy", @@ -539,13 +611,9 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { }, nil }, }, - ctx: context.Background(), - wantErr: true, - expected: &expected{ - x509Policy: existingX509PolicyEngine, - sshUserPolicy: existingSSHUserPolicyEngine, - sshHostPolicy: existingSSHHostPolicyEngine, - }, + ctx: context.Background(), + wantErr: true, + expected: existingPolicyEngine, }, { name: "fail/admin-ssh-host-policy", @@ -567,13 +635,9 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { }, nil }, }, - ctx: context.Background(), - wantErr: true, - expected: &expected{ - x509Policy: existingX509PolicyEngine, - sshUserPolicy: existingSSHUserPolicyEngine, - sshHostPolicy: existingSSHHostPolicyEngine, - }, + ctx: context.Background(), + wantErr: true, + expected: existingPolicyEngine, }, { name: "fail/admin-ssh-user-policy", @@ -595,13 +659,9 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { }, nil }, }, - ctx: context.Background(), - wantErr: true, - expected: &expected{ - x509Policy: existingX509PolicyEngine, - sshUserPolicy: existingSSHUserPolicyEngine, - sshHostPolicy: existingSSHHostPolicyEngine, - }, + ctx: context.Background(), + wantErr: true, + expected: existingPolicyEngine, }, { name: "ok/linkedca-unsupported", @@ -610,14 +670,10 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { EnableAdmin: true, }, }, - adminDB: &linkedCaClient{}, - ctx: context.Background(), - wantErr: false, - expected: &expected{ - x509Policy: existingX509PolicyEngine, - sshUserPolicy: existingSSHUserPolicyEngine, - sshHostPolicy: existingSSHHostPolicyEngine, - }, + adminDB: &linkedCaClient{}, + ctx: context.Background(), + wantErr: false, + expected: existingPolicyEngine, }, { name: "ok/standalone-no-policy", @@ -627,13 +683,9 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { Policy: nil, }, }, - ctx: context.Background(), - wantErr: false, - expected: &expected{ - x509Policy: nil, - sshUserPolicy: nil, - sshHostPolicy: nil, - }, + ctx: context.Background(), + wantErr: false, + expected: mustPolicyEngine(t, nil), }, { name: "ok/standalone-x509-policy", @@ -654,14 +706,9 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { }, }, }, - ctx: context.Background(), - wantErr: false, - expected: &expected{ - // expect only the X.509 policy to exist - x509Policy: newX509PolicyEngine, - sshHostPolicy: nil, - sshUserPolicy: nil, - }, + ctx: context.Background(), + wantErr: false, + expected: mustPolicyEngine(t, newX509Options), }, { name: "ok/standalone-ssh-host-policy", @@ -682,14 +729,9 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { }, }, }, - ctx: context.Background(), - wantErr: false, - expected: &expected{ - // expect only the SSH host policy to exist - x509Policy: nil, - sshHostPolicy: newSSHHostPolicyEngine, - sshUserPolicy: nil, - }, + ctx: context.Background(), + wantErr: false, + expected: mustPolicyEngine(t, newSSHHostOptions), }, { name: "ok/standalone-ssh-user-policy", @@ -710,14 +752,9 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { }, }, }, - ctx: context.Background(), - wantErr: false, - expected: &expected{ - // expect only the SSH user policy to exist - x509Policy: nil, - sshHostPolicy: nil, - sshUserPolicy: newSSHUserPolicyEngine, - }, + ctx: context.Background(), + wantErr: false, + expected: mustPolicyEngine(t, newSSHUserOptions), }, { name: "ok/standalone-ssh-policy", @@ -746,14 +783,9 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { }, }, }, - ctx: context.Background(), - wantErr: false, - expected: &expected{ - // expect only the SSH policy engines to exist - x509Policy: nil, - sshHostPolicy: newSSHHostPolicyEngine, - sshUserPolicy: newSSHUserPolicyEngine, - }, + ctx: context.Background(), + wantErr: false, + expected: mustPolicyEngine(t, newSSHOptions), }, { name: "ok/standalone-full-policy", @@ -792,14 +824,9 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { }, }, }, - ctx: context.Background(), - wantErr: false, - expected: &expected{ - // expect all three policy engines to exist - x509Policy: newX509PolicyEngine, - sshHostPolicy: newSSHHostPolicyEngine, - sshUserPolicy: newSSHUserPolicyEngine, - }, + ctx: context.Background(), + wantErr: false, + expected: mustPolicyEngine(t, newOptions), }, { name: "ok/admin-x509-policy", @@ -819,13 +846,9 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { }, nil }, }, - ctx: context.Background(), - wantErr: false, - expected: &expected{ - x509Policy: newAdminX509PolicyEngine, - sshHostPolicy: nil, - sshUserPolicy: nil, - }, + ctx: context.Background(), + wantErr: false, + expected: mustPolicyEngine(t, newAdminX509Options), }, { name: "ok/admin-ssh-host-policy", @@ -847,13 +870,9 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { }, nil }, }, - ctx: context.Background(), - wantErr: false, - expected: &expected{ - x509Policy: nil, - sshHostPolicy: newAdminSSHHostPolicyEngine, - sshUserPolicy: nil, - }, + ctx: context.Background(), + wantErr: false, + expected: mustPolicyEngine(t, newAdminSSHHostOptions), }, { name: "ok/admin-ssh-user-policy", @@ -875,13 +894,9 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { }, nil }, }, - ctx: context.Background(), - wantErr: false, - expected: &expected{ - x509Policy: nil, - sshHostPolicy: nil, - sshUserPolicy: newAdminSSHUserPolicyEngine, - }, + ctx: context.Background(), + wantErr: false, + expected: mustPolicyEngine(t, newAdminSSHUserOptions), }, { name: "ok/admin-full-policy", @@ -909,23 +924,24 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { Allow: &linkedca.SSHHostNames{ Dns: []string{"*.local"}, }, + Deny: &linkedca.SSHHostNames{ + Dns: []string{"badhost.local"}, + }, }, User: &linkedca.SSHUserPolicy{ Allow: &linkedca.SSHUserNames{ Emails: []string{"@example.com"}, }, + Deny: &linkedca.SSHUserNames{ + Emails: []string{"baduser@example.com"}, + }, }, }, }, nil }, }, - wantErr: false, - expected: &expected{ - // expect all three policy engines to exist - x509Policy: newX509PolicyEngine, - sshHostPolicy: newAdminSSHHostPolicyEngine, - sshUserPolicy: newAdminSSHUserPolicyEngine, - }, + wantErr: false, + expected: mustPolicyEngine(t, newAdminOptions), }, { // both DB and JSON config; DB config is taken if Admin API is enabled @@ -972,31 +988,27 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { }, nil }, }, - wantErr: false, - expected: &expected{ - // expect all three policy engines to exist - x509Policy: newX509PolicyEngine, - sshHostPolicy: nil, - sshUserPolicy: nil, - }, + wantErr: false, + expected: mustPolicyEngine(t, newX509Options), }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { a := &Authority{ - config: tt.config, - adminDB: tt.adminDB, - x509Policy: existingX509PolicyEngine, - sshUserPolicy: existingSSHUserPolicyEngine, - sshHostPolicy: existingSSHHostPolicyEngine, + config: tt.config, + adminDB: tt.adminDB, + policyEngine: existingPolicyEngine, } if err := a.reloadPolicyEngines(tt.ctx); (err != nil) != tt.wantErr { t.Errorf("Authority.reloadPolicyEngines() error = %v, wantErr %v", err, tt.wantErr) } - assert.Equal(t, tt.expected.x509Policy, a.x509Policy) - assert.Equal(t, tt.expected.sshHostPolicy, a.sshHostPolicy) - assert.Equal(t, tt.expected.sshUserPolicy, a.sshUserPolicy) + // TODO(hs): fix those + // assert.Equal(t, tt.expected.x509Policy, a.x509Policy) + // assert.Equal(t, tt.expected.sshHostPolicy, a.sshHostPolicy) + // assert.Equal(t, tt.expected.sshUserPolicy, a.sshUserPolicy) + + assert.Equal(t, tt.expected, a.policyEngine) }) } } diff --git a/authority/provisioner/acme.go b/authority/provisioner/acme.go index 790c6bb1..c9fa02cc 100644 --- a/authority/provisioner/acme.go +++ b/authority/provisioner/acme.go @@ -118,9 +118,9 @@ func (p *ACME) AuthorizeOrderIdentifier(ctx context.Context, identifier ACMEIden var err error switch identifier.Type { case IP: - _, err = x509Policy.IsIPAllowed(net.ParseIP(identifier.Value)) + err = x509Policy.IsIPAllowed(net.ParseIP(identifier.Value)) case DNS: - _, err = x509Policy.IsDNSAllowed(identifier.Value) + err = x509Policy.IsDNSAllowed(identifier.Value) default: err = fmt.Errorf("invalid ACME identifier type '%s' provided", identifier.Type) } diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go index bac40e69..2eefd331 100644 --- a/authority/provisioner/sign_options.go +++ b/authority/provisioner/sign_options.go @@ -422,8 +422,7 @@ func (v *x509NamePolicyValidator) Valid(cert *x509.Certificate, _ SignOptions) e if v.policyEngine == nil { return nil } - _, err := v.policyEngine.IsX509CertificateAllowed(cert) - return err + return v.policyEngine.IsX509CertificateAllowed(cert) } // var ( diff --git a/authority/provisioner/sign_ssh_options.go b/authority/provisioner/sign_ssh_options.go index a41b8bc1..70dffba2 100644 --- a/authority/provisioner/sign_ssh_options.go +++ b/authority/provisioner/sign_ssh_options.go @@ -480,16 +480,14 @@ func (v *sshNamePolicyValidator) Valid(cert *ssh.Certificate, _ SignSSHOptions) if v.hostPolicyEngine == nil && v.userPolicyEngine != nil { return errors.New("SSH host certificate not authorized") } - _, err := v.hostPolicyEngine.IsSSHCertificateAllowed(cert) - return err + return v.hostPolicyEngine.IsSSHCertificateAllowed(cert) case ssh.UserCert: // when no user policy engine is configured, but a host policy engine is // configured, the user certificate is denied. if v.userPolicyEngine == nil && v.hostPolicyEngine != nil { return errors.New("SSH user certificate not authorized") } - _, err := v.userPolicyEngine.IsSSHCertificateAllowed(cert) - return err + return v.userPolicyEngine.IsSSHCertificateAllowed(cert) default: return fmt.Errorf("unexpected SSH certificate type %d", cert.CertType) // satisfy return; shouldn't happen } diff --git a/authority/ssh.go b/authority/ssh.go index 3f08b88a..0521ab58 100644 --- a/authority/ssh.go +++ b/authority/ssh.go @@ -246,63 +246,21 @@ func (a *Authority) SignSSH(ctx context.Context, key ssh.PublicKey, opts provisi return nil, errs.InternalServer("authority.SignSSH: unexpected ssh certificate type: %d", certTpl.CertType) } - switch certTpl.CertType { - case ssh.UserCert: - // when no user policy engine is configured, but a host policy engine is - // configured, the user certificate is denied. - if a.sshUserPolicy == nil && a.sshHostPolicy != nil { - return nil, errs.ForbiddenErr(errors.New("authority not allowed to sign ssh user certificates"), "authority.SignSSH: error creating ssh user certificate") - } - if a.sshUserPolicy != nil { - allowed, err := a.sshUserPolicy.IsSSHCertificateAllowed(certTpl) - if err != nil { - var pe *policy.NamePolicyError - if errors.As(err, &pe) && pe.Reason == policy.NotAllowed { - return nil, &errs.Error{ - // NOTE: custom forbidden error, so that denied name is sent to client - // as well as shown in the logs. - Status: http.StatusForbidden, - Err: fmt.Errorf("authority not allowed to sign: %w", err), - Msg: fmt.Sprintf("The request was forbidden by the certificate authority: %s", err.Error()), - } - } - return nil, errs.InternalServerErr(err, - errs.WithMessage("authority.SignSSH: error creating ssh user certificate"), - ) - } - if !allowed { - return nil, errs.ForbiddenErr(errors.New("authority not allowed to sign"), "authority.SignSSH: error creating ssh user certificate") + // Check if authority is allowed to sign the certificate + if err := a.isAllowedToSignSSHCertificate(certTpl); err != nil { + var pe *policy.NamePolicyError + if errors.As(err, &pe) && pe.Reason == policy.NotAllowed { + return nil, &errs.Error{ + // NOTE: custom forbidden error, so that denied name is sent to client + // as well as shown in the logs. + Status: http.StatusForbidden, + Err: fmt.Errorf("authority not allowed to sign: %w", err), + Msg: fmt.Sprintf("The request was forbidden by the certificate authority: %s", err.Error()), } } - case ssh.HostCert: - // when no host policy engine is configured, but a user policy engine is - // configured, the host certificate is denied. - if a.sshHostPolicy == nil && a.sshUserPolicy != nil { - return nil, errs.ForbiddenErr(errors.New("authority not allowed to sign ssh host certificates"), "authority.SignSSH: error creating ssh user certificate") - } - if a.sshHostPolicy != nil { - allowed, err := a.sshHostPolicy.IsSSHCertificateAllowed(certTpl) - if err != nil { - var pe *policy.NamePolicyError - if errors.As(err, &pe) && pe.Reason == policy.NotAllowed { - return nil, &errs.Error{ - // NOTE: custom forbidden error, so that denied name is sent to client - // as well as shown in the logs. - Status: http.StatusForbidden, - Err: fmt.Errorf("authority not allowed to sign: %w", err), - Msg: fmt.Sprintf("The request was forbidden by the certificate authority: %s", err.Error()), - } - } - return nil, errs.InternalServerErr(err, - errs.WithMessage("authority.SignSSH: error creating ssh host certificate"), - ) - } - if !allowed { - return nil, errs.ForbiddenErr(errors.New("authority not allowed to sign"), "authority.SignSSH: error creating ssh host certificate") - } - } - default: - return nil, errs.InternalServer("authority.SignSSH: unexpected ssh certificate type: %d", certTpl.CertType) + return nil, errs.InternalServerErr(err, + errs.WithMessage("authority.SignSSH: error creating ssh certificate"), + ) } // Sign certificate. @@ -325,6 +283,11 @@ func (a *Authority) SignSSH(ctx context.Context, key ssh.PublicKey, opts provisi return cert, nil } +// isAllowedToSignSSHCertificate checks if the Authority is allowed to sign the SSH certificate. +func (a *Authority) isAllowedToSignSSHCertificate(cert *ssh.Certificate) error { + return a.policyEngine.IsSSHCertificateAllowed(cert) +} + // RenewSSH creates a signed SSH certificate using the old SSH certificate as a template. func (a *Authority) RenewSSH(ctx context.Context, oldCert *ssh.Certificate) (*ssh.Certificate, error) { if oldCert.ValidAfter == 0 || oldCert.ValidBefore == 0 { diff --git a/authority/ssh_test.go b/authority/ssh_test.go index 2a135f4e..4fd7eaa0 100644 --- a/authority/ssh_test.go +++ b/authority/ssh_test.go @@ -191,21 +191,28 @@ func TestAuthority_SignSSH(t *testing.T) { }, sshutil.CreateTemplateData(sshutil.UserCert, "key-id", []string{"user"})) assert.FatalError(t, err) - policyOptions := &policy.SSHPolicyOptions{ - User: &policy.SSHUserCertificateOptions{ - AllowedNames: &policy.SSHNameOptions{ - Principals: []string{"user"}, + userPolicyOptions := &policy.Options{ + SSH: &policy.SSHPolicyOptions{ + User: &policy.SSHUserCertificateOptions{ + AllowedNames: &policy.SSHNameOptions{ + Principals: []string{"user"}, + }, }, }, - Host: &policy.SSHHostCertificateOptions{ - AllowedNames: &policy.SSHNameOptions{ - DNSDomains: []string{"*.test.com"}, + } + userPolicy, err := policy.New(userPolicyOptions) + assert.FatalError(t, err) + + hostPolicyOptions := &policy.Options{ + SSH: &policy.SSHPolicyOptions{ + Host: &policy.SSHHostCertificateOptions{ + AllowedNames: &policy.SSHNameOptions{ + DNSDomains: []string{"*.test.com"}, + }, }, }, } - userPolicy, err := policy.NewSSHUserPolicyEngine(policyOptions) - assert.FatalError(t, err) - hostPolicy, err := policy.NewSSHHostPolicyEngine(policyOptions) + hostPolicy, err := policy.New(hostPolicyOptions) assert.FatalError(t, err) now := time.Now() @@ -213,8 +220,7 @@ func TestAuthority_SignSSH(t *testing.T) { type fields struct { sshCAUserCertSignKey ssh.Signer sshCAHostCertSignKey ssh.Signer - sshUserPolicy policy.UserPolicy - sshHostPolicy policy.HostPolicy + policyEngine *policy.Engine } type args struct { key ssh.PublicKey @@ -234,49 +240,48 @@ func TestAuthority_SignSSH(t *testing.T) { want want wantErr bool }{ - {"ok-user", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions}}, want{CertType: ssh.UserCert}, false}, - {"ok-host", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{hostTemplate, hostOptions}}, want{CertType: ssh.HostCert}, false}, - {"ok-user-only", fields{signer, nil, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions}}, want{CertType: ssh.UserCert}, false}, - {"ok-host-only", fields{nil, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{hostTemplate, hostOptions}}, want{CertType: ssh.HostCert}, false}, - {"ok-opts-type-user", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{CertType: "user"}, []provisioner.SignOption{userTemplate}}, want{CertType: ssh.UserCert}, false}, - {"ok-opts-type-host", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{CertType: "host"}, []provisioner.SignOption{hostTemplate}}, want{CertType: ssh.HostCert}, false}, - {"ok-opts-principals", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{CertType: "user", Principals: []string{"user"}}, []provisioner.SignOption{userTemplateWithUser}}, want{CertType: ssh.UserCert, Principals: []string{"user"}}, false}, - {"ok-opts-principals", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{CertType: "host", Principals: []string{"foo.test.com", "bar.test.com"}}, []provisioner.SignOption{hostTemplateWithHosts}}, want{CertType: ssh.HostCert, Principals: []string{"foo.test.com", "bar.test.com"}}, false}, - {"ok-opts-valid-after", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{CertType: "user", ValidAfter: provisioner.NewTimeDuration(now)}, []provisioner.SignOption{userTemplate}}, want{CertType: ssh.UserCert, ValidAfter: uint64(now.Unix())}, false}, - {"ok-opts-valid-before", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{CertType: "host", ValidBefore: provisioner.NewTimeDuration(now)}, []provisioner.SignOption{hostTemplate}}, want{CertType: ssh.HostCert, ValidBefore: uint64(now.Unix())}, false}, - {"ok-cert-validator", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, sshTestCertValidator("")}}, want{CertType: ssh.UserCert}, false}, - {"ok-cert-modifier", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, sshTestCertModifier("")}}, want{CertType: ssh.UserCert}, false}, - {"ok-opts-validator", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, sshTestOptionsValidator("")}}, want{CertType: ssh.UserCert}, false}, - {"ok-opts-modifier", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, sshTestOptionsModifier("")}}, want{CertType: ssh.UserCert}, false}, - {"ok-custom-template", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userCustomTemplate, userOptions}}, want{CertType: ssh.UserCert, Principals: []string{"user", "admin"}}, false}, - {"ok-user-policy", fields{signer, signer, userPolicy, nil}, args{pub, provisioner.SignSSHOptions{CertType: "user", Principals: []string{"user"}}, []provisioner.SignOption{userTemplateWithUser}}, want{CertType: ssh.UserCert, Principals: []string{"user"}}, false}, - {"ok-host-policy", fields{signer, signer, nil, hostPolicy}, args{pub, provisioner.SignSSHOptions{CertType: "host", Principals: []string{"foo.test.com", "bar.test.com"}}, []provisioner.SignOption{hostTemplateWithHosts}}, want{CertType: ssh.HostCert, Principals: []string{"foo.test.com", "bar.test.com"}}, false}, - {"fail-opts-type", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{CertType: "foo"}, []provisioner.SignOption{userTemplate}}, want{}, true}, - {"fail-cert-validator", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, sshTestCertValidator("an error")}}, want{}, true}, - {"fail-cert-modifier", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, sshTestCertModifier("an error")}}, want{}, true}, - {"fail-opts-validator", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, sshTestOptionsValidator("an error")}}, want{}, true}, - {"fail-opts-modifier", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, sshTestOptionsModifier("an error")}}, want{}, true}, - {"fail-bad-sign-options", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, "wrong type"}}, want{}, true}, - {"fail-no-user-key", fields{nil, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{CertType: "user"}, []provisioner.SignOption{userTemplate}}, want{}, true}, - {"fail-no-host-key", fields{signer, nil, nil, nil}, args{pub, provisioner.SignSSHOptions{CertType: "host"}, []provisioner.SignOption{hostTemplate}}, want{}, true}, - {"fail-bad-type", fields{signer, nil, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, sshTestModifier{CertType: 100}}}, want{}, true}, - {"fail-custom-template", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userFailTemplate, userOptions}}, want{}, true}, - {"fail-custom-template-syntax-error-file", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userJSONSyntaxErrorTemplateFile, userOptions}}, want{}, true}, - {"fail-custom-template-syntax-value-file", fields{signer, signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userJSONValueErrorTemplateFile, userOptions}}, want{}, true}, - {"fail-user-policy", fields{signer, signer, userPolicy, nil}, args{pub, provisioner.SignSSHOptions{CertType: "user", Principals: []string{"root"}}, []provisioner.SignOption{userTemplateWithRoot}}, want{}, true}, - {"fail-user-policy-with-host-cert", fields{signer, signer, userPolicy, nil}, args{pub, provisioner.SignSSHOptions{CertType: "host", Principals: []string{"foo.test.com"}}, []provisioner.SignOption{hostTemplateWithExampleDotCom}}, want{}, true}, - {"fail-user-policy-with-bad-user", fields{signer, signer, userPolicy, nil}, args{pub, provisioner.SignSSHOptions{CertType: "user", Principals: []string{"user"}}, []provisioner.SignOption{badUserTemplate}}, want{}, true}, - {"fail-host-policy", fields{signer, signer, nil, hostPolicy}, args{pub, provisioner.SignSSHOptions{CertType: "host", Principals: []string{"example.com"}}, []provisioner.SignOption{hostTemplateWithExampleDotCom}}, want{}, true}, - {"fail-host-policy-with-user-cert", fields{signer, signer, nil, hostPolicy}, args{pub, provisioner.SignSSHOptions{CertType: "user", Principals: []string{"user"}}, []provisioner.SignOption{userTemplateWithUser}}, want{}, true}, - {"fail-host-policy-with-bad-host", fields{signer, signer, nil, hostPolicy}, args{pub, provisioner.SignSSHOptions{CertType: "host", Principals: []string{"example.com"}}, []provisioner.SignOption{badHostTemplate}}, want{}, true}, + {"ok-user", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions}}, want{CertType: ssh.UserCert}, false}, + {"ok-host", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{hostTemplate, hostOptions}}, want{CertType: ssh.HostCert}, false}, + {"ok-user-only", fields{signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions}}, want{CertType: ssh.UserCert}, false}, + {"ok-host-only", fields{nil, signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{hostTemplate, hostOptions}}, want{CertType: ssh.HostCert}, false}, + {"ok-opts-type-user", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{CertType: "user"}, []provisioner.SignOption{userTemplate}}, want{CertType: ssh.UserCert}, false}, + {"ok-opts-type-host", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{CertType: "host"}, []provisioner.SignOption{hostTemplate}}, want{CertType: ssh.HostCert}, false}, + {"ok-opts-principals", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{CertType: "user", Principals: []string{"user"}}, []provisioner.SignOption{userTemplateWithUser}}, want{CertType: ssh.UserCert, Principals: []string{"user"}}, false}, + {"ok-opts-principals", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{CertType: "host", Principals: []string{"foo.test.com", "bar.test.com"}}, []provisioner.SignOption{hostTemplateWithHosts}}, want{CertType: ssh.HostCert, Principals: []string{"foo.test.com", "bar.test.com"}}, false}, + {"ok-opts-valid-after", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{CertType: "user", ValidAfter: provisioner.NewTimeDuration(now)}, []provisioner.SignOption{userTemplate}}, want{CertType: ssh.UserCert, ValidAfter: uint64(now.Unix())}, false}, + {"ok-opts-valid-before", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{CertType: "host", ValidBefore: provisioner.NewTimeDuration(now)}, []provisioner.SignOption{hostTemplate}}, want{CertType: ssh.HostCert, ValidBefore: uint64(now.Unix())}, false}, + {"ok-cert-validator", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, sshTestCertValidator("")}}, want{CertType: ssh.UserCert}, false}, + {"ok-cert-modifier", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, sshTestCertModifier("")}}, want{CertType: ssh.UserCert}, false}, + {"ok-opts-validator", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, sshTestOptionsValidator("")}}, want{CertType: ssh.UserCert}, false}, + {"ok-opts-modifier", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, sshTestOptionsModifier("")}}, want{CertType: ssh.UserCert}, false}, + {"ok-custom-template", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userCustomTemplate, userOptions}}, want{CertType: ssh.UserCert, Principals: []string{"user", "admin"}}, false}, + {"ok-user-policy", fields{signer, signer, userPolicy}, args{pub, provisioner.SignSSHOptions{CertType: "user", Principals: []string{"user"}}, []provisioner.SignOption{userTemplateWithUser}}, want{CertType: ssh.UserCert, Principals: []string{"user"}}, false}, + {"ok-host-policy", fields{signer, signer, hostPolicy}, args{pub, provisioner.SignSSHOptions{CertType: "host", Principals: []string{"foo.test.com", "bar.test.com"}}, []provisioner.SignOption{hostTemplateWithHosts}}, want{CertType: ssh.HostCert, Principals: []string{"foo.test.com", "bar.test.com"}}, false}, + {"fail-opts-type", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{CertType: "foo"}, []provisioner.SignOption{userTemplate}}, want{}, true}, + {"fail-cert-validator", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, sshTestCertValidator("an error")}}, want{}, true}, + {"fail-cert-modifier", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, sshTestCertModifier("an error")}}, want{}, true}, + {"fail-opts-validator", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, sshTestOptionsValidator("an error")}}, want{}, true}, + {"fail-opts-modifier", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, sshTestOptionsModifier("an error")}}, want{}, true}, + {"fail-bad-sign-options", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, userOptions, "wrong type"}}, want{}, true}, + {"fail-no-user-key", fields{nil, signer, nil}, args{pub, provisioner.SignSSHOptions{CertType: "user"}, []provisioner.SignOption{userTemplate}}, want{}, true}, + {"fail-no-host-key", fields{signer, nil, nil}, args{pub, provisioner.SignSSHOptions{CertType: "host"}, []provisioner.SignOption{hostTemplate}}, want{}, true}, + {"fail-bad-type", fields{signer, nil, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userTemplate, sshTestModifier{CertType: 100}}}, want{}, true}, + {"fail-custom-template", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userFailTemplate, userOptions}}, want{}, true}, + {"fail-custom-template-syntax-error-file", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userJSONSyntaxErrorTemplateFile, userOptions}}, want{}, true}, + {"fail-custom-template-syntax-value-file", fields{signer, signer, nil}, args{pub, provisioner.SignSSHOptions{}, []provisioner.SignOption{userJSONValueErrorTemplateFile, userOptions}}, want{}, true}, + {"fail-user-policy", fields{signer, signer, userPolicy}, args{pub, provisioner.SignSSHOptions{CertType: "user", Principals: []string{"root"}}, []provisioner.SignOption{userTemplateWithRoot}}, want{}, true}, + {"fail-user-policy-with-host-cert", fields{signer, signer, userPolicy}, args{pub, provisioner.SignSSHOptions{CertType: "host", Principals: []string{"foo.test.com"}}, []provisioner.SignOption{hostTemplateWithExampleDotCom}}, want{}, true}, + {"fail-user-policy-with-bad-user", fields{signer, signer, userPolicy}, args{pub, provisioner.SignSSHOptions{CertType: "user", Principals: []string{"user"}}, []provisioner.SignOption{badUserTemplate}}, want{}, true}, + {"fail-host-policy", fields{signer, signer, hostPolicy}, args{pub, provisioner.SignSSHOptions{CertType: "host", Principals: []string{"example.com"}}, []provisioner.SignOption{hostTemplateWithExampleDotCom}}, want{}, true}, + {"fail-host-policy-with-user-cert", fields{signer, signer, hostPolicy}, args{pub, provisioner.SignSSHOptions{CertType: "user", Principals: []string{"user"}}, []provisioner.SignOption{userTemplateWithUser}}, want{}, true}, + {"fail-host-policy-with-bad-host", fields{signer, signer, hostPolicy}, args{pub, provisioner.SignSSHOptions{CertType: "host", Principals: []string{"example.com"}}, []provisioner.SignOption{badHostTemplate}}, want{}, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { a := testAuthority(t) a.sshCAUserCertSignKey = tt.fields.sshCAUserCertSignKey a.sshCAHostCertSignKey = tt.fields.sshCAHostCertSignKey - a.sshUserPolicy = tt.fields.sshUserPolicy - a.sshHostPolicy = tt.fields.sshHostPolicy + a.policyEngine = tt.fields.policyEngine got, err := a.SignSSH(context.Background(), tt.args.key, tt.args.opts, tt.args.signOpts...) if (err != nil) != tt.wantErr { diff --git a/authority/tls.go b/authority/tls.go index cc34ff6a..d23b0da7 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -200,8 +200,7 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Sign } // Check if authority is allowed to sign the certificate - var allowedToSign bool - if allowedToSign, err = a.isAllowedToSign(leaf); err != nil { + if err := a.isAllowedToSignX509Certificate(leaf); err != nil { var pe *policy.NamePolicyError if errors.As(err, &pe) && pe.Reason == policy.NotAllowed { return nil, errs.ApplyOptions(&errs.Error{ @@ -218,12 +217,6 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Sign errs.WithMessage("error creating certificate"), ) } - if !allowedToSign { - return nil, errs.ApplyOptions( - errs.ForbiddenErr(errors.New("authority not allowed to sign"), "error creating certificate"), - opts..., - ) - } // Sign certificate lifetime := leaf.NotAfter.Sub(leaf.NotBefore.Add(signOpts.Backdate)) @@ -248,31 +241,16 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Sign return fullchain, nil } -// isAllowedToSign checks if the Authority is allowed to sign the X.509 certificate. -func (a *Authority) isAllowedToSign(cert *x509.Certificate) (bool, error) { - - // if no policy is configured, the cert is implicitly allowed - if a.x509Policy == nil { - return true, nil - } - - return a.x509Policy.IsX509CertificateAllowed(cert) +// isAllowedToSignX509Certificate checks if the Authority is allowed +// to sign the X.509 certificate. +func (a *Authority) isAllowedToSignX509Certificate(cert *x509.Certificate) error { + return a.policyEngine.IsX509CertificateAllowed(cert) } // AreSANsAllowed evaluates the provided sans against the // authority X.509 policy. func (a *Authority) AreSANsAllowed(ctx context.Context, sans []string) error { - - // no policy configured; return early - if a.x509Policy == nil { - return nil - } - - // evaluate the X.509 policy for the provided sans - var err error - _, err = a.x509Policy.AreSANsAllowed(sans) - - return err + return a.policyEngine.AreSANsAllowed(sans) } // Renew creates a new Certificate identical to the old certificate, except diff --git a/authority/tls_test.go b/authority/tls_test.go index 3f9946ba..3739dbff 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -523,14 +523,16 @@ ZYtQ9Ot36qc= return nil }, } - policyOptions := &policy.X509PolicyOptions{ - DeniedNames: &policy.X509NameOptions{ - DNSDomains: []string{"test.smallstep.com"}, + options := &policy.Options{ + X509: &policy.X509PolicyOptions{ + DeniedNames: &policy.X509NameOptions{ + DNSDomains: []string{"test.smallstep.com"}, + }, }, } - engine, err := policy.NewX509PolicyEngine(policyOptions) + engine, err := policy.New(options) assert.FatalError(t, err) - aa.x509Policy = engine + aa.policyEngine = engine return &signTest{ auth: aa, csr: csr, @@ -696,15 +698,17 @@ ZYtQ9Ot36qc= return nil }, } - policyOptions := &policy.X509PolicyOptions{ - AllowedNames: &policy.X509NameOptions{ - DNSDomains: []string{"*.smallstep.com"}, + options := &policy.Options{ + X509: &policy.X509PolicyOptions{ + AllowedNames: &policy.X509NameOptions{ + DNSDomains: []string{"*.smallstep.com"}, + }, + DisableCommonNameVerification: true, // TODO(hs): allows "smallstep test"; do we want to keep it like this? }, - DisableCommonNameVerification: true, // TODO(hs): allows "smallstep test"; do we want to keep it like this? } - engine, err := policy.NewX509PolicyEngine(policyOptions) + engine, err := policy.New(options) assert.FatalError(t, err) - aa.x509Policy = engine + aa.policyEngine = engine return &signTest{ auth: aa, csr: csr, diff --git a/policy/engine.go b/policy/engine.go index d03665ee..3d8a4755 100755 --- a/policy/engine.go +++ b/policy/engine.go @@ -15,11 +15,10 @@ import ( type NamePolicyReason int const ( - _ NamePolicyReason = iota // NotAllowed results when an instance of NamePolicyEngine // determines that there's a constraint which doesn't permit // a DNS or another type of SAN to be signed (or otherwise used). - NotAllowed + NotAllowed NamePolicyReason = iota + 1 // CannotParseDomain is returned when an error occurs // when parsing the domain part of SAN or subject. CannotParseDomain @@ -198,7 +197,7 @@ func removeDuplicateIPNets(items []*net.IPNet) (ret []*net.IPNet) { } // IsX509CertificateAllowed verifies that all SANs in a Certificate are allowed. -func (e *NamePolicyEngine) IsX509CertificateAllowed(cert *x509.Certificate) (bool, error) { +func (e *NamePolicyEngine) IsX509CertificateAllowed(cert *x509.Certificate) error { dnsNames, ips, emails, uris := cert.DNSNames, cert.IPAddresses, cert.EmailAddresses, cert.URIs // when Subject Common Name must be verified in addition to the SANs, it is // added to the appropriate slice of names. @@ -206,13 +205,13 @@ func (e *NamePolicyEngine) IsX509CertificateAllowed(cert *x509.Certificate) (boo appendSubjectCommonName(cert.Subject, &dnsNames, &ips, &emails, &uris) } if err := e.validateNames(dnsNames, ips, emails, uris, []string{}); err != nil { - return false, err + return err } - return true, nil + return nil } // IsX509CertificateRequestAllowed verifies that all names in the CSR are allowed. -func (e *NamePolicyEngine) IsX509CertificateRequestAllowed(csr *x509.CertificateRequest) (bool, error) { +func (e *NamePolicyEngine) IsX509CertificateRequestAllowed(csr *x509.CertificateRequest) error { dnsNames, ips, emails, uris := csr.DNSNames, csr.IPAddresses, csr.EmailAddresses, csr.URIs // when Subject Common Name must be verified in addition to the SANs, it is // added to the appropriate slice of names. @@ -220,47 +219,47 @@ func (e *NamePolicyEngine) IsX509CertificateRequestAllowed(csr *x509.Certificate appendSubjectCommonName(csr.Subject, &dnsNames, &ips, &emails, &uris) } if err := e.validateNames(dnsNames, ips, emails, uris, []string{}); err != nil { - return false, err + return err } - return true, nil + return nil } // AreSANSAllowed verifies that all names in the slice of SANs are allowed. // The SANs are first split into DNS names, IPs, email addresses and URIs. -func (e *NamePolicyEngine) AreSANsAllowed(sans []string) (bool, error) { +func (e *NamePolicyEngine) AreSANsAllowed(sans []string) error { dnsNames, ips, emails, uris := x509util.SplitSANs(sans) if err := e.validateNames(dnsNames, ips, emails, uris, []string{}); err != nil { - return false, err + return err } - return true, nil + return nil } // IsDNSAllowed verifies a single DNS domain is allowed. -func (e *NamePolicyEngine) IsDNSAllowed(dns string) (bool, error) { +func (e *NamePolicyEngine) IsDNSAllowed(dns string) error { if err := e.validateNames([]string{dns}, []net.IP{}, []string{}, []*url.URL{}, []string{}); err != nil { - return false, err + return err } - return true, nil + return nil } // IsIPAllowed verifies a single IP domain is allowed. -func (e *NamePolicyEngine) IsIPAllowed(ip net.IP) (bool, error) { +func (e *NamePolicyEngine) IsIPAllowed(ip net.IP) error { if err := e.validateNames([]string{}, []net.IP{ip}, []string{}, []*url.URL{}, []string{}); err != nil { - return false, err + return err } - return true, nil + return nil } // IsSSHCertificateAllowed verifies that all principals in an SSH certificate are allowed. -func (e *NamePolicyEngine) IsSSHCertificateAllowed(cert *ssh.Certificate) (bool, error) { +func (e *NamePolicyEngine) IsSSHCertificateAllowed(cert *ssh.Certificate) error { dnsNames, ips, emails, principals, err := splitSSHPrincipals(cert) if err != nil { - return false, err + return err } if err := e.validateNames(dnsNames, ips, emails, []*url.URL{}, principals); err != nil { - return false, err + return err } - return true, nil + return nil } // appendSubjectCommonName appends the Subject Common Name to the appropriate slice of names. The logic is diff --git a/policy/engine_test.go b/policy/engine_test.go index a99885ea..deec2ff9 100755 --- a/policy/engine_test.go +++ b/policy/engine_test.go @@ -2391,16 +2391,16 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { t.Run(tt.name, func(t *testing.T) { engine, err := New(tt.options...) assert.NoError(t, err) - got, err := engine.IsX509CertificateAllowed(tt.cert) + gotErr := engine.IsX509CertificateAllowed(tt.cert) wantErr := tt.wantErr != nil - if (err != nil) != wantErr { - t.Errorf("NamePolicyEngine.IsX509CertificateAllowed() error = %v, wantErr %v", err, tt.wantErr) + if (gotErr != nil) != wantErr { + t.Errorf("NamePolicyEngine.IsX509CertificateAllowed() error = %v, wantErr %v", gotErr, tt.wantErr) return } - if err != nil { + if gotErr != nil { var npe *NamePolicyError - assert.True(t, errors.As(err, &npe)) + assert.True(t, errors.As(gotErr, &npe)) assert.NotEqual(t, "", npe.Error()) assert.Equal(t, tt.wantErr.Reason, npe.Reason) assert.Equal(t, tt.wantErr.NameType, npe.NameType) @@ -2408,9 +2408,6 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { assert.NotEqual(t, "", npe.Detail()) //assert.Equals(t, tt.err.Reason, npe.Reason) // NOTE: reason detail is skipped; it's a detail } - if got != tt.want { - t.Errorf("NamePolicyEngine.IsX509CertificateAllowed() = %v, want %v", got, tt.want) - } // Perform the same tests for a CSR, which are similar to Certificates csr := &x509.CertificateRequest{ @@ -2420,15 +2417,15 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { IPAddresses: tt.cert.IPAddresses, URIs: tt.cert.URIs, } - got, err = engine.IsX509CertificateRequestAllowed(csr) + gotErr = engine.IsX509CertificateRequestAllowed(csr) wantErr = tt.wantErr != nil - if (err != nil) != wantErr { - t.Errorf("NamePolicyEngine.AreCSRNamesAllowed() error = %v, wantErr %v", err, tt.wantErr) + if (gotErr != nil) != wantErr { + t.Errorf("NamePolicyEngine.AreCSRNamesAllowed() error = %v, wantErr %v", gotErr, tt.wantErr) return } - if err != nil { + if gotErr != nil { var npe *NamePolicyError - assert.True(t, errors.As(err, &npe)) + assert.True(t, errors.As(gotErr, &npe)) assert.NotEqual(t, "", npe.Error()) assert.Equal(t, tt.wantErr.Reason, npe.Reason) assert.Equal(t, tt.wantErr.NameType, npe.NameType) @@ -2436,22 +2433,19 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { assert.NotEqual(t, "", npe.Detail()) //assert.Equals(t, tt.err.Reason, npe.Reason) // NOTE: reason detail is skipped; it's a detail } - if got != tt.want { - t.Errorf("NamePolicyEngine.AreCSRNamesAllowed() = %v, want %v", got, tt.want) - } // Perform the same tests for a slice of SANs includeSubject := engine.verifySubjectCommonName // copy behavior of the engine when Subject has to be included as a SAN sans := extractSANs(tt.cert, includeSubject) - got, err = engine.AreSANsAllowed(sans) + gotErr = engine.AreSANsAllowed(sans) wantErr = tt.wantErr != nil - if (err != nil) != wantErr { - t.Errorf("NamePolicyEngine.AreSANsAllowed() error = %v, wantErr %v", err, tt.wantErr) + if (gotErr != nil) != wantErr { + t.Errorf("NamePolicyEngine.AreSANsAllowed() error = %v, wantErr %v", gotErr, tt.wantErr) return } - if err != nil { + if gotErr != nil { var npe *NamePolicyError - assert.True(t, errors.As(err, &npe)) + assert.True(t, errors.As(gotErr, &npe)) assert.NotEqual(t, "", npe.Error()) assert.Equal(t, tt.wantErr.Reason, npe.Reason) assert.Equal(t, tt.wantErr.NameType, npe.NameType) @@ -2459,9 +2453,6 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { assert.NotEqual(t, "", npe.Detail()) //assert.Equals(t, tt.err.Reason, npe.Reason) // NOTE: reason detail is skipped; it's a detail } - if got != tt.want { - t.Errorf("NamePolicyEngine.AreSANsAllowed() = %v, want %v", got, tt.want) - } }) } } @@ -2955,15 +2946,15 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) { t.Run(tt.name, func(t *testing.T) { engine, err := New(tt.options...) assert.NoError(t, err) - got, err := engine.IsSSHCertificateAllowed(tt.cert) + gotErr := engine.IsSSHCertificateAllowed(tt.cert) wantErr := tt.wantErr != nil - if (err != nil) != wantErr { - t.Errorf("NamePolicyEngine.IsSSHCertificateAllowed() error = %v, wantErr %v", err, tt.wantErr) + if (gotErr != nil) != wantErr { + t.Errorf("NamePolicyEngine.IsSSHCertificateAllowed() error = %v, wantErr %v", gotErr, tt.wantErr) return } - if err != nil { + if gotErr != nil { var npe *NamePolicyError - assert.True(t, errors.As(err, &npe)) + assert.True(t, errors.As(gotErr, &npe)) assert.NotEqual(t, "", npe.Error()) assert.Equal(t, tt.wantErr.Reason, npe.Reason) assert.Equal(t, tt.wantErr.NameType, npe.NameType) @@ -2971,9 +2962,6 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) { assert.NotEqual(t, "", npe.Detail()) //assert.Equals(t, tt.err.Reason, npe.Reason) // NOTE: reason detail is skipped; it's a detail } - if got != tt.want { - t.Errorf("NamePolicyEngine.IsSSHCertificateAllowed() = %v, want %v", got, tt.want) - } }) } } diff --git a/policy/ssh.go b/policy/ssh.go index 1ebecb2e..725f9b7b 100644 --- a/policy/ssh.go +++ b/policy/ssh.go @@ -5,5 +5,5 @@ import ( ) type SSHNamePolicyEngine interface { - IsSSHCertificateAllowed(cert *ssh.Certificate) (bool, error) + IsSSHCertificateAllowed(cert *ssh.Certificate) error } diff --git a/policy/x509.go b/policy/x509.go index 666e1b5c..8b6c4de9 100644 --- a/policy/x509.go +++ b/policy/x509.go @@ -6,9 +6,9 @@ import ( ) type X509NamePolicyEngine interface { - IsX509CertificateAllowed(cert *x509.Certificate) (bool, error) - IsX509CertificateRequestAllowed(csr *x509.CertificateRequest) (bool, error) - AreSANsAllowed(sans []string) (bool, error) - IsDNSAllowed(dns string) (bool, error) - IsIPAllowed(ip net.IP) (bool, error) + IsX509CertificateAllowed(cert *x509.Certificate) error + IsX509CertificateRequestAllowed(csr *x509.CertificateRequest) error + AreSANsAllowed(sans []string) error + IsDNSAllowed(dns string) error + IsIPAllowed(ip net.IP) error }