From d981b9e0dc5cf1d430a4c5db3f1c8a8e0d05c265 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 14 Oct 2022 16:01:18 +0200 Subject: [PATCH] Add `--admin-subject` flag to `ca init` The first super admin subject can now be provided through the `--admin-subject` flag when initializing a CA. It's not yet possible to configure the subject of the first super admin when provisioners are migrated from `ca.json` to the database. This effectively limits usage of the flag to scenarios in which the provisioners are written to the database immediately, so when `--remote-management` is enabled. It currently also doesn't work with Helm deployments, because there's no mechanism yet to pass this type of option to the Helm chart. This commit partially addresses https://github.com/smallstep/cli/issues/697 --- authority/authority.go | 8 ++ pki/helm_test.go | 225 ++++++++++++++++++----------------------- pki/pki.go | 45 ++++++--- 3 files changed, 141 insertions(+), 137 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index e3bc3473..ae8b9a56 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -663,6 +663,14 @@ func (a *Authority) init() error { } // Create first super admin, belonging to the first JWK provisioner + // TODO(hs): pass a user-provided first super admin subject to here. With `ca init` it's + // added to the DB immediately if using remote management. But when migrating from + // ca.json to the DB, this option doesn't exist. Adding a flag just to do it during + // migration isn't nice. We could opt for a user to change it afterwards. There exist + // cases in which creation of `step` could lock out a user from API access. This is the + // case if `step` isn't allowed to be signed by Name Constraints or the X.509 policy. + // We have protection for that when creating and updating a policy, but if a policy or + // Name Constraints are in use at the time of migration, that could lock the user out. firstSuperAdminSubject := "step" if err := a.adminDB.CreateAdmin(ctx, &linkedca.Admin{ ProvisionerId: firstJWKProvisioner.Id, diff --git a/pki/helm_test.go b/pki/helm_test.go index 21d6d4db..ea1c4acd 100644 --- a/pki/helm_test.go +++ b/pki/helm_test.go @@ -21,148 +21,125 @@ import ( ) func TestPKI_WriteHelmTemplate(t *testing.T) { - type fields struct { - casOptions apiv1.Options - pkiOptions []Option + var preparePKI = func(t *testing.T, opts ...Option) *PKI { + o := apiv1.Options{ + Type: "softcas", + IsCreator: true, + } + + // Add default WithHelm option + opts = append(opts, WithHelm()) + + // TODO(hs): invoking `New` doesn't perform all operations that are executed + // when `ca init --helm` is executed. Ideally this logic should be handled + // in one place and probably inside of the PKI initialization. For testing + // purposes the missing operations to fill a Helm template fully are faked + // by `setKeyPair`, `setCertificates` and `setSSHSigningKeys` + p, err := New(o, opts...) + assert.NoError(t, err) + + // setKeyPair sets a predefined JWK and a default JWK provisioner. This is one + // of the things performed in the `ca init` code that's not part of `New`, but + // performed after that in p.GenerateKeyPairs`. We're currently using the same + // JWK for every test to keep test variance small: we're not testing JWK generation + // here after all. It's a bit dangerous to redefine the function here, but it's + // the simplest way to make this fully testable without refactoring the init now. + // The password for the predefined encrypted key is \x01\x03\x03\x07. + setKeyPair(t, p) + + // setCertificates sets some static intermediate and root CA certificate bytes. It + // replaces the logic executed in `p.GenerateRootCertificate`, `p.WriteRootCertificate`, + // and `p.GenerateIntermediateCertificate`. + setCertificates(t, p) + + // setSSHSigningKeys sets predefined SSH user and host certificate and key bytes. + // This replaces the logic in `p.GenerateSSHSigningKeys` + setSSHSigningKeys(t, p) + + return p } - tests := []struct { - name string - fields fields + type test struct { + pki *PKI testFile string wantErr bool - }{ - { - name: "ok/simple", - fields: fields{ - pkiOptions: []Option{ - WithHelm(), - }, - casOptions: apiv1.Options{ - Type: "softcas", - IsCreator: true, - }, - }, - testFile: "testdata/helm/simple.yml", - wantErr: false, + } + var tests = map[string]func(t *testing.T) test{ + "ok/simple": func(t *testing.T) test { + return test{ + pki: preparePKI(t), + testFile: "testdata/helm/simple.yml", + wantErr: false, + } }, - { - name: "ok/with-provisioner", - fields: fields{ - pkiOptions: []Option{ - WithHelm(), - WithProvisioner("a-provisioner"), - }, - casOptions: apiv1.Options{ - Type: "softcas", - IsCreator: true, - }, - }, - testFile: "testdata/helm/with-provisioner.yml", - wantErr: false, + "ok/with-provisioner": func(t *testing.T) test { + return test{ + pki: preparePKI(t, WithProvisioner("a-provisioner")), + testFile: "testdata/helm/with-provisioner.yml", + wantErr: false, + } }, - { - name: "ok/with-acme", - fields: fields{ - pkiOptions: []Option{ - WithHelm(), - WithACME(), - }, - casOptions: apiv1.Options{ - Type: "softcas", - IsCreator: true, - }, - }, - testFile: "testdata/helm/with-acme.yml", - wantErr: false, + "ok/with-acme": func(t *testing.T) test { + return test{ + pki: preparePKI(t, WithACME()), + testFile: "testdata/helm/with-acme.yml", + wantErr: false, + } }, - { - name: "ok/with-admin", - fields: fields{ - pkiOptions: []Option{ - WithHelm(), - WithAdmin(), - }, - casOptions: apiv1.Options{ - Type: "softcas", - IsCreator: true, - }, - }, - testFile: "testdata/helm/with-admin.yml", - wantErr: false, + "ok/with-admin": func(t *testing.T) test { + return test{ + pki: preparePKI(t, WithAdmin()), + testFile: "testdata/helm/with-admin.yml", + wantErr: false, + } }, - { - name: "ok/with-ssh", - fields: fields{ - pkiOptions: []Option{ - WithHelm(), - WithSSH(), - }, - casOptions: apiv1.Options{ - Type: "softcas", - IsCreator: true, - }, - }, - testFile: "testdata/helm/with-ssh.yml", - wantErr: false, + "ok/with-ssh": func(t *testing.T) test { + return test{ + pki: preparePKI(t, WithSSH()), + testFile: "testdata/helm/with-ssh.yml", + wantErr: false, + } }, - { - name: "ok/with-ssh-and-acme", - fields: fields{ - pkiOptions: []Option{ - WithHelm(), - WithACME(), - WithSSH(), - }, - casOptions: apiv1.Options{ - Type: "softcas", - IsCreator: true, + "ok/with-ssh-and-acme": func(t *testing.T) test { + return test{ + pki: preparePKI(t, WithSSH(), WithACME()), + testFile: "testdata/helm/with-ssh-and-acme.yml", + wantErr: false, + } + }, + "fail/authority.ProvisionerToCertificates": func(t *testing.T) test { + pki := preparePKI(t) + pki.Authority.Provisioners = append(pki.Authority.Provisioners, + &linkedca.Provisioner{ + Type: linkedca.Provisioner_JWK, + Name: "Broken JWK", + Details: nil, }, - }, - testFile: "testdata/helm/with-ssh-and-acme.yml", - wantErr: false, + ) + return test{ + pki: pki, + wantErr: true, + } }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - o := tt.fields.casOptions - opts := tt.fields.pkiOptions - - // TODO(hs): invoking `New` doesn't perform all operations that are executed - // when `ca init --helm` is executed. Ideally this logic should be handled - // in one place and probably inside of the PKI initialization. For testing - // purposes the missing operations to fill a Helm template fully are faked - // by `setKeyPair`, `setCertificates` and `setSSHSigningKeys` - p, err := New(o, opts...) - assert.NoError(t, err) - - // setKeyPair sets a predefined JWK and a default JWK provisioner. This is one - // of the things performed in the `ca init` code that's not part of `New`, but - // performed after that in p.GenerateKeyPairs`. We're currently using the same - // JWK for every test to keep test variance small: we're not testing JWK generation - // here after all. It's a bit dangerous to redefine the function here, but it's - // the simplest way to make this fully testable without refactoring the init now. - // The password for the predefined encrypted key is \x01\x03\x03\x07. - setKeyPair(t, p) - - // setCertificates sets some static intermediate and root CA certificate bytes. It - // replaces the logic executed in `p.GenerateRootCertificate`, `p.WriteRootCertificate`, - // and `p.GenerateIntermediateCertificate`. - setCertificates(t, p) - - // setSSHSigningKeys sets predefined SSH user and host certificate and key bytes. - // This replaces the logic in `p.GenerateSSHSigningKeys` - setSSHSigningKeys(t, p) + for name, run := range tests { + tc := run(t) + t.Run(name, func(t *testing.T) { w := &bytes.Buffer{} - if err := p.WriteHelmTemplate(w); (err != nil) != tt.wantErr { - t.Errorf("PKI.WriteHelmTemplate() error = %v, wantErr %v", err, tt.wantErr) + if err := tc.pki.WriteHelmTemplate(w); (err != nil) != tc.wantErr { + t.Errorf("PKI.WriteHelmTemplate() error = %v, wantErr %v", err, tc.wantErr) + return + } + + if tc.wantErr { + // don't compare output if an error was expected on output return } - wantBytes, err := os.ReadFile(tt.testFile) + wantBytes, err := os.ReadFile(tc.testFile) assert.NoError(t, err) if diff := cmp.Diff(wantBytes, w.Bytes()); diff != "" { - t.Logf("Generated Helm template did not match reference %q\n", tt.testFile) + t.Logf("Generated Helm template did not match reference %q\n", tc.testFile) t.Errorf("Diff follows:\n%s\n", diff) t.Errorf("Full output:\n%s\n", w.Bytes()) } diff --git a/pki/pki.go b/pki/pki.go index a4a64344..cf7c7d09 100644 --- a/pki/pki.go +++ b/pki/pki.go @@ -175,18 +175,19 @@ func GetProvisionerKey(caURL, rootFile, kid string) (string, error) { } type options struct { - provisioner string - pkiOnly bool - enableACME bool - enableSSH bool - enableAdmin bool - noDB bool - isHelm bool - deploymentType DeploymentType - rootKeyURI string - intermediateKeyURI string - hostKeyURI string - userKeyURI string + provisioner string + firstSuperAdminSubject string + pkiOnly bool + enableACME bool + enableSSH bool + enableAdmin bool + noDB bool + isHelm bool + deploymentType DeploymentType + rootKeyURI string + intermediateKeyURI string + hostKeyURI string + userKeyURI string } // Option is the type of a configuration option on the pki constructor. @@ -220,6 +221,15 @@ func WithProvisioner(s string) Option { } } +// WithFirstSuperAdminSubject defines the subject of the first +// super admin for use with the Admin API. The admin will belong +// to the first JWK provisioner. +func WithFirstSuperAdminSubject(s string) Option { + return func(p *PKI) { + p.options.firstSuperAdminSubject = s + } +} + // WithPKIOnly will only generate the PKI without the step-ca config files. func WithPKIOnly() Option { return func(p *PKI) { @@ -886,6 +896,11 @@ func (p *PKI) GenerateConfig(opt ...ConfigOption) (*authconfig.Config, error) { // // Note that we might want to be able to define the database as a // flag in `step ca init` so we can write to the proper place. + // + // TODO(hs): the logic for creating the provisioners and the super admin + // is similar to what's done when automatically migrating the provisioners. + // This is related to the existing comment above. Refactor this to exist in + // a single place and ensure it happensonly once. _db, err := db.New(cfg.DB) if err != nil { return nil, err @@ -909,9 +924,13 @@ func (p *PKI) GenerateConfig(opt ...ConfigOption) (*authconfig.Config, error) { } } // Add the first provisioner as an admin. + firstSuperAdminSubject := "step" + if p.options.firstSuperAdminSubject != "" { + firstSuperAdminSubject = p.options.firstSuperAdminSubject + } if err := adminDB.CreateAdmin(context.Background(), &linkedca.Admin{ AuthorityId: admin.DefaultAuthorityID, - Subject: "step", + Subject: firstSuperAdminSubject, Type: linkedca.Admin_SUPER_ADMIN, ProvisionerId: adminID, }); err != nil {