From 13173ec8a2d3fdc4150304aeb60937bb4ddf20df Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sun, 1 May 2022 22:29:17 +0200 Subject: [PATCH 1/4] Fix SCEP GET requests --- scep/api/api.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scep/api/api.go b/scep/api/api.go index 31f0f10d..fcabfc58 100644 --- a/scep/api/api.go +++ b/scep/api/api.go @@ -86,7 +86,7 @@ func (h *handler) Get(w http.ResponseWriter, r *http.Request) { case opnGetCACaps: res, err = h.GetCACaps(ctx) case opnPKIOperation: - // TODO: implement the GET for PKI operation? Default CACAPS doesn't specify this is in use, though + res, err = h.PKIOperation(ctx, req) default: err = fmt.Errorf("unknown operation: %s", req.Operation) } @@ -151,8 +151,8 @@ func decodeRequest(r *http.Request) (request, error) { if _, ok := query["message"]; ok { message = query.Get("message") } - // TODO: verify this; it seems like it should be StdEncoding instead of URLEncoding - decodedMessage, err := base64.URLEncoding.DecodeString(message) + // TODO: verify this; right type of encoding? Needs additional transformations? + decodedMessage, err := base64.StdEncoding.DecodeString(message) if err != nil { return request{}, err } From 02c0ae81ac07af43d7009621919f4c3248750800 Mon Sep 17 00:00:00 2001 From: vijayjt <2975049+vijayjt@users.noreply.github.com> Date: Thu, 5 May 2022 00:10:59 +0100 Subject: [PATCH 2/4] Allow KMS type to be specified in the helm chart template if specified on the command line. --- pki/helm.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pki/helm.go b/pki/helm.go index 0a2f7f02..e13bb97c 100644 --- a/pki/helm.go +++ b/pki/helm.go @@ -68,6 +68,10 @@ inject: federateRoots: [] crt: {{ .Intermediate }} key: {{ .IntermediateKey }} + {{- if .Kms }} + kms: + type: {{ lower (.Kms.Type | toString) }} + {{- end }} {{- if .EnableSSH }} ssh: hostKey: {{ .Ssh.HostKey }} From 688ae837a4beaa8ec1c147a8c33aa2f4c22ef561 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sat, 7 May 2022 00:26:18 +0200 Subject: [PATCH 3/4] Add some tests for SCEP request decoding --- scep/api/api_test.go | 113 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100644 scep/api/api_test.go diff --git a/scep/api/api_test.go b/scep/api/api_test.go new file mode 100644 index 00000000..bdb51594 --- /dev/null +++ b/scep/api/api_test.go @@ -0,0 +1,113 @@ +// Package api implements a SCEP HTTP server. +package api + +import ( + "bytes" + "errors" + "net/http" + "net/http/httptest" + "reflect" + "testing" + "testing/iotest" +) + +func Test_decodeRequest(t *testing.T) { + type args struct { + r *http.Request + } + tests := []struct { + name string + args args + want request + wantErr bool + }{ + { + name: "fail/unsupported-method", + args: args{ + r: httptest.NewRequest(http.MethodPatch, "http://scep:8080/?operation=AnUnsupportOperation", nil), + }, + want: request{}, + wantErr: true, + }, + { + name: "fail/get-unsupported-operation", + args: args{ + r: httptest.NewRequest(http.MethodGet, "http://scep:8080/?operation=AnUnsupportOperation", nil), + }, + want: request{}, + wantErr: true, + }, + { + name: "fail/get-PKIOperation", + args: args{ + r: httptest.NewRequest(http.MethodGet, "http://scep:8080/?operation=PKIOperation&message='somewronginput'", nil), + }, + want: request{}, + wantErr: true, + }, + { + name: "fail/post-PKIOperation", + args: args{ + r: httptest.NewRequest(http.MethodPost, "http://scep:8080/?operation=PKIOperation", iotest.ErrReader(errors.New("a read error"))), + }, + want: request{}, + wantErr: true, + }, + { + name: "ok/get-GetCACert", + args: args{ + r: httptest.NewRequest(http.MethodGet, "http://scep:8080/?operation=GetCACert", nil), + }, + want: request{ + Operation: "GetCACert", + Message: []byte{}, + }, + wantErr: false, + }, + { + name: "ok/get-GetCACaps", + args: args{ + r: httptest.NewRequest(http.MethodGet, "http://scep:8080/?operation=GetCACaps", nil), + }, + want: request{ + Operation: "GetCACaps", + Message: []byte{}, + }, + wantErr: false, + }, + { + name: "ok/get-PKIOperation", + args: args{ + r: httptest.NewRequest(http.MethodGet, "http://scep:8080/?operation=PKIOperation&message=MTIzNA==", nil), + }, + want: request{ + Operation: "PKIOperation", + Message: []byte("1234"), + }, + wantErr: false, + }, + { + name: "ok/post-PKIOperation", + args: args{ + r: httptest.NewRequest(http.MethodPost, "http://scep:8080/?operation=PKIOperation", bytes.NewBufferString("1234")), + }, + want: request{ + Operation: "PKIOperation", + Message: []byte("1234"), + }, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := decodeRequest(tt.args.r) + if (err != nil) != tt.wantErr { + t.Errorf("decodeRequest() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("decodeRequest() = %v, want %v", got, tt.want) + } + }) + } +} From c695b23e24655eb838bc775aa674ee928fd7bb60 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 12 May 2022 16:33:32 +0200 Subject: [PATCH 4/4] Fix check for admin not belonging to policy --- authority/policy.go | 21 +++++--- authority/policy_test.go | 100 +++++++++++++++++++++++++++++++++----- authority/provisioners.go | 8 +-- 3 files changed, 104 insertions(+), 25 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: "fail/policy", + fields: fields{ + provisioners: provisioners, + admins: admins, + }, + args: args{ + provName: "jwkProv", + p: &linkedca.Policy{ + X509: &linkedca.X509Policy{ + Allow: &linkedca.X509Names{ + Dns: []string{"otherAdmin"}, // step not in policy + }, + }, + }, + }, + wantErr: true, + }, { name: "ok", 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{ @@ -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 }