From 96f4c49b0ccc3f268b3d43c73b9adae9495f52be Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 4 Apr 2022 13:58:16 +0200 Subject: [PATCH] Improve how policy errors are returned and used --- authority/admin/api/middleware_test.go | 93 ++++++++ authority/admin/api/policy.go | 51 +++-- authority/authority.go | 3 +- authority/policy.go | 139 +++++++++--- authority/policy_test.go | 295 +++++++++++++++++++++++++ authority/provisioners.go | 14 ++ policy/engine_test.go | 2 +- policy/validate.go | 17 +- 8 files changed, 559 insertions(+), 55 deletions(-) create mode 100644 authority/policy_test.go diff --git a/authority/admin/api/middleware_test.go b/authority/admin/api/middleware_test.go index c7314e71..3dfc5823 100644 --- a/authority/admin/api/middleware_test.go +++ b/authority/admin/api/middleware_test.go @@ -20,6 +20,7 @@ import ( "github.com/smallstep/assert" "github.com/smallstep/certificates/authority/admin" + "github.com/smallstep/certificates/authority/admin/db/nosql" "github.com/smallstep/certificates/authority/provisioner" ) @@ -356,3 +357,95 @@ func TestHandler_loadProvisionerByName(t *testing.T) { }) } } + +func TestHandler_checkAction(t *testing.T) { + + type test struct { + adminDB admin.DB + next http.HandlerFunc + supportedInStandalone bool + err *admin.Error + statusCode int + } + var tests = map[string]func(t *testing.T) test{ + "standalone-mockdb-supported": func(t *testing.T) test { + err := admin.NewError(admin.ErrorNotImplementedType, "operation not supported") + err.Message = "operation not supported" + return test{ + adminDB: &admin.MockDB{}, + statusCode: 501, + err: err, + } + }, + "standalone-nosql-supported": func(t *testing.T) test { + return test{ + supportedInStandalone: true, + adminDB: &nosql.DB{}, + next: func(w http.ResponseWriter, r *http.Request) { + w.Write(nil) // mock response with status 200 + }, + statusCode: 200, + } + }, + "standalone-nosql-not-supported": func(t *testing.T) test { + err := admin.NewError(admin.ErrorNotImplementedType, "operation not supported in standalone mode") + err.Message = "operation not supported in standalone mode" + return test{ + supportedInStandalone: false, + adminDB: &nosql.DB{}, + next: func(w http.ResponseWriter, r *http.Request) { + w.Write(nil) // mock response with status 200 + }, + statusCode: 501, + err: err, + } + }, + "standalone-no-nosql-not-supported": func(t *testing.T) test { + // TODO(hs): temporarily expects an error instead of an OK response + err := admin.NewError(admin.ErrorNotImplementedType, "operation not supported") + err.Message = "operation not supported" + return test{ + supportedInStandalone: false, + adminDB: &admin.MockDB{}, + next: func(w http.ResponseWriter, r *http.Request) { + w.Write(nil) // mock response with status 200 + }, + statusCode: 501, + err: err, + } + }, + } + + for name, prep := range tests { + tc := prep(t) + t.Run(name, func(t *testing.T) { + h := &Handler{ + + adminDB: tc.adminDB, + } + + req := httptest.NewRequest("GET", "/foo", nil) + w := httptest.NewRecorder() + h.checkAction(tc.next, tc.supportedInStandalone)(w, req) + res := w.Result() + + assert.Equals(t, tc.statusCode, res.StatusCode) + + body, err := io.ReadAll(res.Body) + res.Body.Close() + assert.FatalError(t, err) + + if res.StatusCode >= 400 { + err := admin.Error{} + assert.FatalError(t, json.Unmarshal(bytes.TrimSpace(body), &err)) + + assert.Equals(t, tc.err.Type, err.Type) + assert.Equals(t, tc.err.Message, err.Message) + assert.Equals(t, tc.err.StatusCode(), res.StatusCode) + assert.Equals(t, tc.err.Detail, err.Detail) + assert.Equals(t, []string{"application/json"}, res.Header["Content-Type"]) + return + } + }) + } +} diff --git a/authority/admin/api/policy.go b/authority/admin/api/policy.go index da0e1d9c..44344271 100644 --- a/authority/admin/api/policy.go +++ b/authority/admin/api/policy.go @@ -1,12 +1,14 @@ package api import ( + "errors" "net/http" "go.step.sm/linkedca" "github.com/smallstep/certificates/api/read" "github.com/smallstep/certificates/api/render" + "github.com/smallstep/certificates/authority" "github.com/smallstep/certificates/authority/admin" ) @@ -43,11 +45,9 @@ func NewPolicyAdminResponder(auth adminAuthority, adminDB admin.DB) *PolicyAdmin func (par *PolicyAdminResponder) GetAuthorityPolicy(w http.ResponseWriter, r *http.Request) { policy, err := par.auth.GetAuthorityPolicy(r.Context()) - if ae, ok := err.(*admin.Error); ok { - if !ae.IsType(admin.ErrorNotFoundType) { - render.Error(w, admin.WrapErrorISE(ae, "error retrieving authority policy")) - return - } + if ae, ok := err.(*admin.Error); ok && !ae.IsType(admin.ErrorNotFoundType) { + render.Error(w, admin.WrapErrorISE(ae, "error retrieving authority policy")) + return } if policy == nil { @@ -85,7 +85,14 @@ func (par *PolicyAdminResponder) CreateAuthorityPolicy(w http.ResponseWriter, r var createdPolicy *linkedca.Policy if createdPolicy, err = par.auth.CreateAuthorityPolicy(ctx, adm, newPolicy); err != nil { - render.Error(w, admin.WrapError(admin.ErrorBadRequestType, err, "error storing authority policy")) + var pe *authority.PolicyError + + if errors.As(err, &pe); pe.Typ == authority.AdminLockOut || pe.Typ == authority.EvaluationFailure { + render.Error(w, admin.WrapError(admin.ErrorBadRequestType, pe, "error storing authority policy")) + return + } + + render.Error(w, admin.WrapErrorISE(err, "error storing authority policy")) return } @@ -118,7 +125,13 @@ func (par *PolicyAdminResponder) UpdateAuthorityPolicy(w http.ResponseWriter, r var updatedPolicy *linkedca.Policy if updatedPolicy, err = par.auth.UpdateAuthorityPolicy(ctx, adm, newPolicy); err != nil { - render.Error(w, admin.WrapError(admin.ErrorBadRequestType, err, "error updating authority policy")) + var pe *authority.PolicyError + if errors.As(err, &pe); pe.Typ == authority.AdminLockOut || pe.Typ == authority.EvaluationFailure { + render.Error(w, admin.WrapError(admin.ErrorBadRequestType, pe, "error updating authority policy")) + return + } + + render.Error(w, admin.WrapErrorISE(err, "error updating authority policy")) return } @@ -131,11 +144,9 @@ func (par *PolicyAdminResponder) DeleteAuthorityPolicy(w http.ResponseWriter, r ctx := r.Context() policy, err := par.auth.GetAuthorityPolicy(ctx) - if ae, ok := err.(*admin.Error); ok { - if !ae.IsType(admin.ErrorNotFoundType) { - render.Error(w, admin.WrapErrorISE(ae, "error retrieving authority policy")) - return - } + if ae, ok := err.(*admin.Error); ok && !ae.IsType(admin.ErrorNotFoundType) { + render.Error(w, admin.WrapErrorISE(ae, "error retrieving authority policy")) + return } if policy == nil { @@ -189,7 +200,13 @@ func (par *PolicyAdminResponder) CreateProvisionerPolicy(w http.ResponseWriter, err := par.auth.UpdateProvisioner(ctx, prov) if err != nil { - render.Error(w, admin.WrapError(admin.ErrorBadRequestType, err, "error creating provisioner policy")) + var pe *authority.PolicyError + if errors.As(err, &pe); pe.Typ == authority.AdminLockOut || pe.Typ == authority.EvaluationFailure { + render.Error(w, admin.WrapError(admin.ErrorBadRequestType, pe, "error creating provisioner policy")) + return + } + + render.Error(w, admin.WrapErrorISE(err, "error creating provisioner policy")) return } @@ -215,6 +232,12 @@ func (par *PolicyAdminResponder) UpdateProvisionerPolicy(w http.ResponseWriter, prov.Policy = newPolicy err := par.auth.UpdateProvisioner(ctx, prov) if err != nil { + var pe *authority.PolicyError + if errors.As(err, &pe); pe.Typ == authority.AdminLockOut || pe.Typ == authority.EvaluationFailure { + render.Error(w, admin.WrapError(admin.ErrorBadRequestType, pe, "error updating provisioner policy")) + return + } + render.Error(w, admin.WrapError(admin.ErrorBadRequestType, err, "error updating provisioner policy")) return } @@ -238,7 +261,7 @@ func (par *PolicyAdminResponder) DeleteProvisionerPolicy(w http.ResponseWriter, err := par.auth.UpdateProvisioner(ctx, prov) if err != nil { - render.Error(w, err) + render.Error(w, admin.WrapErrorISE(err, "error deleting provisioner policy")) return } diff --git a/authority/authority.go b/authority/authority.go index 5caec0fb..c451aef5 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -6,6 +6,7 @@ import ( "crypto/sha256" "crypto/x509" "encoding/hex" + "fmt" "log" "strings" "sync" @@ -235,7 +236,7 @@ func (a *Authority) reloadPolicyEngines(ctx context.Context) error { linkedPolicy, err := a.adminDB.GetAuthorityPolicy(ctx) if err != nil { - return admin.WrapErrorISE(err, "error getting policy to (re)load policy engines") + return fmt.Errorf("error getting policy to (re)load policy engines: %w", err) } policyOptions = policyToCertificates(linkedPolicy) } else { diff --git a/authority/policy.go b/authority/policy.go index bb57a7d0..a88258fe 100644 --- a/authority/policy.go +++ b/authority/policy.go @@ -7,11 +7,31 @@ import ( "go.step.sm/linkedca" - "github.com/smallstep/certificates/authority/admin" authPolicy "github.com/smallstep/certificates/authority/policy" policy "github.com/smallstep/certificates/policy" ) +type policyErrorType int + +const ( + _ policyErrorType = iota + AdminLockOut + StoreFailure + ReloadFailure + ConfigurationFailure + EvaluationFailure + InternalFailure +) + +type PolicyError struct { + Typ policyErrorType + err error +} + +func (p *PolicyError) Error() string { + return p.err.Error() +} + func (a *Authority) GetAuthorityPolicy(ctx context.Context) (*linkedca.Policy, error) { a.adminMutex.Lock() defer a.adminMutex.Unlock() @@ -28,16 +48,25 @@ func (a *Authority) CreateAuthorityPolicy(ctx context.Context, adm *linkedca.Adm a.adminMutex.Lock() defer a.adminMutex.Unlock() - if err := a.checkPolicy(ctx, adm, p); err != nil { - return nil, err + if err := a.checkAuthorityPolicy(ctx, adm, p); err != nil { + return nil, &PolicyError{ + Typ: AdminLockOut, + err: err, + } } if err := a.adminDB.CreateAuthorityPolicy(ctx, p); err != nil { - return nil, err + return nil, &PolicyError{ + Typ: StoreFailure, + err: err, + } } if err := a.reloadPolicyEngines(ctx); err != nil { - return nil, admin.WrapErrorISE(err, "error reloading policy engines when creating authority policy") + return nil, &PolicyError{ + Typ: ReloadFailure, + err: fmt.Errorf("error reloading policy engines when creating authority policy: %w", err), + } } return p, nil // TODO: return the newly stored policy @@ -47,16 +76,22 @@ func (a *Authority) UpdateAuthorityPolicy(ctx context.Context, adm *linkedca.Adm a.adminMutex.Lock() defer a.adminMutex.Unlock() - if err := a.checkPolicy(ctx, adm, p); err != nil { + if err := a.checkAuthorityPolicy(ctx, adm, p); err != nil { return nil, err } if err := a.adminDB.UpdateAuthorityPolicy(ctx, p); err != nil { - return nil, err + return nil, &PolicyError{ + Typ: StoreFailure, + err: err, + } } if err := a.reloadPolicyEngines(ctx); err != nil { - return nil, admin.WrapErrorISE(err, "error reloading policy engines when updating authority policy") + return nil, &PolicyError{ + Typ: ReloadFailure, + err: fmt.Errorf("error reloading policy engines when updating authority policy %w", err), + } } return p, nil // TODO: return the updated stored policy @@ -67,19 +102,63 @@ func (a *Authority) RemoveAuthorityPolicy(ctx context.Context) error { defer a.adminMutex.Unlock() if err := a.adminDB.DeleteAuthorityPolicy(ctx); err != nil { - return err + return &PolicyError{ + Typ: StoreFailure, + err: err, + } } if err := a.reloadPolicyEngines(ctx); err != nil { - return admin.WrapErrorISE(err, "error reloading policy engines when deleting authority policy") + return &PolicyError{ + Typ: ReloadFailure, + err: fmt.Errorf("error reloading policy engines when deleting authority policy %w", err), + } } return nil } +func (a *Authority) checkAuthorityPolicy(ctx context.Context, currentAdmin *linkedca.Admin, p *linkedca.Policy) error { + + // no policy and thus nothing to evaluate; return early + if p == nil { + return nil + } + + // get all current admins from the database + allAdmins, err := a.adminDB.GetAdmins(ctx) + if err != nil { + return &PolicyError{ + Typ: InternalFailure, + err: fmt.Errorf("error retrieving admins: %w", err), + } + } + + return a.checkPolicy(ctx, currentAdmin, allAdmins, p) +} + +func (a *Authority) checkProvisionerPolicy(ctx context.Context, currentAdmin *linkedca.Admin, provName string, p *linkedca.Policy) error { + + // no policy and thus nothing to evaluate; return early + if p == nil { + return nil + } + + // get all admins for the provisioner + allProvisionerAdmins, ok := a.admins.LoadByProvisioner(provName) + if !ok { + return &PolicyError{ + Typ: InternalFailure, + err: errors.New("error retrieving admins by provisioner"), + } + } + + return a.checkPolicy(ctx, currentAdmin, allProvisionerAdmins, p) +} + // checkPolicy checks if a new or updated policy configuration results in the user // locking themselves or other admins out of the CA. -func (a *Authority) checkPolicy(ctx context.Context, adm *linkedca.Admin, p *linkedca.Policy) error { +func (a *Authority) checkPolicy(ctx context.Context, currentAdmin *linkedca.Admin, otherAdmins []*linkedca.Admin, p *linkedca.Policy) error { // convert the policy; return early if nil policyOptions := policyToCertificates(p) @@ -89,10 +168,13 @@ func (a *Authority) checkPolicy(ctx context.Context, adm *linkedca.Admin, p *lin engine, err := authPolicy.NewX509PolicyEngine(policyOptions.GetX509Options()) if err != nil { - return admin.WrapErrorISE(err, "error creating temporary policy engine") + return &PolicyError{ + Typ: ConfigurationFailure, + err: fmt.Errorf("error creating temporary policy engine: %w", err), + } } - // when an empty policy is provided, the resulting engine is nil + // when an empty X.509 policy is provided, the resulting engine is nil // and there's no policy to evaluate. if engine == nil { return nil @@ -102,23 +184,17 @@ func (a *Authority) checkPolicy(ctx context.Context, adm *linkedca.Admin, p *lin // check if the admin user that instructed the authority policy to be // created or updated, would still be allowed when the provided policy - // would be applied to the authority. - sans := []string{adm.GetSubject()} + // would be applied. + sans := []string{currentAdmin.GetSubject()} if err := isAllowed(engine, sans); err != nil { return err } - // get all current admins from the database - admins, err := a.adminDB.GetAdmins(ctx) - if err != nil { - return err - } - // loop through admins to verify that none of them would be // locked out when the new policy were to be applied. Returns // an error with a message that includes the admin subject that - // would be locked out - for _, adm := range admins { + // would be locked out. + for _, adm := range otherAdmins { sans = []string{adm.GetSubject()} if err := isAllowed(engine, sans); err != nil { return err @@ -137,14 +213,23 @@ func isAllowed(engine authPolicy.X509Policy, sans []string) error { ) if allowed, err = engine.AreSANsAllowed(sans); err != nil { var policyErr *policy.NamePolicyError - if isPolicyErr := errors.As(err, &policyErr); isPolicyErr && policyErr.Reason == policy.NotAuthorizedForThisName { - return 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) + if errors.As(err, &policyErr); policyErr.Reason == policy.NotAuthorizedForThisName { + 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 &PolicyError{ + Typ: EvaluationFailure, + err: err, } - return err } if !allowed { - return 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 &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_test.go b/authority/policy_test.go new file mode 100644 index 00000000..39edf700 --- /dev/null +++ b/authority/policy_test.go @@ -0,0 +1,295 @@ +package authority + +import ( + "context" + "errors" + "testing" + + "github.com/google/go-cmp/cmp" + + "go.step.sm/linkedca" + + "github.com/smallstep/assert" + authPolicy "github.com/smallstep/certificates/authority/policy" +) + +func TestAuthority_checkPolicy(t *testing.T) { + type test struct { + ctx context.Context + currentAdmin *linkedca.Admin + otherAdmins []*linkedca.Admin + policy *linkedca.Policy + err *PolicyError + } + tests := map[string]func(t *testing.T) test{ + "fail/NewX509PolicyEngine-error": func(t *testing.T) test { + return test{ + ctx: context.Background(), + policy: &linkedca.Policy{ + X509: &linkedca.X509Policy{ + Allow: &linkedca.X509Names{ + Dns: []string{"**.local"}, + }, + }, + }, + err: &PolicyError{ + Typ: ConfigurationFailure, + err: errors.New("error creating temporary policy engine: cannot parse permitted domain constraint \"**.local\""), + }, + } + }, + "fail/currentAdmin-evaluation-error": func(t *testing.T) test { + return test{ + ctx: context.Background(), + currentAdmin: &linkedca.Admin{Subject: "*"}, + otherAdmins: []*linkedca.Admin{}, + policy: &linkedca.Policy{ + X509: &linkedca.X509Policy{ + Allow: &linkedca.X509Names{ + Dns: []string{".local"}, + }, + }, + }, + err: &PolicyError{ + Typ: EvaluationFailure, + err: errors.New("cannot parse domain: dns \"*\" cannot be converted to ASCII"), + }, + } + }, + "fail/currentAdmin-lockout": func(t *testing.T) test { + return test{ + ctx: context.Background(), + currentAdmin: &linkedca.Admin{Subject: "step"}, + otherAdmins: []*linkedca.Admin{ + { + Subject: "otherAdmin", + }, + }, + policy: &linkedca.Policy{ + X509: &linkedca.X509Policy{ + Allow: &linkedca.X509Names{ + Dns: []string{".local"}, + }, + }, + }, + err: &PolicyError{ + Typ: AdminLockOut, + err: errors.New("the provided policy would lock out [step] from the CA. Please update your policy to include [step] as an allowed name"), + }, + } + }, + "fail/otherAdmins-evaluation-error": func(t *testing.T) test { + return test{ + ctx: context.Background(), + currentAdmin: &linkedca.Admin{Subject: "step"}, + otherAdmins: []*linkedca.Admin{ + { + Subject: "other", + }, + { + Subject: "**", + }, + }, + policy: &linkedca.Policy{ + X509: &linkedca.X509Policy{ + Allow: &linkedca.X509Names{ + Dns: []string{"step", "other", "*.local"}, + }, + }, + }, + err: &PolicyError{ + Typ: EvaluationFailure, + err: errors.New("cannot parse domain: dns \"**\" cannot be converted to ASCII"), + }, + } + }, + "fail/otherAdmins-lockout": func(t *testing.T) test { + return test{ + ctx: context.Background(), + currentAdmin: &linkedca.Admin{Subject: "step"}, + otherAdmins: []*linkedca.Admin{ + { + Subject: "otherAdmin", + }, + }, + policy: &linkedca.Policy{ + X509: &linkedca.X509Policy{ + Allow: &linkedca.X509Names{ + Dns: []string{"step"}, + }, + }, + }, + err: &PolicyError{ + Typ: AdminLockOut, + err: errors.New("the provided policy would lock out [otherAdmin] from the CA. Please update your policy to include [otherAdmin] as an allowed name"), + }, + } + }, + "ok/no-policy": func(t *testing.T) test { + return test{ + ctx: context.Background(), + currentAdmin: &linkedca.Admin{Subject: "step"}, + otherAdmins: []*linkedca.Admin{}, + policy: nil, + } + }, + "ok/empty-policy": func(t *testing.T) test { + return test{ + ctx: context.Background(), + currentAdmin: &linkedca.Admin{Subject: "step"}, + otherAdmins: []*linkedca.Admin{}, + policy: &linkedca.Policy{ + X509: &linkedca.X509Policy{ + Allow: &linkedca.X509Names{ + Dns: []string{}, + }, + }, + }, + } + }, + "ok/policy": func(t *testing.T) test { + return test{ + ctx: context.Background(), + currentAdmin: &linkedca.Admin{Subject: "step"}, + otherAdmins: []*linkedca.Admin{ + { + Subject: "otherAdmin", + }, + }, + policy: &linkedca.Policy{ + X509: &linkedca.X509Policy{ + Allow: &linkedca.X509Names{ + Dns: []string{"step", "otherAdmin"}, + }, + }, + }, + } + }, + } + + for name, prep := range tests { + tc := prep(t) + t.Run(name, func(t *testing.T) { + a := &Authority{} + + err := a.checkPolicy(tc.ctx, tc.currentAdmin, tc.otherAdmins, tc.policy) + + if tc.err == nil { + assert.Nil(t, err) + } else { + assert.Type(t, &PolicyError{}, err) + + pe, ok := err.(*PolicyError) + assert.Fatal(t, ok) + + assert.Equals(t, tc.err.Typ, pe.Typ) + assert.Equals(t, tc.err.Error(), pe.Error()) + } + }) + } +} + +func Test_policyToCertificates(t *testing.T) { + tests := []struct { + name string + policy *linkedca.Policy + want *authPolicy.Options + }{ + { + name: "no-policy", + policy: nil, + want: nil, + }, + { + name: "full-policy", + policy: &linkedca.Policy{ + X509: &linkedca.X509Policy{ + Allow: &linkedca.X509Names{ + Dns: []string{"step"}, + Ips: []string{"127.0.0.1/24"}, + Emails: []string{"*.example.com"}, + Uris: []string{"https://*.local"}, + }, + Deny: &linkedca.X509Names{ + Dns: []string{"bad"}, + Ips: []string{"127.0.0.30"}, + Emails: []string{"badhost.example.com"}, + Uris: []string{"https://badhost.local"}, + }, + }, + Ssh: &linkedca.SSHPolicy{ + Host: &linkedca.SSHHostPolicy{ + Allow: &linkedca.SSHHostNames{ + Dns: []string{"*.localhost"}, + Ips: []string{"127.0.0.1/24"}, + Principals: []string{"user"}, + }, + Deny: &linkedca.SSHHostNames{ + Dns: []string{"badhost.localhost"}, + Ips: []string{"127.0.0.40"}, + Principals: []string{"root"}, + }, + }, + User: &linkedca.SSHUserPolicy{ + Allow: &linkedca.SSHUserNames{ + Emails: []string{"@work"}, + Principals: []string{"user"}, + }, + Deny: &linkedca.SSHUserNames{ + Emails: []string{"root@work"}, + Principals: []string{"root"}, + }, + }, + }, + }, + want: &authPolicy.Options{ + X509: &authPolicy.X509PolicyOptions{ + AllowedNames: &authPolicy.X509NameOptions{ + DNSDomains: []string{"step"}, + IPRanges: []string{"127.0.0.1/24"}, + EmailAddresses: []string{"*.example.com"}, + URIDomains: []string{"https://*.local"}, + }, + DeniedNames: &authPolicy.X509NameOptions{ + DNSDomains: []string{"bad"}, + IPRanges: []string{"127.0.0.30"}, + EmailAddresses: []string{"badhost.example.com"}, + URIDomains: []string{"https://badhost.local"}, + }, + }, + SSH: &authPolicy.SSHPolicyOptions{ + Host: &authPolicy.SSHHostCertificateOptions{ + AllowedNames: &authPolicy.SSHNameOptions{ + DNSDomains: []string{"*.localhost"}, + IPRanges: []string{"127.0.0.1/24"}, + Principals: []string{"user"}, + }, + DeniedNames: &authPolicy.SSHNameOptions{ + DNSDomains: []string{"badhost.localhost"}, + IPRanges: []string{"127.0.0.40"}, + Principals: []string{"root"}, + }, + }, + User: &authPolicy.SSHUserCertificateOptions{ + AllowedNames: &authPolicy.SSHNameOptions{ + EmailAddresses: []string{"@work"}, + Principals: []string{"user"}, + }, + DeniedNames: &authPolicy.SSHNameOptions{ + EmailAddresses: []string{"root@work"}, + Principals: []string{"root"}, + }, + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := policyToCertificates(tt.policy) + if !cmp.Equal(tt.want, got) { + t.Errorf("policyToCertificates() diff=\n%s", cmp.Diff(tt.want, got)) + } + }) + } +} diff --git a/authority/provisioners.go b/authority/provisioners.go index b47eff1d..1bea7c1b 100644 --- a/authority/provisioners.go +++ b/authority/provisioners.go @@ -141,6 +141,12 @@ func (a *Authority) StoreProvisioner(ctx context.Context, prov *linkedca.Provisi return admin.WrapErrorISE(err, "error generating provisioner config") } + adm := linkedca.AdminFromContext(ctx) + + if err := a.checkProvisionerPolicy(ctx, adm, prov.Name, prov.Policy); err != nil { + return err + } + if err := certProv.Init(provisionerConfig); err != nil { return admin.WrapError(admin.ErrorBadRequestType, err, "error validating configuration for provisioner %s", prov.Name) } @@ -186,6 +192,12 @@ func (a *Authority) UpdateProvisioner(ctx context.Context, nu *linkedca.Provisio return admin.WrapErrorISE(err, "error generating provisioner config") } + adm := linkedca.AdminFromContext(ctx) + + if err := a.checkProvisionerPolicy(ctx, adm, nu.Name, nu.Policy); err != nil { + return err + } + if err := certProv.Init(provisionerConfig); err != nil { return admin.WrapErrorISE(err, "error initializing provisioner %s", nu.Name) } @@ -424,12 +436,14 @@ func optionsToCertificates(p *linkedca.Provisioner) *provisioner.Options { ops.SSH.Host.AllowedNames = &policy.SSHNameOptions{ DNSDomains: p.Policy.Ssh.Host.Allow.Dns, IPRanges: p.Policy.Ssh.Host.Allow.Ips, + Principals: p.Policy.Ssh.Host.Allow.Principals, } } if p.Policy.Ssh.Host.Deny != nil { ops.SSH.Host.DeniedNames = &policy.SSHNameOptions{ DNSDomains: p.Policy.Ssh.Host.Deny.Dns, IPRanges: p.Policy.Ssh.Host.Deny.Ips, + Principals: p.Policy.Ssh.Host.Deny.Principals, } } } diff --git a/policy/engine_test.go b/policy/engine_test.go index 603ef6ce..38aa709a 100755 --- a/policy/engine_test.go +++ b/policy/engine_test.go @@ -14,7 +14,7 @@ import ( ) // TODO(hs): the functionality in the policy engine is a nice candidate for trying fuzzing on -// TODO(hs): more complex use cases that combine multiple names and permitted/excluded entries +// TODO(hs): more complex test use cases that combine multiple names and permitted/excluded entries? func TestNamePolicyEngine_matchDomainConstraint(t *testing.T) { tests := []struct { diff --git a/policy/validate.go b/policy/validate.go index f259515f..b85eb299 100644 --- a/policy/validate.go +++ b/policy/validate.go @@ -1,6 +1,9 @@ // Copyright 2011 The Go Authors. All rights reserved. // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. +// +// The code in this file is an adapted version of the code in +// https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go package policy import ( @@ -15,8 +18,6 @@ import ( ) // validateNames verifies that all names are allowed. -// Its logic follows that of (a large part of) the (c *Certificate) isValid() function -// in https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailAddresses []string, uris []*url.URL, principals []string) error { // nothing to compare against; return early @@ -160,7 +161,6 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA // checkNameConstraints checks that a name, of type nameType is permitted. // The argument parsedName contains the parsed form of name, suitable for passing // to the match function. -// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go func checkNameConstraints( nameType string, name string, @@ -218,7 +218,6 @@ func checkNameConstraints( // domainToReverseLabels converts a textual domain name like foo.example.com to // the list of labels in reverse order, e.g. ["com", "example", "foo"]. -// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go func domainToReverseLabels(domain string) (reverseLabels []string, ok bool) { for len(domain) > 0 { if i := strings.LastIndexByte(domain, '.'); i == -1 { @@ -255,7 +254,6 @@ func domainToReverseLabels(domain string) (reverseLabels []string, ok bool) { // rfc2821Mailbox represents a “mailbox” (which is an email address to most // people) by breaking it into the “local” (i.e. before the '@') and “domain” // parts. -// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go type rfc2821Mailbox struct { local, domain string } @@ -264,7 +262,6 @@ type rfc2821Mailbox struct { // based on the ABNF for a “Mailbox” from RFC 2821. According to RFC 5280, // Section 4.2.1.6 that's correct for an rfc822Name from a certificate: “The // format of an rfc822Name is a "Mailbox" as defined in RFC 2821, Section 4.1.2”. -// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go func parseRFC2821Mailbox(in string) (mailbox rfc2821Mailbox, ok bool) { if in == "" { return mailbox, false @@ -401,7 +398,7 @@ func parseRFC2821Mailbox(in string) (mailbox rfc2821Mailbox, ok bool) { return mailbox, true } -// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go +// matchDomainConstraint matches a domain agains the given constraint func (e *NamePolicyEngine) matchDomainConstraint(domain, constraint string) (bool, error) { // The meaning of zero length constraints is not specified, but this // code follows NSS and accepts them as matching everything. @@ -462,10 +459,6 @@ func (e *NamePolicyEngine) matchDomainConstraint(domain, constraint string) (boo return false, fmt.Errorf("cannot parse domain constraint %q", constraint) } - // fmt.Println(mustHaveSubdomains) - // fmt.Println(constraintLabels) - // fmt.Println(domainLabels) - expectedNumberOfLabels := len(constraintLabels) if mustHaveSubdomains { // we expect exactly one more label if it starts with the "canonical" x509 "wildcard": "." @@ -532,7 +525,7 @@ func (e *NamePolicyEngine) matchEmailConstraint(mailbox rfc2821Mailbox, constrai return e.matchDomainConstraint(mailbox.domain, constraint) } -// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go +// matchURIConstraint matches an URL against a constraint func (e *NamePolicyEngine) matchURIConstraint(uri *url.URL, constraint string) (bool, error) { // From RFC 5280, Section 4.2.1.10: // “a uniformResourceIdentifier that does not include an authority