From c695b23e24655eb838bc775aa674ee928fd7bb60 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 12 May 2022 16:33:32 +0200 Subject: [PATCH] Fix check for admin not belonging to policy --- authority/policy.go | 21 +++++--- authority/policy_test.go | 102 +++++++++++++++++++++++++++++++++----- authority/provisioners.go | 8 +-- 3 files changed, 105 insertions(+), 26 deletions(-) diff --git a/authority/policy.go b/authority/policy.go index 6348c690..258873af 100644 --- a/authority/policy.go +++ b/authority/policy.go @@ -137,7 +137,7 @@ func (a *Authority) checkAuthorityPolicy(ctx context.Context, currentAdmin *link return a.checkPolicy(ctx, currentAdmin, allAdmins, p) } -func (a *Authority) checkProvisionerPolicy(ctx context.Context, currentAdmin *linkedca.Admin, provName string, p *linkedca.Policy) error { +func (a *Authority) checkProvisionerPolicy(ctx context.Context, provName string, p *linkedca.Policy) error { // no policy and thus nothing to evaluate; return early if p == nil { @@ -147,7 +147,11 @@ func (a *Authority) checkProvisionerPolicy(ctx context.Context, currentAdmin *li // get all admins for the provisioner; ignoring case in which they're not found allProvisionerAdmins, _ := a.admins.LoadByProvisioner(provName) - return a.checkPolicy(ctx, currentAdmin, allProvisionerAdmins, p) + // check the policy; pass in nil as the current admin, as all admins for the + // provisioner will be checked by looping through allProvisionerAdmins. Also, + // the current admin may be a super admin not belonging to the provisioner, so + // can't be blocked, but is not required to be in the policy, either. + return a.checkPolicy(ctx, nil, allProvisionerAdmins, p) } // checkPolicy checks if a new or updated policy configuration results in the user @@ -178,10 +182,13 @@ func (a *Authority) checkPolicy(ctx context.Context, currentAdmin *linkedca.Admi // 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. - sans := []string{currentAdmin.GetSubject()} - if err := isAllowed(engine, sans); err != nil { - return err + // would be applied. This case is skipped when current admin is nil, which + // is the case when a provisioner policy is checked. + if currentAdmin != nil { + sans := []string{currentAdmin.GetSubject()} + if err := isAllowed(engine, sans); err != nil { + return err + } } // loop through admins to verify that none of them would be @@ -189,7 +196,7 @@ func (a *Authority) checkPolicy(ctx context.Context, currentAdmin *linkedca.Admi // an error with a message that includes the admin subject that // would be locked out. for _, adm := range otherAdmins { - sans = []string{adm.GetSubject()} + sans := []string{adm.GetSubject()} if err := isAllowed(engine, sans); err != nil { return err } diff --git a/authority/policy_test.go b/authority/policy_test.go index efeb743b..1dccf0d1 100644 --- a/authority/policy_test.go +++ b/authority/policy_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "gopkg.in/square/go-jose.v2" "go.step.sm/linkedca" @@ -917,13 +918,58 @@ func TestAuthority_checkAuthorityPolicy(t *testing.T) { }, wantErr: true, }, + { + name: "fail/policy", + fields: fields{ + admins: administrator.NewCollection(nil), + adminDB: &admin.MockDB{ + MockGetAdmins: func(ctx context.Context) ([]*linkedca.Admin, error) { + return []*linkedca.Admin{ + { + Id: "adminID1", + Subject: "anotherAdmin", + }, + { + Id: "adminID2", + Subject: "step", + }, + { + Id: "adminID3", + Subject: "otherAdmin", + }, + }, nil + }, + }, + }, + args: args{ + currentAdmin: &linkedca.Admin{Subject: "step"}, + provName: "prov", + p: &linkedca.Policy{ + X509: &linkedca.X509Policy{ + Allow: &linkedca.X509Names{ + Dns: []string{"step", "otherAdmin"}, + }, + }, + }, + }, + wantErr: true, + }, { name: "ok", fields: fields{ admins: administrator.NewCollection(nil), adminDB: &admin.MockDB{ MockGetAdmins: func(ctx context.Context) ([]*linkedca.Admin, error) { - return []*linkedca.Admin{}, nil + return []*linkedca.Admin{ + { + Id: "adminID2", + Subject: "step", + }, + { + Id: "adminID3", + Subject: "otherAdmin", + }, + }, nil }, }, }, @@ -957,6 +1003,20 @@ func TestAuthority_checkAuthorityPolicy(t *testing.T) { } func TestAuthority_checkProvisionerPolicy(t *testing.T) { + jwkProvisioner := &provisioner.JWK{ + ID: "jwkID", + Type: "JWK", + Name: "jwkProv", + Key: &jose.JSONWebKey{KeyID: "jwkKeyID"}, + } + provisioners := provisioner.NewCollection(testAudiences) + provisioners.Store(jwkProvisioner) + admins := administrator.NewCollection(provisioners) + admins.Store(&linkedca.Admin{ + Id: "adminID", + Subject: "step", + ProvisionerId: "jwkID", + }, jwkProvisioner) type fields struct { provisioners *provisioner.Collection admins *administrator.Collection @@ -964,10 +1024,9 @@ func TestAuthority_checkProvisionerPolicy(t *testing.T) { adminDB admin.DB } type args struct { - ctx context.Context - currentAdmin *linkedca.Admin - provName string - p *linkedca.Policy + ctx context.Context + provName string + p *linkedca.Policy } tests := []struct { name string @@ -979,20 +1038,37 @@ func TestAuthority_checkProvisionerPolicy(t *testing.T) { name: "no policy", fields: fields{}, args: args{ - currentAdmin: nil, - provName: "prov", - p: nil, + provName: "prov", + p: nil, }, wantErr: false, }, { - name: "ok", + name: "fail/policy", fields: fields{ - admins: administrator.NewCollection(nil), + provisioners: provisioners, + admins: admins, }, args: args{ - currentAdmin: &linkedca.Admin{Subject: "step"}, - provName: "prov", + provName: "jwkProv", + p: &linkedca.Policy{ + X509: &linkedca.X509Policy{ + Allow: &linkedca.X509Names{ + Dns: []string{"otherAdmin"}, // step not in policy + }, + }, + }, + }, + wantErr: true, + }, + { + name: "ok", + fields: fields{ + provisioners: provisioners, + admins: admins, + }, + args: args{ + provName: "jwkProv", p: &linkedca.Policy{ X509: &linkedca.X509Policy{ Allow: &linkedca.X509Names{ @@ -1012,7 +1088,7 @@ func TestAuthority_checkProvisionerPolicy(t *testing.T) { db: tt.fields.db, adminDB: tt.fields.adminDB, } - if err := a.checkProvisionerPolicy(tt.args.ctx, tt.args.currentAdmin, tt.args.provName, tt.args.p); (err != nil) != tt.wantErr { + if err := a.checkProvisionerPolicy(tt.args.ctx, tt.args.provName, tt.args.p); (err != nil) != tt.wantErr { t.Errorf("Authority.checkProvisionerPolicy() error = %v, wantErr %v", err, tt.wantErr) } }) diff --git a/authority/provisioners.go b/authority/provisioners.go index cde4a6e9..76c1a129 100644 --- a/authority/provisioners.go +++ b/authority/provisioners.go @@ -173,9 +173,7 @@ func (a *Authority) StoreProvisioner(ctx context.Context, prov *linkedca.Provisi return admin.WrapErrorISE(err, "error generating provisioner config") } - adm := linkedca.MustAdminFromContext(ctx) - - if err := a.checkProvisionerPolicy(ctx, adm, prov.Name, prov.Policy); err != nil { + if err := a.checkProvisionerPolicy(ctx, prov.Name, prov.Policy); err != nil { return err } @@ -224,9 +222,7 @@ func (a *Authority) UpdateProvisioner(ctx context.Context, nu *linkedca.Provisio return admin.WrapErrorISE(err, "error generating provisioner config") } - adm := linkedca.MustAdminFromContext(ctx) - - if err := a.checkProvisionerPolicy(ctx, adm, nu.Name, nu.Policy); err != nil { + if err := a.checkProvisionerPolicy(ctx, nu.Name, nu.Policy); err != nil { return err }