alexvanin/certificates
1
0
Fork 0
forked from TrueCloudLab/certificates
Code Pull requests Activity

Merge pull request #933 from smallstep/herman/allow-deny

Browse source 
Fix check for admin not belonging to provisioner that policy applies to
This commit is contained in:
Herman Slatman 2022-05-12 16:42:26 +02:00 committed by GitHub
commit ea084d71fb
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 105 additions and 26 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

21
authority/policy.go
View file

@ -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
}

102
authority/policy_test.go
View file

@ -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)
}
})

8
authority/provisioners.go
View file

@ -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
}