From b91affdd34ae30e6c6e3ef61ea2fa8ac30944aed Mon Sep 17 00:00:00 2001 From: max furman Date: Mon, 25 Apr 2022 10:23:07 -0700 Subject: [PATCH 01/25] exposing authority configuration for provisioner cli commands --- authority/admin/db.go | 46 +++++++++++++++++++++++++++++++++ authority/admins.go | 6 ++--- authority/authority.go | 26 ++++++++++++++++--- authority/provisioners.go | 8 +++--- ca/adminClient.go | 24 +++++++++--------- ca/client.go | 53 ++++++++++++++++++++------------------- 6 files changed, 115 insertions(+), 48 deletions(-) diff --git a/authority/admin/db.go b/authority/admin/db.go index bf34a3c2..6e4e7c49 100644 --- a/authority/admin/db.go +++ b/authority/admin/db.go @@ -71,6 +71,52 @@ type DB interface { DeleteAdmin(ctx context.Context, id string) error } +type NoDB struct{} + +func NewNoDB() *NoDB { + return &NoDB{} +} + +func (n *NoDB) CreateProvisioner(ctx context.Context, prov *linkedca.Provisioner) error { + return nil +} + +func (n *NoDB) GetProvisioner(ctx context.Context, id string) (*linkedca.Provisioner, error) { + return nil, nil +} + +func (n *NoDB) GetProvisioners(ctx context.Context) ([]*linkedca.Provisioner, error) { + return nil, nil +} + +func (n *NoDB) UpdateProvisioner(ctx context.Context, prov *linkedca.Provisioner) error { + return nil +} + +func (n *NoDB) DeleteProvisioner(ctx context.Context, id string) error { + return nil +} + +func (n *NoDB) CreateAdmin(ctx context.Context, admin *linkedca.Admin) error { + return nil +} + +func (n *NoDB) GetAdmin(ctx context.Context, id string) (*linkedca.Admin, error) { + return nil, nil +} + +func (n *NoDB) GetAdmins(ctx context.Context) ([]*linkedca.Admin, error) { + return nil, nil +} + +func (n *NoDB) UpdateAdmin(ctx context.Context, prov *linkedca.Admin) error { + return nil +} + +func (n *NoDB) DeleteAdmin(ctx context.Context, id string) error { + return nil +} + // MockDB is an implementation of the DB interface that should only be used as // a mock in tests. type MockDB struct { diff --git a/authority/admins.go b/authority/admins.go index b975297a..c8e1ac66 100644 --- a/authority/admins.go +++ b/authority/admins.go @@ -49,7 +49,7 @@ func (a *Authority) StoreAdmin(ctx context.Context, adm *linkedca.Admin, prov pr return admin.WrapErrorISE(err, "error creating admin") } if err := a.admins.Store(adm, prov); err != nil { - if err := a.reloadAdminResources(ctx); err != nil { + if err := a.ReloadAdminResources(ctx); err != nil { return admin.WrapErrorISE(err, "error reloading admin resources on failed admin store") } return admin.WrapErrorISE(err, "error storing admin in authority cache") @@ -66,7 +66,7 @@ func (a *Authority) UpdateAdmin(ctx context.Context, id string, nu *linkedca.Adm return nil, admin.WrapErrorISE(err, "error updating cached admin %s", id) } if err := a.adminDB.UpdateAdmin(ctx, adm); err != nil { - if err := a.reloadAdminResources(ctx); err != nil { + if err := a.ReloadAdminResources(ctx); err != nil { return nil, admin.WrapErrorISE(err, "error reloading admin resources on failed admin update") } return nil, admin.WrapErrorISE(err, "error updating admin %s", id) @@ -88,7 +88,7 @@ func (a *Authority) removeAdmin(ctx context.Context, id string) error { return admin.WrapErrorISE(err, "error removing admin %s from authority cache", id) } if err := a.adminDB.DeleteAdmin(ctx, id); err != nil { - if err := a.reloadAdminResources(ctx); err != nil { + if err := a.ReloadAdminResources(ctx); err != nil { return admin.WrapErrorISE(err, "error reloading admin resources on failed admin remove") } return admin.WrapErrorISE(err, "error deleting admin %s", id) diff --git a/authority/authority.go b/authority/authority.go index 9db38e14..2c10b626 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -115,6 +115,20 @@ func New(cfg *config.Config, opts ...Option) (*Authority, error) { return a, nil } +// FromOptions creates an Authority exclusively using the passed in options +// and does not intialize the Authority. +func FromOptions(opts ...Option) (*Authority, error) { + var a = new(Authority) + + // Apply options. + for _, fn := range opts { + if err := fn(a); err != nil { + return nil, err + } + } + return a, nil +} + // NewEmbedded initializes an authority that can be embedded in a different // project without the limitations of the config. func NewEmbedded(opts ...Option) (*Authority, error) { @@ -153,8 +167,8 @@ func NewEmbedded(opts ...Option) (*Authority, error) { return a, nil } -// reloadAdminResources reloads admins and provisioners from the DB. -func (a *Authority) reloadAdminResources(ctx context.Context) error { +// ReloadAdminResources reloads admins and provisioners from the DB. +func (a *Authority) ReloadAdminResources(ctx context.Context) error { var ( provList provisioner.List adminList []*linkedca.Admin @@ -551,7 +565,7 @@ func (a *Authority) init() error { } // Load Provisioners and Admins - if err := a.reloadAdminResources(context.Background()); err != nil { + if err := a.ReloadAdminResources(context.Background()); err != nil { return err } @@ -587,6 +601,12 @@ func (a *Authority) GetAdminDatabase() admin.DB { return a.adminDB } +// GetConfig returns the config. +func (a *Authority) GetConfig() *config.Config { + return a.config +} + +// GetInfo returns information about the authority. func (a *Authority) GetInfo() Info { ai := Info{ StartTime: a.startTime, diff --git a/authority/provisioners.go b/authority/provisioners.go index 63fb630b..5944f007 100644 --- a/authority/provisioners.go +++ b/authority/provisioners.go @@ -145,7 +145,7 @@ func (a *Authority) generateProvisionerConfig(ctx context.Context) (provisioner. } -// StoreProvisioner stores an provisioner.Interface to the authority. +// StoreProvisioner stores a provisioner to the authority. func (a *Authority) StoreProvisioner(ctx context.Context, prov *linkedca.Provisioner) error { a.adminMutex.Lock() defer a.adminMutex.Unlock() @@ -191,7 +191,7 @@ func (a *Authority) StoreProvisioner(ctx context.Context, prov *linkedca.Provisi } if err := a.provisioners.Store(certProv); err != nil { - if err := a.reloadAdminResources(ctx); err != nil { + if err := a.ReloadAdminResources(ctx); err != nil { return admin.WrapErrorISE(err, "error reloading admin resources on failed provisioner store") } return admin.WrapErrorISE(err, "error storing provisioner in authority cache") @@ -223,7 +223,7 @@ func (a *Authority) UpdateProvisioner(ctx context.Context, nu *linkedca.Provisio return admin.WrapErrorISE(err, "error updating provisioner '%s' in authority cache", nu.Name) } if err := a.adminDB.UpdateProvisioner(ctx, nu); err != nil { - if err := a.reloadAdminResources(ctx); err != nil { + if err := a.ReloadAdminResources(ctx); err != nil { return admin.WrapErrorISE(err, "error reloading admin resources on failed provisioner update") } return admin.WrapErrorISE(err, "error updating provisioner '%s'", nu.Name) @@ -267,7 +267,7 @@ func (a *Authority) RemoveProvisioner(ctx context.Context, id string) error { } // Remove provisioner from database. if err := a.adminDB.DeleteProvisioner(ctx, provID); err != nil { - if err := a.reloadAdminResources(ctx); err != nil { + if err := a.ReloadAdminResources(ctx); err != nil { return admin.WrapErrorISE(err, "error reloading admin resources on failed provisioner remove") } return admin.WrapErrorISE(err, "error deleting provisioner %s", provName) diff --git a/ca/adminClient.go b/ca/adminClient.go index 72f62dd8..e898a898 100644 --- a/ca/adminClient.go +++ b/ca/adminClient.go @@ -363,19 +363,19 @@ retry: // GetProvisioner performs the GET /admin/provisioners/{name} request to the CA. func (c *AdminClient) GetProvisioner(opts ...ProvisionerOption) (*linkedca.Provisioner, error) { var retried bool - o := new(provisionerOptions) - if err := o.apply(opts); err != nil { + o := new(ProvisionerOptions) + if err := o.Apply(opts); err != nil { return nil, err } var u *url.URL switch { - case len(o.id) > 0: + case len(o.ID) > 0: u = c.endpoint.ResolveReference(&url.URL{ Path: "/admin/provisioners/id", RawQuery: o.rawQuery(), }) - case len(o.name) > 0: - u = c.endpoint.ResolveReference(&url.URL{Path: path.Join(adminURLPrefix, "provisioners", o.name)}) + case len(o.Name) > 0: + u = c.endpoint.ResolveReference(&url.URL{Path: path.Join(adminURLPrefix, "provisioners", o.Name)}) default: return nil, errors.New("must set either name or id in method options") } @@ -410,8 +410,8 @@ retry: // GetProvisionersPaginate performs the GET /admin/provisioners request to the CA. func (c *AdminClient) GetProvisionersPaginate(opts ...ProvisionerOption) (*adminAPI.GetProvisionersResponse, error) { var retried bool - o := new(provisionerOptions) - if err := o.apply(opts); err != nil { + o := new(ProvisionerOptions) + if err := o.Apply(opts); err != nil { return nil, err } u := c.endpoint.ResolveReference(&url.URL{ @@ -472,19 +472,19 @@ func (c *AdminClient) RemoveProvisioner(opts ...ProvisionerOption) error { retried bool ) - o := new(provisionerOptions) - if err := o.apply(opts); err != nil { + o := new(ProvisionerOptions) + if err := o.Apply(opts); err != nil { return err } switch { - case len(o.id) > 0: + case len(o.ID) > 0: u = c.endpoint.ResolveReference(&url.URL{ Path: path.Join(adminURLPrefix, "provisioners/id"), RawQuery: o.rawQuery(), }) - case len(o.name) > 0: - u = c.endpoint.ResolveReference(&url.URL{Path: path.Join(adminURLPrefix, "provisioners", o.name)}) + case len(o.Name) > 0: + u = c.endpoint.ResolveReference(&url.URL{Path: path.Join(adminURLPrefix, "provisioners", o.Name)}) default: return errors.New("must set either name or id in method options") } diff --git a/ca/client.go b/ca/client.go index 0bd93195..3871c749 100644 --- a/ca/client.go +++ b/ca/client.go @@ -425,16 +425,17 @@ func parseEndpoint(endpoint string) (*url.URL, error) { } // ProvisionerOption is the type of options passed to the Provisioner method. -type ProvisionerOption func(o *provisionerOptions) error +type ProvisionerOption func(o *ProvisionerOptions) error -type provisionerOptions struct { - cursor string - limit int - id string - name string +// ProvisionerOptions stores options for the provisioner CRUD API. +type ProvisionerOptions struct { + Cursor string + Limit int + ID string + Name string } -func (o *provisionerOptions) apply(opts []ProvisionerOption) (err error) { +func (o *ProvisionerOptions) Apply(opts []ProvisionerOption) (err error) { for _, fn := range opts { if err = fn(o); err != nil { return @@ -443,51 +444,51 @@ func (o *provisionerOptions) apply(opts []ProvisionerOption) (err error) { return } -func (o *provisionerOptions) rawQuery() string { +func (o *ProvisionerOptions) rawQuery() string { v := url.Values{} - if len(o.cursor) > 0 { - v.Set("cursor", o.cursor) + if len(o.Cursor) > 0 { + v.Set("cursor", o.Cursor) } - if o.limit > 0 { - v.Set("limit", strconv.Itoa(o.limit)) + if o.Limit > 0 { + v.Set("limit", strconv.Itoa(o.Limit)) } - if len(o.id) > 0 { - v.Set("id", o.id) + if len(o.ID) > 0 { + v.Set("id", o.ID) } - if len(o.name) > 0 { - v.Set("name", o.name) + if len(o.Name) > 0 { + v.Set("name", o.Name) } return v.Encode() } // WithProvisionerCursor will request the provisioners starting with the given cursor. func WithProvisionerCursor(cursor string) ProvisionerOption { - return func(o *provisionerOptions) error { - o.cursor = cursor + return func(o *ProvisionerOptions) error { + o.Cursor = cursor return nil } } // WithProvisionerLimit will request the given number of provisioners. func WithProvisionerLimit(limit int) ProvisionerOption { - return func(o *provisionerOptions) error { - o.limit = limit + return func(o *ProvisionerOptions) error { + o.Limit = limit return nil } } // WithProvisionerID will request the given provisioner. func WithProvisionerID(id string) ProvisionerOption { - return func(o *provisionerOptions) error { - o.id = id + return func(o *ProvisionerOptions) error { + o.ID = id return nil } } // WithProvisionerName will request the given provisioner. func WithProvisionerName(name string) ProvisionerOption { - return func(o *provisionerOptions) error { - o.name = name + return func(o *ProvisionerOptions) error { + o.Name = name return nil } } @@ -810,8 +811,8 @@ retry: // paginate the provisioners. func (c *Client) Provisioners(opts ...ProvisionerOption) (*api.ProvisionersResponse, error) { var retried bool - o := new(provisionerOptions) - if err := o.apply(opts); err != nil { + o := new(ProvisionerOptions) + if err := o.Apply(opts); err != nil { return nil, err } u := c.endpoint.ResolveReference(&url.URL{ From 4cb74e7d8ba5a70b2b5c6c00f3907ad0899524fe Mon Sep 17 00:00:00 2001 From: max furman Date: Sat, 30 Apr 2022 13:08:28 -0700 Subject: [PATCH 02/25] fix linter warnings --- authority/authority.go | 2 +- docs/GETTING_STARTED.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index 2c10b626..63375351 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -116,7 +116,7 @@ func New(cfg *config.Config, opts ...Option) (*Authority, error) { } // FromOptions creates an Authority exclusively using the passed in options -// and does not intialize the Authority. +// and does not initialize the Authority. func FromOptions(opts ...Option) (*Authority, error) { var a = new(Authority) diff --git a/docs/GETTING_STARTED.md b/docs/GETTING_STARTED.md index 84e968ab..67c5673d 100644 --- a/docs/GETTING_STARTED.md +++ b/docs/GETTING_STARTED.md @@ -654,7 +654,7 @@ preferably not all - meaning it never leaves the server on which it was created. ### Passwords -When you intialize your PKI (`step ca init`) the root and intermediate +When you initialize your PKI (`step ca init`) the root and intermediate private keys will be encrypted with the same password. We recommend that you change the password with which the intermediate is encrypted at your earliest convenience. @@ -681,7 +681,7 @@ to divide the root private key password across a handful of trusted parties. ### Provisioners -When you intialize your PKI (`step ca init`) a default provisioner will be created +When you initialize your PKI (`step ca init`) a default provisioner will be created and it's private key will be encrypted using the same password used to encrypt the root private key. Before deploying the Step CA you should remove this provisioner and add new ones that are encrypted with new, secure, random passwords. From 7030dbb7a1c5badb204d4af614b890e2a590809e Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 11 May 2022 21:18:47 +0200 Subject: [PATCH 03/25] Use github.com/smallstep/pkcs7 fork with patches applied --- go.mod | 3 +++ go.sum | 7 ++----- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 36e39b4d..b55b7266 100644 --- a/go.mod +++ b/go.mod @@ -64,3 +64,6 @@ require ( // replace go.step.sm/crypto => ../crypto // replace go.step.sm/cli-utils => ../cli-utils // replace go.step.sm/linkedca => ../linkedca + +// use github.com/smallstep/pkcs7 fork with patches applied +replace go.mozilla.org/pkcs7 => github.com/smallstep/pkcs7 v0.0.0-20211016004704-52592125d6f6 diff --git a/go.sum b/go.sum index 55528b4f..0758ad69 100644 --- a/go.sum +++ b/go.sum @@ -735,6 +735,8 @@ github.com/smallstep/assert v0.0.0-20200723003110-82e2b9b3b262 h1:unQFBIznI+VYD1 github.com/smallstep/assert v0.0.0-20200723003110-82e2b9b3b262/go.mod h1:MyOHs9Po2fbM1LHej6sBUT8ozbxmMOFG+E+rx/GSGuc= github.com/smallstep/nosql v0.4.0 h1:Go3WYwttUuvwqMtFiiU4g7kBIlY+hR0bIZAqVdakQ3M= github.com/smallstep/nosql v0.4.0/go.mod h1:yKZT5h7cdIVm6wEKM9+jN5dgK80Hljpuy8HNsnI7Gzo= +github.com/smallstep/pkcs7 v0.0.0-20211016004704-52592125d6f6 h1:8Rjy6IZbSM/jcYgBWCoLIGjug7QcoLtF9sUuhDrHD2U= +github.com/smallstep/pkcs7 v0.0.0-20211016004704-52592125d6f6/go.mod h1:SNgMg+EgDFwmvSmLRTNKC5fegJjB7v23qTQ0XLGUNHk= github.com/smartystreets/assertions v0.0.0-20180927180507-b2de0cb4f26d/go.mod h1:OnSkiWE9lh6wB0YB77sQom3nweQdgAjqCqsofrRNTgc= github.com/smartystreets/goconvey v1.6.4/go.mod h1:syvi0/a8iFYH4r/RixwvyeAJjdLS9QV7WQ/tjFTllLA= github.com/soheilhy/cmux v0.1.4/go.mod h1:IM3LyeVVIOuxMH7sFAkER9+bJ4dT7Ms6E4xg4kGIyLM= @@ -796,9 +798,6 @@ go.etcd.io/bbolt v1.3.5/go.mod h1:G5EMThwa9y8QZGBClrRx5EY+Yw9kAhnjy3bSjsnlVTQ= go.etcd.io/bbolt v1.3.6 h1:/ecaJf0sk1l4l6V4awd65v2C3ILy7MSj+s/x1ADCIMU= go.etcd.io/bbolt v1.3.6/go.mod h1:qXsaaIqmgQH0T+OPdb99Bf+PKfBBQVAdyD6TY9G8XM4= go.etcd.io/etcd v0.0.0-20191023171146-3cf2f69b5738/go.mod h1:dnLIgRNXwCJa5e+c6mIZCrds/GIG4ncV9HhK5PX7jPg= -go.mozilla.org/pkcs7 v0.0.0-20210730143726-725912489c62/go.mod h1:SNgMg+EgDFwmvSmLRTNKC5fegJjB7v23qTQ0XLGUNHk= -go.mozilla.org/pkcs7 v0.0.0-20210826202110-33d05740a352 h1:CCriYyAfq1Br1aIYettdHZTy8mBTIPo7We18TuO/bak= -go.mozilla.org/pkcs7 v0.0.0-20210826202110-33d05740a352/go.mod h1:SNgMg+EgDFwmvSmLRTNKC5fegJjB7v23qTQ0XLGUNHk= go.opencensus.io v0.20.1/go.mod h1:6WKK9ahsWS3RSO+PY9ZHZUfv2irvY6gN279GOPZjmmk= go.opencensus.io v0.20.2/go.mod h1:6WKK9ahsWS3RSO+PY9ZHZUfv2irvY6gN279GOPZjmmk= go.opencensus.io v0.21.0/go.mod h1:mSImk1erAIZhrmZN+AvHh14ztQfjbGwt4TtuofqLduU= @@ -815,8 +814,6 @@ go.step.sm/cli-utils v0.7.0/go.mod h1:Ur6bqA/yl636kCUJbp30J7Unv5JJ226eW2KqXPDwF/ go.step.sm/crypto v0.9.0/go.mod h1:+CYG05Mek1YDqi5WK0ERc6cOpKly2i/a5aZmU1sfGj0= go.step.sm/crypto v0.16.1 h1:4mnZk21cSxyMGxsEpJwZKKvJvDu1PN09UVrWWFNUBdk= go.step.sm/crypto v0.16.1/go.mod h1:3G0yQr5lQqfEG0CMYz8apC/qMtjLRQlzflL2AxkcN+g= -go.step.sm/linkedca v0.16.0 h1:9xdE150lRTEoBq1gJl+prePpSmNqXRXsez3qzRs3Lic= -go.step.sm/linkedca v0.16.0/go.mod h1:W59ucS4vFpuR0g4PtkGbbtXAwxbDEnNCg+ovkej1ANM= go.step.sm/linkedca v0.16.1 h1:CdbMV5SjnlRsgeYTXaaZmQCkYIgJq8BOzpewri57M2k= go.step.sm/linkedca v0.16.1/go.mod h1:W59ucS4vFpuR0g4PtkGbbtXAwxbDEnNCg+ovkej1ANM= go.uber.org/atomic v1.3.2/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= From 25b8d196d84542c97a946a963782a4d20c3c9ca9 Mon Sep 17 00:00:00 2001 From: max furman Date: Wed, 11 May 2022 17:04:43 -0700 Subject: [PATCH 04/25] Couple changes in response to PR - add skipInit option to skip authority initialization - check admin API status when removing provisioners - no need to check admins when not using Admin API --- authority/authority.go | 32 +++++++++++++------------------- authority/options.go | 9 +++++++++ authority/provisioners.go | 30 ++++++++++++++++-------------- 3 files changed, 38 insertions(+), 33 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index 63375351..5b08ec40 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -78,8 +78,12 @@ type Authority struct { authorizeSSHRenewFunc provisioner.AuthorizeSSHRenewFunc adminMutex sync.RWMutex + + // Do Not initialize the authority + skipInit bool } +// Info contains information about the authority. type Info struct { StartTime time.Time RootX509Certs []*x509.Certificate @@ -107,25 +111,13 @@ func New(cfg *config.Config, opts ...Option) (*Authority, error) { } } - // Initialize authority from options or configuration. - if err := a.init(); err != nil { - return nil, err - } - - return a, nil -} - -// FromOptions creates an Authority exclusively using the passed in options -// and does not initialize the Authority. -func FromOptions(opts ...Option) (*Authority, error) { - var a = new(Authority) - - // Apply options. - for _, fn := range opts { - if err := fn(a); err != nil { + if !a.skipInit { + // Initialize authority from options or configuration. + if err := a.init(); err != nil { return nil, err } } + return a, nil } @@ -159,9 +151,11 @@ func NewEmbedded(opts ...Option) (*Authority, error) { // Initialize config required fields. a.config.Init() - // Initialize authority from options or configuration. - if err := a.init(); err != nil { - return nil, err + if !a.skipInit { + // Initialize authority from options or configuration. + if err := a.init(); err != nil { + return nil, err + } } return a, nil diff --git a/authority/options.go b/authority/options.go index 1c154577..b583bb89 100644 --- a/authority/options.go +++ b/authority/options.go @@ -284,6 +284,15 @@ func WithX509Enforcers(ces ...provisioner.CertificateEnforcer) Option { } } +// WithSkipInit is an option that allows the constructor to skip initializtion +// of the authority. +func WithSkipInit() Option { + return func(a *Authority) error { + a.skipInit = true + return nil + } +} + func readCertificateBundle(pemCerts []byte) ([]*x509.Certificate, error) { var block *pem.Block var certs []*x509.Certificate diff --git a/authority/provisioners.go b/authority/provisioners.go index 5944f007..642bb5b1 100644 --- a/authority/provisioners.go +++ b/authority/provisioners.go @@ -243,27 +243,29 @@ func (a *Authority) RemoveProvisioner(ctx context.Context, id string) error { } provName, provID := p.GetName(), p.GetID() - // Validate - // - Check that there will be SUPER_ADMINs that remain after we - // remove this provisioner. - if a.admins.SuperCount() == a.admins.SuperCountByProvisioner(provName) { - return admin.NewError(admin.ErrorBadRequestType, - "cannot remove provisioner %s because no super admins will remain", provName) - } + if a.IsAdminAPIEnabled() { + // Validate + // - Check that there will be SUPER_ADMINs that remain after we + // remove this provisioner. + if a.IsAdminAPIEnabled() && a.admins.SuperCount() == a.admins.SuperCountByProvisioner(provName) { + return admin.NewError(admin.ErrorBadRequestType, + "cannot remove provisioner %s because no super admins will remain", provName) + } - // Delete all admins associated with the provisioner. - admins, ok := a.admins.LoadByProvisioner(provName) - if ok { - for _, adm := range admins { - if err := a.removeAdmin(ctx, adm.Id); err != nil { - return admin.WrapErrorISE(err, "error deleting admin %s, as part of provisioner %s deletion", adm.Subject, provName) + // Delete all admins associated with the provisioner. + admins, ok := a.admins.LoadByProvisioner(provName) + if ok { + for _, adm := range admins { + if err := a.removeAdmin(ctx, adm.Id); err != nil { + return admin.WrapErrorISE(err, "error deleting admin %s, as part of provisioner %s deletion", adm.Subject, provName) + } } } } // Remove provisioner from authority caches. if err := a.provisioners.Remove(provID); err != nil { - return admin.WrapErrorISE(err, "error removing admin from authority cache") + return admin.WrapErrorISE(err, "error removing provisioner from authority cache") } // Remove provisioner from database. if err := a.adminDB.DeleteProvisioner(ctx, provID); err != nil { From b75ce3acbdfb61a74ce85adfad5db2f3dcc355b5 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Tue, 17 May 2022 23:39:01 +0200 Subject: [PATCH 05/25] Update to go.step.sm/crypto v0.16.2 This patch release of go.step.sm/crypto fixes an issue with not all `Subject` names being available for usage in a template as `ExtraNames`. --- go.mod | 2 +- go.sum | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 36e39b4d..8d0f935d 100644 --- a/go.mod +++ b/go.mod @@ -46,7 +46,7 @@ require ( go.etcd.io/bbolt v1.3.6 // indirect go.mozilla.org/pkcs7 v0.0.0-20210826202110-33d05740a352 go.step.sm/cli-utils v0.7.0 - go.step.sm/crypto v0.16.1 + go.step.sm/crypto v0.16.2 go.step.sm/linkedca v0.16.1 golang.org/x/crypto v0.0.0-20211215153901-e495a2d5b3d3 golang.org/x/net v0.0.0-20220403103023-749bd193bc2b diff --git a/go.sum b/go.sum index 55528b4f..cad29555 100644 --- a/go.sum +++ b/go.sum @@ -813,10 +813,8 @@ go.opentelemetry.io/proto/otlp v0.7.0/go.mod h1:PqfVotwruBrMGOCsRd/89rSnXhoiJIqe go.step.sm/cli-utils v0.7.0 h1:2GvY5Muid1yzp7YQbfCCS+gK3q7zlHjjLL5Z0DXz8ds= go.step.sm/cli-utils v0.7.0/go.mod h1:Ur6bqA/yl636kCUJbp30J7Unv5JJ226eW2KqXPDwF/E= go.step.sm/crypto v0.9.0/go.mod h1:+CYG05Mek1YDqi5WK0ERc6cOpKly2i/a5aZmU1sfGj0= -go.step.sm/crypto v0.16.1 h1:4mnZk21cSxyMGxsEpJwZKKvJvDu1PN09UVrWWFNUBdk= -go.step.sm/crypto v0.16.1/go.mod h1:3G0yQr5lQqfEG0CMYz8apC/qMtjLRQlzflL2AxkcN+g= -go.step.sm/linkedca v0.16.0 h1:9xdE150lRTEoBq1gJl+prePpSmNqXRXsez3qzRs3Lic= -go.step.sm/linkedca v0.16.0/go.mod h1:W59ucS4vFpuR0g4PtkGbbtXAwxbDEnNCg+ovkej1ANM= +go.step.sm/crypto v0.16.2 h1:Pr9aazTwWBBZNogUsOqhOrPSdwAa9pPs+lMB602lnDA= +go.step.sm/crypto v0.16.2/go.mod h1:1WkTOTY+fOX/RY4TnZREp6trQAsBHRQ7nu6QJBiNQF8= go.step.sm/linkedca v0.16.1 h1:CdbMV5SjnlRsgeYTXaaZmQCkYIgJq8BOzpewri57M2k= go.step.sm/linkedca v0.16.1/go.mod h1:W59ucS4vFpuR0g4PtkGbbtXAwxbDEnNCg+ovkej1ANM= go.uber.org/atomic v1.3.2/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= From bfb406bf703d716d98dc86c169acb92d49cb4cf4 Mon Sep 17 00:00:00 2001 From: max furman Date: Wed, 18 May 2022 09:43:32 -0700 Subject: [PATCH 06/25] Fixes for PR review --- authority/admin/db.go | 46 ------------------------------------------- authority/options.go | 8 ++++++++ ca/adminClient.go | 8 ++++---- ca/client.go | 7 ++++--- 4 files changed, 16 insertions(+), 53 deletions(-) diff --git a/authority/admin/db.go b/authority/admin/db.go index 6e4e7c49..bf34a3c2 100644 --- a/authority/admin/db.go +++ b/authority/admin/db.go @@ -71,52 +71,6 @@ type DB interface { DeleteAdmin(ctx context.Context, id string) error } -type NoDB struct{} - -func NewNoDB() *NoDB { - return &NoDB{} -} - -func (n *NoDB) CreateProvisioner(ctx context.Context, prov *linkedca.Provisioner) error { - return nil -} - -func (n *NoDB) GetProvisioner(ctx context.Context, id string) (*linkedca.Provisioner, error) { - return nil, nil -} - -func (n *NoDB) GetProvisioners(ctx context.Context) ([]*linkedca.Provisioner, error) { - return nil, nil -} - -func (n *NoDB) UpdateProvisioner(ctx context.Context, prov *linkedca.Provisioner) error { - return nil -} - -func (n *NoDB) DeleteProvisioner(ctx context.Context, id string) error { - return nil -} - -func (n *NoDB) CreateAdmin(ctx context.Context, admin *linkedca.Admin) error { - return nil -} - -func (n *NoDB) GetAdmin(ctx context.Context, id string) (*linkedca.Admin, error) { - return nil, nil -} - -func (n *NoDB) GetAdmins(ctx context.Context) ([]*linkedca.Admin, error) { - return nil, nil -} - -func (n *NoDB) UpdateAdmin(ctx context.Context, prov *linkedca.Admin) error { - return nil -} - -func (n *NoDB) DeleteAdmin(ctx context.Context, id string) error { - return nil -} - // MockDB is an implementation of the DB interface that should only be used as // a mock in tests. type MockDB struct { diff --git a/authority/options.go b/authority/options.go index b583bb89..755e0fbc 100644 --- a/authority/options.go +++ b/authority/options.go @@ -266,6 +266,14 @@ func WithAdminDB(d admin.DB) Option { } } +// WithProvisioners is an option to set the provisioner collection. +func WithProvisioners(ps *provisioner.Collection) Option { + return func(a *Authority) error { + a.provisioners = ps + return nil + } +} + // WithLinkedCAToken is an option to set the authentication token used to enable // linked ca. func WithLinkedCAToken(token string) Option { diff --git a/ca/adminClient.go b/ca/adminClient.go index e898a898..90b0ab1d 100644 --- a/ca/adminClient.go +++ b/ca/adminClient.go @@ -369,12 +369,12 @@ func (c *AdminClient) GetProvisioner(opts ...ProvisionerOption) (*linkedca.Provi } var u *url.URL switch { - case len(o.ID) > 0: + case o.ID != "": u = c.endpoint.ResolveReference(&url.URL{ Path: "/admin/provisioners/id", RawQuery: o.rawQuery(), }) - case len(o.Name) > 0: + case o.Name != "": u = c.endpoint.ResolveReference(&url.URL{Path: path.Join(adminURLPrefix, "provisioners", o.Name)}) default: return nil, errors.New("must set either name or id in method options") @@ -478,12 +478,12 @@ func (c *AdminClient) RemoveProvisioner(opts ...ProvisionerOption) error { } switch { - case len(o.ID) > 0: + case o.ID != "": u = c.endpoint.ResolveReference(&url.URL{ Path: path.Join(adminURLPrefix, "provisioners/id"), RawQuery: o.rawQuery(), }) - case len(o.Name) > 0: + case o.Name != "": u = c.endpoint.ResolveReference(&url.URL{Path: path.Join(adminURLPrefix, "provisioners", o.Name)}) default: return errors.New("must set either name or id in method options") diff --git a/ca/client.go b/ca/client.go index 3871c749..44961357 100644 --- a/ca/client.go +++ b/ca/client.go @@ -435,6 +435,7 @@ type ProvisionerOptions struct { Name string } +// Apply caches provisioner options on a struct for later use. func (o *ProvisionerOptions) Apply(opts []ProvisionerOption) (err error) { for _, fn := range opts { if err = fn(o); err != nil { @@ -446,16 +447,16 @@ func (o *ProvisionerOptions) Apply(opts []ProvisionerOption) (err error) { func (o *ProvisionerOptions) rawQuery() string { v := url.Values{} - if len(o.Cursor) > 0 { + if o.Cursor != "" { v.Set("cursor", o.Cursor) } if o.Limit > 0 { v.Set("limit", strconv.Itoa(o.Limit)) } - if len(o.ID) > 0 { + if o.ID != "" { v.Set("id", o.ID) } - if len(o.Name) > 0 { + if o.Name != "" { v.Set("name", o.Name) } return v.Encode() From fff00aca785958596dec61691bcc06317218f33c Mon Sep 17 00:00:00 2001 From: max furman Date: Wed, 18 May 2022 15:56:40 -0700 Subject: [PATCH 07/25] Updates to issue templates --- .github/ISSUE_TEMPLATE/bug-report.yml | 56 +++++++++++++++++++ .github/ISSUE_TEMPLATE/bug_report.md | 27 --------- .../ISSUE_TEMPLATE/documentation-request.md | 12 +++- .github/ISSUE_TEMPLATE/enhancement.md | 17 +++++- 4 files changed, 80 insertions(+), 32 deletions(-) create mode 100644 .github/ISSUE_TEMPLATE/bug-report.yml delete mode 100644 .github/ISSUE_TEMPLATE/bug_report.md diff --git a/.github/ISSUE_TEMPLATE/bug-report.yml b/.github/ISSUE_TEMPLATE/bug-report.yml new file mode 100644 index 00000000..64b4d103 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/bug-report.yml @@ -0,0 +1,56 @@ +name: Bug Report +description: File a bug report +title: "[Bug]: " +labels: ["bug", "needs triage"] +body: + - type: markdown + attributes: + value: | + Thanks for taking the time to fill out this bug report! + - type: textarea + id: steps + attributes: + label: Steps to Reproduce + description: Tell us how to reproduce this issue. + placeholder: These are the steps! + validations: + required: true + - type: textarea + id: your-env + attributes: + label: Your Environment + value: |- + * OS - + * `step-ca` Version - + validations: + required: true + - type: textarea + id: expected-behavior + attributes: + label: Expected Behavior + description: What did you expect to happen? + validations: + required: true + - type: textarea + id: actual-behavior + attributes: + label: Actual Behavior + description: What happens instead? + validations: + required: true + - type: textarea + id: context + attributes: + label: Additional Context + description: Add any other context about the problem here. + validations: + required: false + - type: textarea + id: contributing + attributes: + label: Contributing + value: | + Vote on this issue by adding a 👍 reaction. + To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already). + validations: + required: false diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md deleted file mode 100644 index 066c4951..00000000 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ /dev/null @@ -1,27 +0,0 @@ ---- -name: Bug report -about: Create a report to help us improve -title: '' -labels: bug, needs triage -assignees: '' - ---- - -### Subject of the issue -Describe your issue here. - -### Your environment -* OS - -* Version - - -### Steps to reproduce -Tell us how to reproduce this issue. Please provide a working demo, you can use [this template](https://plnkr.co/edit/XorWgI?p=preview) as a base. - -### Expected behaviour -Tell us what should happen - -### Actual behaviour -Tell us what happens instead - -### Additional context -Add any other context about the problem here. diff --git a/.github/ISSUE_TEMPLATE/documentation-request.md b/.github/ISSUE_TEMPLATE/documentation-request.md index 86d15328..cf0250ae 100644 --- a/.github/ISSUE_TEMPLATE/documentation-request.md +++ b/.github/ISSUE_TEMPLATE/documentation-request.md @@ -1,12 +1,20 @@ --- name: Documentation Request about: Request documentation for a feature -title: '' -labels: documentation, needs triage +title: '[Docs]:' +labels: docs, needs triage assignees: '' --- +## Hello! + + +- Vote on this issue by adding a 👍 reaction +- If you want to document this feature, comment to let us know (we'll work with you on design, scheduling, etc.) + +## Affected area/feature + +- Vote on this issue by adding a 👍 reaction +- If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.) -### Why this is needed +## Issue details + + + +## Why is this needed? + + From 479eda73397a98f729c87b0253b7684921c251ba Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 19 May 2022 01:25:30 +0200 Subject: [PATCH 08/25] Improve error message when client renews with expired certificate When a client provides an expired certificate and `AllowAfterExpiry` is not enabled, the client would get a rather generic error with instructions to view the CA logs. Viewing the CA logs can be done when running `step-ca`, but they can't be accessed easily in the hosted solution. This commit returns a slightly more informational message to the client in this specific situation. --- authority/provisioner/controller.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/authority/provisioner/controller.go b/authority/provisioner/controller.go index 0ca40267..063ab50c 100644 --- a/authority/provisioner/controller.go +++ b/authority/provisioner/controller.go @@ -3,6 +3,7 @@ package provisioner import ( "context" "crypto/x509" + "net/http" "regexp" "strings" "time" @@ -131,7 +132,9 @@ func DefaultAuthorizeRenew(ctx context.Context, p *Controller, cert *x509.Certif return errs.Unauthorized("certificate is not yet valid" + " " + now.UTC().Format(time.RFC3339Nano) + " vs " + cert.NotBefore.Format(time.RFC3339Nano)) } if now.After(cert.NotAfter) && !p.Claimer.AllowRenewalAfterExpiry() { - return errs.Unauthorized("certificate has expired") + // return a custom 401 Unauthorized error with a clearer message for the client + // TODO(hs): these errors likely need to be refactored as a whole; HTTP status codes shouldn't be in this layer. + return errs.New(http.StatusUnauthorized, "The request lacked necessary authorization to be completed: certificate expired on %s", cert.NotAfter) } return nil From 20b2c6a2017ac0f370a7916b7fd2104575427347 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 18 May 2022 18:27:37 -0700 Subject: [PATCH 09/25] Extract cert storer methods from AuthDB To be able to extend the AuthDB with methods that also extend the provisioner we need to either create a new method or to split the interface. This change splits the interface so we can have a cleaner implementation. --- db/db.go | 9 +++++++-- db/simple.go | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/db/db.go b/db/db.go index eccaf801..8cd1db0f 100644 --- a/db/db.go +++ b/db/db.go @@ -50,14 +50,19 @@ type AuthDB interface { Revoke(rci *RevokedCertificateInfo) error RevokeSSH(rci *RevokedCertificateInfo) error GetCertificate(serialNumber string) (*x509.Certificate, error) - StoreCertificate(crt *x509.Certificate) error UseToken(id, tok string) (bool, error) IsSSHHost(name string) (bool, error) - StoreSSHCertificate(crt *ssh.Certificate) error GetSSHHostPrincipals() ([]string, error) Shutdown() error } +// CertificateStorer is an extension of AuthDB that allows to store +// certificates. +type CertificateStorer interface { + StoreCertificate(crt *x509.Certificate) error + StoreSSHCertificate(crt *ssh.Certificate) error +} + // DB is a wrapper over the nosql.DB interface. type DB struct { nosql.DB diff --git a/db/simple.go b/db/simple.go index 0e5426ec..a7e38de9 100644 --- a/db/simple.go +++ b/db/simple.go @@ -20,7 +20,7 @@ type SimpleDB struct { usedTokens *sync.Map } -func newSimpleDB(c *Config) (AuthDB, error) { +func newSimpleDB(c *Config) (*SimpleDB, error) { db := &SimpleDB{} db.usedTokens = new(sync.Map) return db, nil From de99c3cac00385ee20b92c1e74eaf7185ba69ce5 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 18 May 2022 18:30:53 -0700 Subject: [PATCH 10/25] Report provisioner and parent on linkedca --- authority/linkedca.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/authority/linkedca.go b/authority/linkedca.go index 0552f2d1..9daba353 100644 --- a/authority/linkedca.go +++ b/authority/linkedca.go @@ -292,11 +292,22 @@ func (c *linkedCaClient) StoreRenewedCertificate(parent *x509.Certificate, fullc return errors.Wrap(err, "error posting certificate") } -func (c *linkedCaClient) StoreSSHCertificate(crt *ssh.Certificate) error { +func (c *linkedCaClient) StoreSSHCertificate(prov provisioner.Interface, crt *ssh.Certificate) error { ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() _, err := c.client.PostSSHCertificate(ctx, &linkedca.SSHCertificateRequest{ Certificate: string(ssh.MarshalAuthorizedKey(crt)), + Provisioner: createProvisionerIdentity(prov), + }) + return errors.Wrap(err, "error posting ssh certificate") +} + +func (c *linkedCaClient) StoreRenewedSSHCertificate(parent, crt *ssh.Certificate) error { + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + _, err := c.client.PostSSHCertificate(ctx, &linkedca.SSHCertificateRequest{ + Certificate: string(ssh.MarshalAuthorizedKey(crt)), + ParentCertificate: string(ssh.MarshalAuthorizedKey(parent)), }) return errors.Wrap(err, "error posting ssh certificate") } From c8d7ad7ab92d2f3963ef6e9559d89d7c1562f7fc Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 18 May 2022 18:33:22 -0700 Subject: [PATCH 11/25] Fix store certificates methods with new interface --- authority/tls.go | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/authority/tls.go b/authority/tls.go index d23b0da7..fd21ae98 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -365,28 +365,31 @@ func (a *Authority) Rekey(oldCert *x509.Certificate, pk crypto.PublicKey) ([]*x5 // `StoreCertificate(...*x509.Certificate) error` instead of just // `StoreCertificate(*x509.Certificate) error`. func (a *Authority) storeCertificate(prov provisioner.Interface, fullchain []*x509.Certificate) error { - type linkedChainStorer interface { + type certificateChainStorer interface { StoreCertificateChain(provisioner.Interface, ...*x509.Certificate) error } - type certificateChainStorer interface { + type certificateChainSimpleStorer interface { StoreCertificateChain(...*x509.Certificate) error } + // Store certificate in linkedca switch s := a.adminDB.(type) { - case linkedChainStorer: - return s.StoreCertificateChain(prov, fullchain...) case certificateChainStorer: + return s.StoreCertificateChain(prov, fullchain...) + case certificateChainSimpleStorer: return s.StoreCertificateChain(fullchain...) } // Store certificate in local db switch s := a.db.(type) { - case linkedChainStorer: - return s.StoreCertificateChain(prov, fullchain...) case certificateChainStorer: + return s.StoreCertificateChain(prov, fullchain...) + case certificateChainSimpleStorer: return s.StoreCertificateChain(fullchain...) + case db.CertificateStorer: + return s.StoreCertificate(fullchain[0]) default: - return a.db.StoreCertificate(fullchain[0]) + return nil } } @@ -398,15 +401,21 @@ func (a *Authority) storeRenewedCertificate(oldCert *x509.Certificate, fullchain type renewedCertificateChainStorer interface { StoreRenewedCertificate(*x509.Certificate, ...*x509.Certificate) error } + // Store certificate in linkedca if s, ok := a.adminDB.(renewedCertificateChainStorer); ok { return s.StoreRenewedCertificate(oldCert, fullchain...) } + // Store certificate in local db - if s, ok := a.db.(renewedCertificateChainStorer); ok { + switch s := a.db.(type) { + case renewedCertificateChainStorer: return s.StoreRenewedCertificate(oldCert, fullchain...) + case db.CertificateStorer: + return s.StoreCertificate(fullchain[0]) + default: + return nil } - return a.db.StoreCertificate(fullchain[0]) } // RevokeOptions are the options for the Revoke API. From 293586079af39d21b10510ee6ebc472c33ff4028 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 18 May 2022 18:33:53 -0700 Subject: [PATCH 12/25] Store provisioner with SignSSH This change also allows to store the old certificate on renewal on linkedca or if the db interface supports it. --- authority/ssh.go | 59 +++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 51 insertions(+), 8 deletions(-) diff --git a/authority/ssh.go b/authority/ssh.go index 0521ab58..bb69f9db 100644 --- a/authority/ssh.go +++ b/authority/ssh.go @@ -161,8 +161,13 @@ func (a *Authority) SignSSH(ctx context.Context, key ssh.PublicKey, opts provisi // Set backdate with the configured value opts.Backdate = a.config.AuthorityConfig.Backdate.Duration + var prov provisioner.Interface for _, op := range signOpts { switch o := op.(type) { + // Capture current provisioner + case provisioner.Interface: + prov = o + // add options to NewCertificate case provisioner.SSHCertificateOptions: certOptions = append(certOptions, o.Options(opts)...) @@ -276,7 +281,7 @@ func (a *Authority) SignSSH(ctx context.Context, key ssh.PublicKey, opts provisi } } - if err = a.storeSSHCertificate(cert); err != nil && err != db.ErrNotImplemented { + if err = a.storeSSHCertificate(prov, cert); err != nil && err != db.ErrNotImplemented { return nil, errs.Wrap(http.StatusInternalServerError, err, "authority.SignSSH: error storing certificate in db") } @@ -340,7 +345,7 @@ func (a *Authority) RenewSSH(ctx context.Context, oldCert *ssh.Certificate) (*ss return nil, errs.Wrap(http.StatusInternalServerError, err, "signSSH: error signing certificate") } - if err = a.storeSSHCertificate(cert); err != nil && err != db.ErrNotImplemented { + if err = a.storeRenewedSSHCertificate(oldCert, cert); err != nil && err != db.ErrNotImplemented { return nil, errs.Wrap(http.StatusInternalServerError, err, "renewSSH: error storing certificate in db") } @@ -419,21 +424,59 @@ func (a *Authority) RekeySSH(ctx context.Context, oldCert *ssh.Certificate, pub } } - if err = a.storeSSHCertificate(cert); err != nil && err != db.ErrNotImplemented { + if err = a.storeRenewedSSHCertificate(oldCert, cert); err != nil && err != db.ErrNotImplemented { return nil, errs.Wrap(http.StatusInternalServerError, err, "rekeySSH; error storing certificate in db") } return cert, nil } -func (a *Authority) storeSSHCertificate(cert *ssh.Certificate) error { +func (a *Authority) storeSSHCertificate(prov provisioner.Interface, cert *ssh.Certificate) error { type sshCertificateStorer interface { - StoreSSHCertificate(crt *ssh.Certificate) error + StoreSSHCertificate(provisioner.Interface, *ssh.Certificate) error } - if s, ok := a.adminDB.(sshCertificateStorer); ok { + + // Store certificate in admindb or linkedca + switch s := a.adminDB.(type) { + case sshCertificateStorer: + return s.StoreSSHCertificate(prov, cert) + case db.CertificateStorer: return s.StoreSSHCertificate(cert) } - return a.db.StoreSSHCertificate(cert) + + // Store certificate in localdb + switch s := a.db.(type) { + case sshCertificateStorer: + return s.StoreSSHCertificate(prov, cert) + case db.CertificateStorer: + return s.StoreSSHCertificate(cert) + default: + return nil + } +} + +func (a *Authority) storeRenewedSSHCertificate(parent, cert *ssh.Certificate) error { + type sshRenewerCertificateStorer interface { + StoreRenewedSSHCertificate(parent, cert *ssh.Certificate) error + } + + // Store certificate in admindb or linkedca + switch s := a.adminDB.(type) { + case sshRenewerCertificateStorer: + return s.StoreRenewedSSHCertificate(parent, cert) + case db.CertificateStorer: + return s.StoreSSHCertificate(cert) + } + + // Store certificate in localdb + switch s := a.db.(type) { + case sshRenewerCertificateStorer: + return s.StoreRenewedSSHCertificate(parent, cert) + case db.CertificateStorer: + return s.StoreSSHCertificate(cert) + default: + return nil + } } // IsValidForAddUser checks if a user provisioner certificate can be issued to @@ -511,7 +554,7 @@ func (a *Authority) SignSSHAddUser(ctx context.Context, key ssh.PublicKey, subje } cert.Signature = sig - if err = a.storeSSHCertificate(cert); err != nil && err != db.ErrNotImplemented { + if err = a.storeRenewedSSHCertificate(subject, cert); err != nil && err != db.ErrNotImplemented { return nil, errs.Wrap(http.StatusInternalServerError, err, "signSSHAddUser: error storing certificate in db") } From e7d7eb1a947bd60cc70fc67abe775c9f6060a29e Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 18 May 2022 18:42:42 -0700 Subject: [PATCH 13/25] Add provisioner as a signOption for SSH --- authority/provisioner/aws.go | 1 + authority/provisioner/azure.go | 1 + authority/provisioner/gcp.go | 1 + authority/provisioner/jwk.go | 1 + authority/provisioner/k8sSA.go | 1 + authority/provisioner/nebula.go | 1 + authority/provisioner/noop.go | 2 +- authority/provisioner/oidc.go | 1 + authority/provisioner/x5c.go | 1 + 9 files changed, 9 insertions(+), 1 deletion(-) diff --git a/authority/provisioner/aws.go b/authority/provisioner/aws.go index 8433fde5..afc61dd7 100644 --- a/authority/provisioner/aws.go +++ b/authority/provisioner/aws.go @@ -747,6 +747,7 @@ func (p *AWS) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption, signOptions = append(signOptions, templateOptions) return append(signOptions, + p, // Validate user SignSSHOptions. sshCertOptionsValidator(defaults), // Set the validity bounds if not set. diff --git a/authority/provisioner/azure.go b/authority/provisioner/azure.go index 438ab5b3..b6f7ec91 100644 --- a/authority/provisioner/azure.go +++ b/authority/provisioner/azure.go @@ -418,6 +418,7 @@ func (p *Azure) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOptio signOptions = append(signOptions, templateOptions) return append(signOptions, + p, // Validate user SignSSHOptions. sshCertOptionsValidator(defaults), // Set the validity bounds if not set. diff --git a/authority/provisioner/gcp.go b/authority/provisioner/gcp.go index 94c19e17..a116312d 100644 --- a/authority/provisioner/gcp.go +++ b/authority/provisioner/gcp.go @@ -425,6 +425,7 @@ func (p *GCP) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption, signOptions = append(signOptions, templateOptions) return append(signOptions, + p, // Validate user SignSSHOptions. sshCertOptionsValidator(defaults), // Set the validity bounds if not set. diff --git a/authority/provisioner/jwk.go b/authority/provisioner/jwk.go index 336736db..de592941 100644 --- a/authority/provisioner/jwk.go +++ b/authority/provisioner/jwk.go @@ -257,6 +257,7 @@ func (p *JWK) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption, } return append(signOptions, + p, // Set the validity bounds if not set. &sshDefaultDuration{p.ctl.Claimer}, // Validate public key diff --git a/authority/provisioner/k8sSA.go b/authority/provisioner/k8sSA.go index e2dbf840..28be0d5c 100644 --- a/authority/provisioner/k8sSA.go +++ b/authority/provisioner/k8sSA.go @@ -275,6 +275,7 @@ func (p *K8sSA) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOptio signOptions := []SignOption{templateOptions} return append(signOptions, + p, // Require type, key-id and principals in the SignSSHOptions. &sshCertOptionsRequireValidator{CertType: true, KeyID: true, Principals: true}, // Set the validity bounds if not set. diff --git a/authority/provisioner/nebula.go b/authority/provisioner/nebula.go index 38a2409f..cde5857c 100644 --- a/authority/provisioner/nebula.go +++ b/authority/provisioner/nebula.go @@ -250,6 +250,7 @@ func (p *Nebula) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOpti } return append(signOptions, + p, templateOptions, // Checks the validity bounds, and set the validity if has not been set. &sshLimitDuration{p.ctl.Claimer, crt.Details.NotAfter}, diff --git a/authority/provisioner/noop.go b/authority/provisioner/noop.go index 39661e54..9ccd0c8c 100644 --- a/authority/provisioner/noop.go +++ b/authority/provisioner/noop.go @@ -50,7 +50,7 @@ func (p *noop) AuthorizeRevoke(ctx context.Context, token string) error { } func (p *noop) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption, error) { - return []SignOption{}, nil + return []SignOption{p}, nil } func (p *noop) AuthorizeSSHRenew(ctx context.Context, token string) (*ssh.Certificate, error) { diff --git a/authority/provisioner/oidc.go b/authority/provisioner/oidc.go index 9f389b29..e64d98d9 100644 --- a/authority/provisioner/oidc.go +++ b/authority/provisioner/oidc.go @@ -434,6 +434,7 @@ func (o *OIDC) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption } return append(signOptions, + o, // Set the validity bounds if not set. &sshDefaultDuration{o.ctl.Claimer}, // Validate public key diff --git a/authority/provisioner/x5c.go b/authority/provisioner/x5c.go index 69576da5..b9ae24c5 100644 --- a/authority/provisioner/x5c.go +++ b/authority/provisioner/x5c.go @@ -312,6 +312,7 @@ func (p *X5C) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption, } return append(signOptions, + p, // Checks the validity bounds, and set the validity if has not been set. &sshLimitDuration{p.ctl.Claimer, claims.chains[0][0].NotAfter}, // Validate public key. From a627f2144039e9b2e56020e18ced2f7b6b80c2cf Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 18 May 2022 18:51:36 -0700 Subject: [PATCH 14/25] Fix AuthorizeSSHSign tests with extra SignOption --- authority/authorize_test.go | 2 +- authority/provisioner/aws_test.go | 1 - authority/provisioner/k8sSA_test.go | 3 ++- authority/provisioner/ssh_test.go | 1 + authority/provisioner/x5c_test.go | 5 +++-- 5 files changed, 7 insertions(+), 5 deletions(-) diff --git a/authority/authorize_test.go b/authority/authorize_test.go index 087318be..b221d0de 100644 --- a/authority/authorize_test.go +++ b/authority/authorize_test.go @@ -1034,7 +1034,7 @@ func TestAuthority_authorizeSSHSign(t *testing.T) { } } else { if assert.Nil(t, tc.err) { - assert.Len(t, 8, got) // number of provisioner.SignOptions returned + assert.Len(t, 9, got) // number of provisioner.SignOptions returned } } }) diff --git a/authority/provisioner/aws_test.go b/authority/provisioner/aws_test.go index 0d9b5d4d..d12d0626 100644 --- a/authority/provisioner/aws_test.go +++ b/authority/provisioner/aws_test.go @@ -813,7 +813,6 @@ func TestAWS_AuthorizeSSHSign(t *testing.T) { } else if assert.NotNil(t, got) { cert, err := signSSHCertificate(tt.args.key, tt.args.sshOpts, got, signer.Key.(crypto.Signer)) if (err != nil) != tt.wantSignErr { - t.Errorf("SignSSH error = %v, wantSignErr %v", err, tt.wantSignErr) } else { if tt.wantSignErr { diff --git a/authority/provisioner/k8sSA_test.go b/authority/provisioner/k8sSA_test.go index 1eff379d..2458babb 100644 --- a/authority/provisioner/k8sSA_test.go +++ b/authority/provisioner/k8sSA_test.go @@ -368,9 +368,10 @@ func TestK8sSA_AuthorizeSSHSign(t *testing.T) { } else { if assert.Nil(t, tc.err) { if assert.NotNil(t, opts) { - assert.Len(t, 7, opts) + assert.Len(t, 8, opts) for _, o := range opts { switch v := o.(type) { + case Interface: case sshCertificateOptionsFunc: case *sshCertOptionsRequireValidator: assert.Equals(t, v, &sshCertOptionsRequireValidator{CertType: true, KeyID: true, Principals: true}) diff --git a/authority/provisioner/ssh_test.go b/authority/provisioner/ssh_test.go index c530cd3c..90271443 100644 --- a/authority/provisioner/ssh_test.go +++ b/authority/provisioner/ssh_test.go @@ -53,6 +53,7 @@ func signSSHCertificate(key crypto.PublicKey, opts SignSSHOptions, signOpts []Si for _, op := range signOpts { switch o := op.(type) { + case Interface: // add options to NewCertificate case SSHCertificateOptions: certOptions = append(certOptions, o.Options(opts)...) diff --git a/authority/provisioner/x5c_test.go b/authority/provisioner/x5c_test.go index f28fcc7c..3bcf30d1 100644 --- a/authority/provisioner/x5c_test.go +++ b/authority/provisioner/x5c_test.go @@ -769,6 +769,7 @@ func TestX5C_AuthorizeSSHSign(t *testing.T) { nw := now() for _, o := range opts { switch v := o.(type) { + case Interface: case sshCertOptionsValidator: tc.claims.Step.SSH.ValidAfter.t = time.Time{} tc.claims.Step.SSH.ValidBefore.t = time.Time{} @@ -799,9 +800,9 @@ func TestX5C_AuthorizeSSHSign(t *testing.T) { tot++ } if len(tc.claims.Step.SSH.CertType) > 0 { - assert.Equals(t, tot, 10) + assert.Equals(t, tot, 11) } else { - assert.Equals(t, tot, 8) + assert.Equals(t, tot, 9) } } } From dd985ce154a4abd7733e303679df2698b365c07b Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 19 May 2022 18:41:13 -0700 Subject: [PATCH 15/25] Clarify errors when sending renewed certificates --- authority/linkedca.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/authority/linkedca.go b/authority/linkedca.go index 9daba353..fd5c0a81 100644 --- a/authority/linkedca.go +++ b/authority/linkedca.go @@ -289,7 +289,7 @@ func (c *linkedCaClient) StoreRenewedCertificate(parent *x509.Certificate, fullc PemCertificateChain: serializeCertificateChain(fullchain[1:]...), PemParentCertificate: serializeCertificateChain(parent), }) - return errors.Wrap(err, "error posting certificate") + return errors.Wrap(err, "error posting renewed certificate") } func (c *linkedCaClient) StoreSSHCertificate(prov provisioner.Interface, crt *ssh.Certificate) error { @@ -309,7 +309,7 @@ func (c *linkedCaClient) StoreRenewedSSHCertificate(parent, crt *ssh.Certificate Certificate: string(ssh.MarshalAuthorizedKey(crt)), ParentCertificate: string(ssh.MarshalAuthorizedKey(parent)), }) - return errors.Wrap(err, "error posting ssh certificate") + return errors.Wrap(err, "error posting renewed ssh certificate") } func (c *linkedCaClient) Revoke(crt *x509.Certificate, rci *db.RevokedCertificateInfo) error { From 1ad75a3bdb10c77de7d8dc671b5000d2c27ac563 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 19 May 2022 18:51:51 -0700 Subject: [PATCH 16/25] Skip failing test for now This test fails randomly on VMs, there's an issue to fix this so skipping it for now --- ca/bootstrap_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/ca/bootstrap_test.go b/ca/bootstrap_test.go index 9aaa5f1f..9eb3b2ea 100644 --- a/ca/bootstrap_test.go +++ b/ca/bootstrap_test.go @@ -370,6 +370,7 @@ func TestBootstrapClient(t *testing.T) { } func TestBootstrapClientServerRotation(t *testing.T) { + t.Skipf("skip until we fix https://github.com/smallstep/certificates/issues/873") reset := setMinCertDuration(1 * time.Second) defer reset() From 586e4fd3b5b9285ed0629ade4a121bf5ee65457c Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 19 May 2022 22:26:20 -0700 Subject: [PATCH 17/25] Update authority/options.go Co-authored-by: Mariano Cano --- authority/options.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/authority/options.go b/authority/options.go index 755e0fbc..429ccb91 100644 --- a/authority/options.go +++ b/authority/options.go @@ -267,6 +267,8 @@ func WithAdminDB(d admin.DB) Option { } // WithProvisioners is an option to set the provisioner collection. +// +// Deprecated: provisioner collections will likely change func WithProvisioners(ps *provisioner.Collection) Option { return func(a *Authority) error { a.provisioners = ps From 8ca9442fe9f2f5dd1e8f48d7bf7270cff3a18c62 Mon Sep 17 00:00:00 2001 From: max furman Date: Thu, 19 May 2022 22:40:12 -0700 Subject: [PATCH 18/25] Add -s to make fmt and bump golangci-lint to 1.45.2 --- .github/workflows/release.yml | 2 +- .github/workflows/test.yml | 2 +- Makefile | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index c90d949a..807cfdd6 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -33,7 +33,7 @@ jobs: uses: golangci/golangci-lint-action@v2 with: # Optional: version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version - version: 'v1.45.0' + version: 'v1.45.2' # Optional: working directory, useful for monorepos # working-directory: somedir diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b24426a0..046589af 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -33,7 +33,7 @@ jobs: uses: golangci/golangci-lint-action@v2 with: # Optional: version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version - version: 'v1.45.0' + version: 'v1.45.2' # Optional: working directory, useful for monorepos # working-directory: somedir diff --git a/Makefile b/Makefile index 09e342df..906569f1 100644 --- a/Makefile +++ b/Makefile @@ -151,7 +151,7 @@ integration: bin/$(BINNAME) ######################################### fmt: - $Q gofmt -l -w $(SRC) + $Q gofmt -l -s -w $(SRC) lint: $Q golangci-lint run --timeout=30m From 5443aa073a40c64bb91b0b6535abec6cb1d0f735 Mon Sep 17 00:00:00 2001 From: max furman Date: Thu, 19 May 2022 22:46:25 -0700 Subject: [PATCH 19/25] gofmt -s --- authority/options.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authority/options.go b/authority/options.go index 429ccb91..6e1949f5 100644 --- a/authority/options.go +++ b/authority/options.go @@ -267,7 +267,7 @@ func WithAdminDB(d admin.DB) Option { } // WithProvisioners is an option to set the provisioner collection. -// +// // Deprecated: provisioner collections will likely change func WithProvisioners(ps *provisioner.Collection) Option { return func(a *Authority) error { From eebbd65dd534091bdbaa610988f278eb56a27c5a Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Fri, 20 May 2022 12:03:36 -0700 Subject: [PATCH 20/25] Fix linter error --- ca/bootstrap_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ca/bootstrap_test.go b/ca/bootstrap_test.go index 9eb3b2ea..10e22519 100644 --- a/ca/bootstrap_test.go +++ b/ca/bootstrap_test.go @@ -7,6 +7,7 @@ import ( "net" "net/http" "net/http/httptest" + "os" "reflect" "strings" "sync" @@ -370,7 +371,9 @@ func TestBootstrapClient(t *testing.T) { } func TestBootstrapClientServerRotation(t *testing.T) { - t.Skipf("skip until we fix https://github.com/smallstep/certificates/issues/873") + if os.Getenv("CI") == "true" { + t.Skipf("skip until we fix https://github.com/smallstep/certificates/issues/873") + } reset := setMinCertDuration(1 * time.Second) defer reset() From dec1067addc5bd8f43fafeaca7cd4f6264738abf Mon Sep 17 00:00:00 2001 From: Erik De Lamarter Date: Mon, 25 Apr 2022 22:45:22 +0200 Subject: [PATCH 21/25] vault kubernetes auth --- cas/vaultcas/vaultcas.go | 72 +++++++++++++++++++++++++--------------- go.mod | 1 + go.sum | 2 ++ 3 files changed, 49 insertions(+), 26 deletions(-) diff --git a/cas/vaultcas/vaultcas.go b/cas/vaultcas/vaultcas.go index c29ef691..8a09a850 100644 --- a/cas/vaultcas/vaultcas.go +++ b/cas/vaultcas/vaultcas.go @@ -18,6 +18,7 @@ import ( vault "github.com/hashicorp/vault/api" auth "github.com/hashicorp/vault/api/auth/approle" + kubeauth "github.com/hashicorp/vault/api/auth/kubernetes" ) func init() { @@ -34,6 +35,7 @@ type VaultOptions struct { PKIRoleRSA string `json:"pkiRoleRSA,omitempty"` PKIRoleEC string `json:"pkiRoleEC,omitempty"` PKIRoleEd25519 string `json:"pkiRoleEd25519,omitempty"` + KubernetesRole string `json:"kubernetesRole,omitempty"` RoleID string `json:"roleID,omitempty"` SecretID auth.SecretID `json:"secretID,omitempty"` AppRole string `json:"appRole,omitempty"` @@ -77,31 +79,49 @@ func New(ctx context.Context, opts apiv1.Options) (*VaultCAS, error) { return nil, fmt.Errorf("unable to initialize vault client: %w", err) } - var appRoleAuth *auth.AppRoleAuth - if vc.IsWrappingToken { - appRoleAuth, err = auth.NewAppRoleAuth( - vc.RoleID, - &vc.SecretID, - auth.WithWrappingToken(), - auth.WithMountPath(vc.AppRole), + if vc.KubernetesRole != "" { + var kubernetesAuth *kubeauth.KubernetesAuth + kubernetesAuth, err = kubeauth.NewKubernetesAuth( + vc.KubernetesRole, ) - } else { - appRoleAuth, err = auth.NewAppRoleAuth( - vc.RoleID, - &vc.SecretID, - auth.WithMountPath(vc.AppRole), - ) - } - if err != nil { - return nil, fmt.Errorf("unable to initialize AppRole auth method: %w", err) - } + if err != nil { + return nil, fmt.Errorf("unable to initialize Kubernetes auth method: %w", err) + } - authInfo, err := client.Auth().Login(ctx, appRoleAuth) - if err != nil { - return nil, fmt.Errorf("unable to login to AppRole auth method: %w", err) - } - if authInfo == nil { - return nil, errors.New("no auth info was returned after login") + authInfo, err := client.Auth().Login(ctx, kubernetesAuth) + if err != nil { + return nil, fmt.Errorf("unable to login to Kubernetes auth method: %w", err) + } + if authInfo == nil { + return nil, errors.New("no auth info was returned after login") + } + } else { + var appRoleAuth *auth.AppRoleAuth + if vc.IsWrappingToken { + appRoleAuth, err = auth.NewAppRoleAuth( + vc.RoleID, + &vc.SecretID, + auth.WithWrappingToken(), + auth.WithMountPath(vc.AppRole), + ) + } else { + appRoleAuth, err = auth.NewAppRoleAuth( + vc.RoleID, + &vc.SecretID, + auth.WithMountPath(vc.AppRole), + ) + } + if err != nil { + return nil, fmt.Errorf("unable to initialize AppRole auth method: %w", err) + } + + authInfo, err := client.Auth().Login(ctx, appRoleAuth) + if err != nil { + return nil, fmt.Errorf("unable to login to AppRole auth method: %w", err) + } + if authInfo == nil { + return nil, errors.New("no auth info was returned after login") + } } return &VaultCAS{ @@ -272,11 +292,11 @@ func loadOptions(config json.RawMessage) (*VaultOptions, error) { vc.PKIRoleEd25519 = vc.PKIRoleDefault } - if vc.RoleID == "" { - return nil, errors.New("vaultCAS config options must define `roleID`") + if vc.RoleID == "" && vc.KubernetesRole == "" { + return nil, errors.New("vaultCAS config options must define `roleID` or `kubernetesRole`") } - if vc.SecretID.FromEnv == "" && vc.SecretID.FromFile == "" && vc.SecretID.FromString == "" { + if vc.SecretID.FromEnv == "" && vc.SecretID.FromFile == "" && vc.SecretID.FromString == "" && vc.RoleID != "" { return nil, errors.New("vaultCAS config options must define `secretID` object with one of `FromEnv`, `FromFile` or `FromString`") } diff --git a/go.mod b/go.mod index 8b66f470..0b772018 100644 --- a/go.mod +++ b/go.mod @@ -29,6 +29,7 @@ require ( github.com/googleapis/gax-go/v2 v2.1.1 github.com/hashicorp/vault/api v1.3.1 github.com/hashicorp/vault/api/auth/approle v0.1.1 + github.com/hashicorp/vault/api/auth/kubernetes v0.1.0 github.com/jhump/protoreflect v1.9.0 // indirect github.com/mattn/go-colorable v0.1.8 // indirect github.com/mattn/go-isatty v0.0.13 // indirect diff --git a/go.sum b/go.sum index 4780111e..d76648c2 100644 --- a/go.sum +++ b/go.sum @@ -449,6 +449,8 @@ github.com/hashicorp/vault/api v1.3.1 h1:pkDkcgTh47PRjY1NEFeofqR4W/HkNUi9qIakESO github.com/hashicorp/vault/api v1.3.1/go.mod h1:QeJoWxMFt+MsuWcYhmwRLwKEXrjwAFFywzhptMsTIUw= github.com/hashicorp/vault/api/auth/approle v0.1.1 h1:R5yA+xcNvw1ix6bDuWOaLOq2L4L77zDCVsethNw97xQ= github.com/hashicorp/vault/api/auth/approle v0.1.1/go.mod h1:mHOLgh//xDx4dpqXoq6tS8Ob0FoCFWLU2ibJ26Lfmag= +github.com/hashicorp/vault/api/auth/kubernetes v0.1.0 h1:6BtyahbF4aQp8gg3ww0A/oIoqzbhpNP1spXU3nHE0n0= +github.com/hashicorp/vault/api/auth/kubernetes v0.1.0/go.mod h1:Pdgk78uIs0mgDOLvc3a+h/vYIT9rznw2sz+ucuH9024= github.com/hashicorp/vault/sdk v0.3.0 h1:kR3dpxNkhh/wr6ycaJYqp6AFT/i2xaftbfnwZduTKEY= github.com/hashicorp/vault/sdk v0.3.0/go.mod h1:aZ3fNuL5VNydQk8GcLJ2TV8YCRVvyaakYkhZRoVuhj0= github.com/hashicorp/yamux v0.0.0-20180604194846-3520598351bb h1:b5rjCoWHc7eqmAS4/qyk21ZsHyb6Mxv/jykxvNTkU4M= From 6c44291d8df63e16e662a9cc03ffa8783fa364ce Mon Sep 17 00:00:00 2001 From: Erik De Lamarter Date: Mon, 9 May 2022 13:27:37 +0200 Subject: [PATCH 22/25] refactor vault auth --- cas/vaultcas/auth/approle/approle.go | 46 ++++ cas/vaultcas/auth/approle/approle_test.go | 16 ++ cas/vaultcas/auth/kubernetes/kubernetes.go | 43 +++ .../auth/kubernetes/kubernetes_test.go | 21 ++ cas/vaultcas/auth/kubernetes/token | 1 + cas/vaultcas/vaultcas.go | 120 +++----- cas/vaultcas/vaultcas_test.go | 256 ++++-------------- 7 files changed, 220 insertions(+), 283 deletions(-) create mode 100644 cas/vaultcas/auth/approle/approle.go create mode 100644 cas/vaultcas/auth/approle/approle_test.go create mode 100644 cas/vaultcas/auth/kubernetes/kubernetes.go create mode 100644 cas/vaultcas/auth/kubernetes/kubernetes_test.go create mode 100644 cas/vaultcas/auth/kubernetes/token diff --git a/cas/vaultcas/auth/approle/approle.go b/cas/vaultcas/auth/approle/approle.go new file mode 100644 index 00000000..38d3c51c --- /dev/null +++ b/cas/vaultcas/auth/approle/approle.go @@ -0,0 +1,46 @@ +package approle + +import ( + "encoding/json" + "fmt" + + "github.com/hashicorp/vault/api/auth/approle" +) + +// AuthOptions defines the configuration options added using the +// VaultOptions.AuthOptions field when AuthType is approle +type AuthOptions struct { + RoleID string `json:"roleID,omitempty"` + SecretID string `json:"secretID,omitempty"` + IsWrappingToken bool `json:"isWrappingToken,omitempty"` +} + +func NewApproleAuthMethod(mountPath string, options json.RawMessage) (*approle.AppRoleAuth, error) { + var opts *AuthOptions + + err := json.Unmarshal(options, &opts) + if err != nil { + return nil, fmt.Errorf("error decoding AppRole auth options: %w", err) + } + + var approleAuth *approle.AppRoleAuth + + var loginOptions []approle.LoginOption + if mountPath != "" { + loginOptions = append(loginOptions, approle.WithMountPath(mountPath)) + } + if opts.IsWrappingToken { + loginOptions = append(loginOptions, approle.WithWrappingToken()) + } + + sid := approle.SecretID{ + FromString: opts.SecretID, + } + + approleAuth, err = approle.NewAppRoleAuth(opts.RoleID, &sid, loginOptions...) + if err != nil { + return nil, fmt.Errorf("unable to initialize Kubernetes auth method: %w", err) + } + + return approleAuth, nil +} diff --git a/cas/vaultcas/auth/approle/approle_test.go b/cas/vaultcas/auth/approle/approle_test.go new file mode 100644 index 00000000..ab7e6a97 --- /dev/null +++ b/cas/vaultcas/auth/approle/approle_test.go @@ -0,0 +1,16 @@ +package approle + +import ( + "encoding/json" + "testing" +) + +func TestKubernetes_NewKubernetesAuthMethod(t *testing.T) { + mountPath := "approle" + raw := `{"roleID": "roleID", "secretID": "secretIDwrapped", "isWrappedToken": true}` + + _, err := NewApproleAuthMethod(mountPath, json.RawMessage(raw)) + if err != nil { + t.Fatal(err) + } +} diff --git a/cas/vaultcas/auth/kubernetes/kubernetes.go b/cas/vaultcas/auth/kubernetes/kubernetes.go new file mode 100644 index 00000000..0c4db62f --- /dev/null +++ b/cas/vaultcas/auth/kubernetes/kubernetes.go @@ -0,0 +1,43 @@ +package kubernetes + +import ( + "encoding/json" + "fmt" + + "github.com/hashicorp/vault/api/auth/kubernetes" +) + +// AuthOptions defines the configuration options added using the +// VaultOptions.AuthOptions field when AuthType is kubernetes +type AuthOptions struct { + Role string `json:"role,omitempty"` + TokenPath string `json:"tokenPath,omitempty"` +} + +func NewKubernetesAuthMethod(mountPath string, options json.RawMessage) (*kubernetes.KubernetesAuth, error) { + var opts *AuthOptions + + err := json.Unmarshal(options, &opts) + if err != nil { + return nil, fmt.Errorf("error decoding Kubernetes auth options: %w", err) + } + + var kubernetesAuth *kubernetes.KubernetesAuth + + var loginOptions []kubernetes.LoginOption + if mountPath != "" { + loginOptions = append(loginOptions, kubernetes.WithMountPath(mountPath)) + } + if opts.TokenPath != "" { + loginOptions = append(loginOptions, kubernetes.WithServiceAccountTokenPath(opts.TokenPath)) + } + kubernetesAuth, err = kubernetes.NewKubernetesAuth( + opts.Role, + loginOptions..., + ) + if err != nil { + return nil, fmt.Errorf("unable to initialize Kubernetes auth method: %w", err) + } + + return kubernetesAuth, nil +} diff --git a/cas/vaultcas/auth/kubernetes/kubernetes_test.go b/cas/vaultcas/auth/kubernetes/kubernetes_test.go new file mode 100644 index 00000000..604f1898 --- /dev/null +++ b/cas/vaultcas/auth/kubernetes/kubernetes_test.go @@ -0,0 +1,21 @@ +package kubernetes + +import ( + "encoding/json" + "path" + "path/filepath" + "runtime" + "testing" +) + +func TestKubernetes_NewKubernetesAuthMethod(t *testing.T) { + _, filename, _, _ := runtime.Caller(0) + tokenPath := filepath.Join(path.Dir(filename), "token") + mountPath := "kubernetes" + raw := `{"role": "SomeRoleName", "tokenPath": "` + tokenPath + `"}` + + _, err := NewKubernetesAuthMethod(mountPath, json.RawMessage(raw)) + if err != nil { + t.Fatal(err) + } +} diff --git a/cas/vaultcas/auth/kubernetes/token b/cas/vaultcas/auth/kubernetes/token new file mode 100644 index 00000000..6745be67 --- /dev/null +++ b/cas/vaultcas/auth/kubernetes/token @@ -0,0 +1 @@ +token \ No newline at end of file diff --git a/cas/vaultcas/vaultcas.go b/cas/vaultcas/vaultcas.go index 8a09a850..02c814b7 100644 --- a/cas/vaultcas/vaultcas.go +++ b/cas/vaultcas/vaultcas.go @@ -15,10 +15,10 @@ import ( "time" "github.com/smallstep/certificates/cas/apiv1" + "github.com/smallstep/certificates/cas/vaultcas/auth/approle" + "github.com/smallstep/certificates/cas/vaultcas/auth/kubernetes" vault "github.com/hashicorp/vault/api" - auth "github.com/hashicorp/vault/api/auth/approle" - kubeauth "github.com/hashicorp/vault/api/auth/kubernetes" ) func init() { @@ -30,16 +30,14 @@ func init() { // VaultOptions defines the configuration options added using the // apiv1.Options.Config field. type VaultOptions struct { - PKI string `json:"pki,omitempty"` - PKIRoleDefault string `json:"pkiRoleDefault,omitempty"` - PKIRoleRSA string `json:"pkiRoleRSA,omitempty"` - PKIRoleEC string `json:"pkiRoleEC,omitempty"` - PKIRoleEd25519 string `json:"pkiRoleEd25519,omitempty"` - KubernetesRole string `json:"kubernetesRole,omitempty"` - RoleID string `json:"roleID,omitempty"` - SecretID auth.SecretID `json:"secretID,omitempty"` - AppRole string `json:"appRole,omitempty"` - IsWrappingToken bool `json:"isWrappingToken,omitempty"` + PKIMountPath string `json:"pkiMountPath,omitempty"` + PKIRoleDefault string `json:"pkiRoleDefault,omitempty"` + PKIRoleRSA string `json:"pkiRoleRSA,omitempty"` + PKIRoleEC string `json:"pkiRoleEC,omitempty"` + PKIRoleEd25519 string `json:"pkiRoleEd25519,omitempty"` + AuthType string `json:"authType,omitempty"` + AuthMountPath string `json:"authMountPath,omitempty"` + AuthOptions json.RawMessage `json:"authOptions,omitempty"` } // VaultCAS implements a Certificate Authority Service using Hashicorp Vault. @@ -79,49 +77,25 @@ func New(ctx context.Context, opts apiv1.Options) (*VaultCAS, error) { return nil, fmt.Errorf("unable to initialize vault client: %w", err) } - if vc.KubernetesRole != "" { - var kubernetesAuth *kubeauth.KubernetesAuth - kubernetesAuth, err = kubeauth.NewKubernetesAuth( - vc.KubernetesRole, - ) - if err != nil { - return nil, fmt.Errorf("unable to initialize Kubernetes auth method: %w", err) - } + var method vault.AuthMethod + switch vc.AuthType { + case "kubernetes": + method, err = kubernetes.NewKubernetesAuthMethod(vc.AuthMountPath, vc.AuthOptions) + case "approle": + method, err = approle.NewApproleAuthMethod(vc.AuthMountPath, vc.AuthOptions) + default: + return nil, fmt.Errorf("unknown auth type: %v", vc.AuthType) + } + if err != nil { + return nil, fmt.Errorf("unable to configure auth method: %w", err) + } - authInfo, err := client.Auth().Login(ctx, kubernetesAuth) - if err != nil { - return nil, fmt.Errorf("unable to login to Kubernetes auth method: %w", err) - } - if authInfo == nil { - return nil, errors.New("no auth info was returned after login") - } - } else { - var appRoleAuth *auth.AppRoleAuth - if vc.IsWrappingToken { - appRoleAuth, err = auth.NewAppRoleAuth( - vc.RoleID, - &vc.SecretID, - auth.WithWrappingToken(), - auth.WithMountPath(vc.AppRole), - ) - } else { - appRoleAuth, err = auth.NewAppRoleAuth( - vc.RoleID, - &vc.SecretID, - auth.WithMountPath(vc.AppRole), - ) - } - if err != nil { - return nil, fmt.Errorf("unable to initialize AppRole auth method: %w", err) - } - - authInfo, err := client.Auth().Login(ctx, appRoleAuth) - if err != nil { - return nil, fmt.Errorf("unable to login to AppRole auth method: %w", err) - } - if authInfo == nil { - return nil, errors.New("no auth info was returned after login") - } + authInfo, err := client.Auth().Login(ctx, method) + if err != nil { + return nil, fmt.Errorf("unable to login to Kubernetes auth method: %w", err) + } + if authInfo == nil { + return nil, errors.New("no auth info was returned after login") } return &VaultCAS{ @@ -154,7 +128,7 @@ func (v *VaultCAS) CreateCertificate(req *apiv1.CreateCertificateRequest) (*apiv // GetCertificateAuthority returns the root certificate of the certificate // authority using the configured fingerprint. func (v *VaultCAS) GetCertificateAuthority(req *apiv1.GetCertificateAuthorityRequest) (*apiv1.GetCertificateAuthorityResponse, error) { - secret, err := v.client.Logical().Read(v.config.PKI + "/cert/ca_chain") + secret, err := v.client.Logical().Read(v.config.PKIMountPath + "/cert/ca_chain") if err != nil { return nil, fmt.Errorf("error reading ca chain: %w", err) } @@ -210,7 +184,7 @@ func (v *VaultCAS) RevokeCertificate(req *apiv1.RevokeCertificateRequest) (*apiv vaultReq := map[string]interface{}{ "serial_number": formatSerialNumber(sn), } - _, err := v.client.Logical().Write(v.config.PKI+"/revoke/", vaultReq) + _, err := v.client.Logical().Write(v.config.PKIMountPath+"/revoke/", vaultReq) if err != nil { return nil, fmt.Errorf("error revoking certificate: %w", err) } @@ -244,7 +218,7 @@ func (v *VaultCAS) createCertificate(cr *x509.CertificateRequest, lifetime time. "ttl": lifetime.Seconds(), } - secret, err := v.client.Logical().Write(v.config.PKI+"/sign/"+vaultPKIRole, vaultReq) + secret, err := v.client.Logical().Write(v.config.PKIMountPath+"/sign/"+vaultPKIRole, vaultReq) if err != nil { return nil, nil, fmt.Errorf("error signing certificate: %w", err) } @@ -267,21 +241,17 @@ func (v *VaultCAS) createCertificate(cr *x509.CertificateRequest, lifetime time. } func loadOptions(config json.RawMessage) (*VaultOptions, error) { - var vc *VaultOptions + // setup default values + vc := VaultOptions{ + PKIMountPath: "pki", + PKIRoleDefault: "default", + } err := json.Unmarshal(config, &vc) if err != nil { return nil, fmt.Errorf("error decoding vaultCAS config: %w", err) } - if vc.PKI == "" { - vc.PKI = "pki" // use default pki vault name - } - - if vc.PKIRoleDefault == "" { - vc.PKIRoleDefault = "default" // use default pki role name - } - if vc.PKIRoleRSA == "" { vc.PKIRoleRSA = vc.PKIRoleDefault } @@ -292,23 +262,7 @@ func loadOptions(config json.RawMessage) (*VaultOptions, error) { vc.PKIRoleEd25519 = vc.PKIRoleDefault } - if vc.RoleID == "" && vc.KubernetesRole == "" { - return nil, errors.New("vaultCAS config options must define `roleID` or `kubernetesRole`") - } - - if vc.SecretID.FromEnv == "" && vc.SecretID.FromFile == "" && vc.SecretID.FromString == "" && vc.RoleID != "" { - return nil, errors.New("vaultCAS config options must define `secretID` object with one of `FromEnv`, `FromFile` or `FromString`") - } - - if vc.PKI == "" { - vc.PKI = "pki" // use default pki vault name - } - - if vc.AppRole == "" { - vc.AppRole = "auth/approle" - } - - return vc, nil + return &vc, nil } func parseCertificates(pemCert string) []*x509.Certificate { diff --git a/cas/vaultcas/vaultcas_test.go b/cas/vaultcas/vaultcas_test.go index 9f73a1ee..3c1f09a3 100644 --- a/cas/vaultcas/vaultcas_test.go +++ b/cas/vaultcas/vaultcas_test.go @@ -14,7 +14,6 @@ import ( "time" vault "github.com/hashicorp/vault/api" - auth "github.com/hashicorp/vault/api/auth/approle" "github.com/smallstep/certificates/cas/apiv1" "go.step.sm/crypto/pemutil" ) @@ -99,7 +98,7 @@ func testCAHelper(t *testing.T) (*url.URL, *vault.Client) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch { - case r.RequestURI == "/v1/auth/auth/approle/login": + case r.RequestURI == "/v1/auth/approle/login": w.WriteHeader(http.StatusOK) fmt.Fprintf(w, `{ "auth": { @@ -183,11 +182,10 @@ func TestNew_register(t *testing.T) { CertificateAuthority: caURL.String(), CertificateAuthorityFingerprint: testRootFingerprint, Config: json.RawMessage(`{ - "PKI": "pki", + "PKIMountPath": "pki", "PKIRoleDefault": "pki-role", - "RoleID": "roleID", - "SecretID": {"FromString": "secretID"}, - "IsWrappingToken": false + "AuthType": "approle", + "AuthOptions": {"RoleID":"roleID","SecretID":"secretID","IsWrappingToken":false} }`), }) @@ -201,15 +199,13 @@ func TestVaultCAS_CreateCertificate(t *testing.T) { _, client := testCAHelper(t) options := VaultOptions{ - PKI: "pki", - PKIRoleDefault: "role", - PKIRoleRSA: "rsa", - PKIRoleEC: "ec", - PKIRoleEd25519: "ed25519", - RoleID: "roleID", - SecretID: auth.SecretID{FromString: "secretID"}, - AppRole: "approle", - IsWrappingToken: false, + PKIMountPath: "pki", + PKIRoleDefault: "role", + PKIRoleRSA: "rsa", + PKIRoleEC: "ec", + PKIRoleEd25519: "ed25519", + AuthType: "approle", + AuthOptions: json.RawMessage(`{"RoleID":"roleID","SecretID":"secretID","IsWrappingToken":false}`), } type fields struct { @@ -291,7 +287,7 @@ func TestVaultCAS_GetCertificateAuthority(t *testing.T) { } options := VaultOptions{ - PKI: "pki", + PKIMountPath: "pki", } rootCert := parseCertificates(testRootCertificate)[0] @@ -335,15 +331,13 @@ func TestVaultCAS_RevokeCertificate(t *testing.T) { _, client := testCAHelper(t) options := VaultOptions{ - PKI: "pki", - PKIRoleDefault: "role", - PKIRoleRSA: "rsa", - PKIRoleEC: "ec", - PKIRoleEd25519: "ed25519", - RoleID: "roleID", - SecretID: auth.SecretID{FromString: "secretID"}, - AppRole: "approle", - IsWrappingToken: false, + PKIMountPath: "pki", + PKIRoleDefault: "role", + PKIRoleRSA: "rsa", + PKIRoleEC: "ec", + PKIRoleEd25519: "ed25519", + AuthType: "approle", + AuthOptions: json.RawMessage(`{"RoleID":"roleID","SecretID":"secretID","IsWrappingToken":false}`), } type fields struct { @@ -407,15 +401,13 @@ func TestVaultCAS_RenewCertificate(t *testing.T) { _, client := testCAHelper(t) options := VaultOptions{ - PKI: "pki", - PKIRoleDefault: "role", - PKIRoleRSA: "rsa", - PKIRoleEC: "ec", - PKIRoleEd25519: "ed25519", - RoleID: "roleID", - SecretID: auth.SecretID{FromString: "secretID"}, - AppRole: "approle", - IsWrappingToken: false, + PKIMountPath: "pki", + PKIRoleDefault: "role", + PKIRoleRSA: "rsa", + PKIRoleEC: "ec", + PKIRoleEd25519: "ed25519", + AuthType: "approle", + AuthOptions: json.RawMessage(`{"RoleID":"roleID","SecretID":"secretID","IsWrappingToken":false}`), } type fields struct { @@ -464,202 +456,66 @@ func TestVaultCAS_loadOptions(t *testing.T) { want *VaultOptions wantErr bool }{ - { - "ok mandatory with SecretID FromString", - `{"RoleID": "roleID", "SecretID": {"FromString": "secretID"}}`, - &VaultOptions{ - PKI: "pki", - PKIRoleDefault: "default", - PKIRoleRSA: "default", - PKIRoleEC: "default", - PKIRoleEd25519: "default", - RoleID: "roleID", - SecretID: auth.SecretID{FromString: "secretID"}, - AppRole: "auth/approle", - IsWrappingToken: false, - }, - false, - }, - { - "ok mandatory with SecretID FromFile", - `{"RoleID": "roleID", "SecretID": {"FromFile": "secretID"}}`, - &VaultOptions{ - PKI: "pki", - PKIRoleDefault: "default", - PKIRoleRSA: "default", - PKIRoleEC: "default", - PKIRoleEd25519: "default", - RoleID: "roleID", - SecretID: auth.SecretID{FromFile: "secretID"}, - AppRole: "auth/approle", - IsWrappingToken: false, - }, - false, - }, - { - "ok mandatory with SecretID FromEnv", - `{"RoleID": "roleID", "SecretID": {"FromEnv": "secretID"}}`, - &VaultOptions{ - PKI: "pki", - PKIRoleDefault: "default", - PKIRoleRSA: "default", - PKIRoleEC: "default", - PKIRoleEd25519: "default", - RoleID: "roleID", - SecretID: auth.SecretID{FromEnv: "secretID"}, - AppRole: "auth/approle", - IsWrappingToken: false, - }, - false, - }, { "ok mandatory PKIRole PKIRoleEd25519", - `{"PKIRoleDefault": "role", "PKIRoleEd25519": "ed25519" , "RoleID": "roleID", "SecretID": {"FromEnv": "secretID"}}`, + `{"PKIRoleDefault": "role", "PKIRoleEd25519": "ed25519"}`, &VaultOptions{ - PKI: "pki", - PKIRoleDefault: "role", - PKIRoleRSA: "role", - PKIRoleEC: "role", - PKIRoleEd25519: "ed25519", - RoleID: "roleID", - SecretID: auth.SecretID{FromEnv: "secretID"}, - AppRole: "auth/approle", - IsWrappingToken: false, + PKIMountPath: "pki", + PKIRoleDefault: "role", + PKIRoleRSA: "role", + PKIRoleEC: "role", + PKIRoleEd25519: "ed25519", }, false, }, { "ok mandatory PKIRole PKIRoleEC", - `{"PKIRoleDefault": "role", "PKIRoleEC": "ec" , "RoleID": "roleID", "SecretID": {"FromEnv": "secretID"}}`, + `{"PKIRoleDefault": "role", "PKIRoleEC": "ec"}`, &VaultOptions{ - PKI: "pki", - PKIRoleDefault: "role", - PKIRoleRSA: "role", - PKIRoleEC: "ec", - PKIRoleEd25519: "role", - RoleID: "roleID", - SecretID: auth.SecretID{FromEnv: "secretID"}, - AppRole: "auth/approle", - IsWrappingToken: false, + PKIMountPath: "pki", + PKIRoleDefault: "role", + PKIRoleRSA: "role", + PKIRoleEC: "ec", + PKIRoleEd25519: "role", }, false, }, { "ok mandatory PKIRole PKIRoleRSA", - `{"PKIRoleDefault": "role", "PKIRoleRSA": "rsa" , "RoleID": "roleID", "SecretID": {"FromEnv": "secretID"}}`, + `{"PKIRoleDefault": "role", "PKIRoleRSA": "rsa"}`, &VaultOptions{ - PKI: "pki", - PKIRoleDefault: "role", - PKIRoleRSA: "rsa", - PKIRoleEC: "role", - PKIRoleEd25519: "role", - RoleID: "roleID", - SecretID: auth.SecretID{FromEnv: "secretID"}, - AppRole: "auth/approle", - IsWrappingToken: false, + PKIMountPath: "pki", + PKIRoleDefault: "role", + PKIRoleRSA: "rsa", + PKIRoleEC: "role", + PKIRoleEd25519: "role", }, false, }, { "ok mandatory PKIRoleRSA PKIRoleEC PKIRoleEd25519", - `{"PKIRoleRSA": "rsa", "PKIRoleEC": "ec", "PKIRoleEd25519": "ed25519", "RoleID": "roleID", "SecretID": {"FromEnv": "secretID"}}`, + `{"PKIRoleRSA": "rsa", "PKIRoleEC": "ec", "PKIRoleEd25519": "ed25519"}`, &VaultOptions{ - PKI: "pki", - PKIRoleDefault: "default", - PKIRoleRSA: "rsa", - PKIRoleEC: "ec", - PKIRoleEd25519: "ed25519", - RoleID: "roleID", - SecretID: auth.SecretID{FromEnv: "secretID"}, - AppRole: "auth/approle", - IsWrappingToken: false, + PKIMountPath: "pki", + PKIRoleDefault: "default", + PKIRoleRSA: "rsa", + PKIRoleEC: "ec", + PKIRoleEd25519: "ed25519", }, false, }, { "ok mandatory PKIRoleRSA PKIRoleEC PKIRoleEd25519 with useless PKIRoleDefault", - `{"PKIRoleDefault": "role", "PKIRoleRSA": "rsa", "PKIRoleEC": "ec", "PKIRoleEd25519": "ed25519", "RoleID": "roleID", "SecretID": {"FromEnv": "secretID"}}`, + `{"PKIRoleDefault": "role", "PKIRoleRSA": "rsa", "PKIRoleEC": "ec", "PKIRoleEd25519": "ed25519"}`, &VaultOptions{ - PKI: "pki", - PKIRoleDefault: "role", - PKIRoleRSA: "rsa", - PKIRoleEC: "ec", - PKIRoleEd25519: "ed25519", - RoleID: "roleID", - SecretID: auth.SecretID{FromEnv: "secretID"}, - AppRole: "auth/approle", - IsWrappingToken: false, + PKIMountPath: "pki", + PKIRoleDefault: "role", + PKIRoleRSA: "rsa", + PKIRoleEC: "ec", + PKIRoleEd25519: "ed25519", }, false, }, - { - "ok mandatory with AppRole", - `{"AppRole": "test", "RoleID": "roleID", "SecretID": {"FromString": "secretID"}}`, - &VaultOptions{ - PKI: "pki", - PKIRoleDefault: "default", - PKIRoleRSA: "default", - PKIRoleEC: "default", - PKIRoleEd25519: "default", - RoleID: "roleID", - SecretID: auth.SecretID{FromString: "secretID"}, - AppRole: "test", - IsWrappingToken: false, - }, - false, - }, - { - "ok mandatory with IsWrappingToken", - `{"IsWrappingToken": true, "RoleID": "roleID", "SecretID": {"FromString": "secretID"}}`, - &VaultOptions{ - PKI: "pki", - PKIRoleDefault: "default", - PKIRoleRSA: "default", - PKIRoleEC: "default", - PKIRoleEd25519: "default", - RoleID: "roleID", - SecretID: auth.SecretID{FromString: "secretID"}, - AppRole: "auth/approle", - IsWrappingToken: true, - }, - false, - }, - { - "fail with SecretID FromFail", - `{"RoleID": "roleID", "SecretID": {"FromFail": "secretID"}}`, - nil, - true, - }, - { - "fail with SecretID empty FromEnv", - `{"RoleID": "roleID", "SecretID": {"FromEnv": ""}}`, - nil, - true, - }, - { - "fail with SecretID empty FromFile", - `{"RoleID": "roleID", "SecretID": {"FromFile": ""}}`, - nil, - true, - }, - { - "fail with SecretID empty FromString", - `{"RoleID": "roleID", "SecretID": {"FromString": ""}}`, - nil, - true, - }, - { - "fail mandatory with SecretID FromFail", - `{"RoleID": "roleID", "SecretID": {"FromFail": "secretID"}}`, - nil, - true, - }, - { - "fail missing RoleID", - `{"SecretID": {"FromString": "secretID"}}`, - nil, - true, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 6989c7f146c534df3ebb9fbac5c92e9735fde882 Mon Sep 17 00:00:00 2001 From: Erik De Lamarter Date: Sun, 15 May 2022 17:42:08 +0200 Subject: [PATCH 23/25] vault auth unit tests --- cas/vaultcas/auth/approle/approle.go | 24 ++- cas/vaultcas/auth/approle/approle_test.go | 163 +++++++++++++++++- cas/vaultcas/auth/kubernetes/kubernetes.go | 6 + .../auth/kubernetes/kubernetes_test.go | 140 ++++++++++++++- cas/vaultcas/vaultcas_test.go | 8 - 5 files changed, 321 insertions(+), 20 deletions(-) diff --git a/cas/vaultcas/auth/approle/approle.go b/cas/vaultcas/auth/approle/approle.go index 38d3c51c..d842bae0 100644 --- a/cas/vaultcas/auth/approle/approle.go +++ b/cas/vaultcas/auth/approle/approle.go @@ -2,6 +2,7 @@ package approle import ( "encoding/json" + "errors" "fmt" "github.com/hashicorp/vault/api/auth/approle" @@ -12,6 +13,8 @@ import ( type AuthOptions struct { RoleID string `json:"roleID,omitempty"` SecretID string `json:"secretID,omitempty"` + SecretIDFile string `json:"secretIDFile,omitempty"` + SecretIDEnv string `json:"secretIDEnv,omitempty"` IsWrappingToken bool `json:"isWrappingToken,omitempty"` } @@ -33,8 +36,25 @@ func NewApproleAuthMethod(mountPath string, options json.RawMessage) (*approle.A loginOptions = append(loginOptions, approle.WithWrappingToken()) } - sid := approle.SecretID{ - FromString: opts.SecretID, + if opts.RoleID == "" { + return nil, errors.New("you must set roleID") + } + + var sid approle.SecretID + if opts.SecretID != "" { + sid = approle.SecretID{ + FromString: opts.SecretID, + } + } else if opts.SecretIDFile != "" { + sid = approle.SecretID{ + FromFile: opts.SecretIDFile, + } + } else if opts.SecretIDEnv != "" { + sid = approle.SecretID{ + FromEnv: opts.SecretIDEnv, + } + } else { + return nil, errors.New("you must set one of secretID, secretIDFile or secretIDEnv") } approleAuth, err = approle.NewAppRoleAuth(opts.RoleID, &sid, loginOptions...) diff --git a/cas/vaultcas/auth/approle/approle_test.go b/cas/vaultcas/auth/approle/approle_test.go index ab7e6a97..ec4d523f 100644 --- a/cas/vaultcas/auth/approle/approle_test.go +++ b/cas/vaultcas/auth/approle/approle_test.go @@ -1,16 +1,171 @@ package approle import ( + "context" "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "net/url" "testing" + + vault "github.com/hashicorp/vault/api" ) -func TestKubernetes_NewKubernetesAuthMethod(t *testing.T) { - mountPath := "approle" - raw := `{"roleID": "roleID", "secretID": "secretIDwrapped", "isWrappedToken": true}` +func testCAHelper(t *testing.T) (*url.URL, *vault.Client) { + t.Helper() - _, err := NewApproleAuthMethod(mountPath, json.RawMessage(raw)) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.RequestURI == "/v1/auth/approle/login": + w.WriteHeader(http.StatusOK) + fmt.Fprintf(w, `{ + "auth": { + "client_token": "hvs.0000" + } + }`) + case r.RequestURI == "/v1/auth/custom-approle/login": + w.WriteHeader(http.StatusOK) + fmt.Fprintf(w, `{ + "auth": { + "client_token": "hvs.9999" + } + }`) + default: + w.WriteHeader(http.StatusNotFound) + fmt.Fprintf(w, `{"error":"not found"}`) + } + })) + t.Cleanup(func() { + srv.Close() + }) + u, err := url.Parse(srv.URL) if err != nil { + srv.Close() t.Fatal(err) } + + config := vault.DefaultConfig() + config.Address = srv.URL + + client, err := vault.NewClient(config) + if err != nil { + srv.Close() + t.Fatal(err) + } + + return u, client +} + +func TestApprole_LoginMountPaths(t *testing.T) { + caURL, _ := testCAHelper(t) + + config := vault.DefaultConfig() + config.Address = caURL.String() + client, _ := vault.NewClient(config) + + tests := []struct { + name string + mountPath string + token string + }{ + { + name: "ok default mount path", + mountPath: "", + token: "hvs.0000", + }, + { + name: "ok explicit mount path", + mountPath: "approle", + token: "hvs.0000", + }, + { + name: "ok custom mount path", + mountPath: "custom-approle", + token: "hvs.9999", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + method, err := NewApproleAuthMethod(tt.mountPath, json.RawMessage(`{"RoleID":"roleID","SecretID":"secretID","IsWrappingToken":false}`)) + if err != nil { + t.Errorf("NewApproleAuthMethod() error = %v", err) + return + } + + secret, err := client.Auth().Login(context.Background(), method) + if err != nil { + t.Errorf("Login() error = %v", err) + return + } + + token, _ := secret.TokenID() + if token != tt.token { + t.Errorf("Token error got %v, expected %v", token, tt.token) + return + } + }) + } +} + +func TestApprole_NewApproleAuthMethod(t *testing.T) { + tests := []struct { + name string + mountPath string + raw string + wantErr bool + }{ + { + "ok secret-id string", + "", + `{"RoleID": "0000-0000-0000-0000", "SecretID": "0000-0000-0000-0000"}`, + false, + }, + { + "ok secret-id string and wrapped", + "", + `{"RoleID": "0000-0000-0000-0000", "SecretID": "0000-0000-0000-0000", "isWrappedToken": true}`, + false, + }, + { + "ok secret-id string and wrapped with custom mountPath", + "approle2", + `{"RoleID": "0000-0000-0000-0000", "SecretID": "0000-0000-0000-0000", "isWrappedToken": true}`, + false, + }, + { + "ok secret-id file", + "", + `{"RoleID": "0000-0000-0000-0000", "SecretIDFile": "./secret-id"}`, + false, + }, + { + "ok secret-id env", + "", + `{"RoleID": "0000-0000-0000-0000", "SecretIDEnv": "VAULT_APPROLE_SECRETID"}`, + false, + }, + { + "fail mandatory role-id", + "", + `{}`, + true, + }, + { + "fail mandatory secret-id any", + "", + `{"RoleID": "0000-0000-0000-0000"}`, + true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := NewApproleAuthMethod(tt.mountPath, json.RawMessage(tt.raw)) + if (err != nil) != tt.wantErr { + t.Errorf("Approle.NewApproleAuthMethod() error = %v, wantErr %v", err, tt.wantErr) + return + } + }) + } } diff --git a/cas/vaultcas/auth/kubernetes/kubernetes.go b/cas/vaultcas/auth/kubernetes/kubernetes.go index 0c4db62f..267bcdca 100644 --- a/cas/vaultcas/auth/kubernetes/kubernetes.go +++ b/cas/vaultcas/auth/kubernetes/kubernetes.go @@ -2,6 +2,7 @@ package kubernetes import ( "encoding/json" + "errors" "fmt" "github.com/hashicorp/vault/api/auth/kubernetes" @@ -31,6 +32,11 @@ func NewKubernetesAuthMethod(mountPath string, options json.RawMessage) (*kubern if opts.TokenPath != "" { loginOptions = append(loginOptions, kubernetes.WithServiceAccountTokenPath(opts.TokenPath)) } + + if opts.Role == "" { + return nil, errors.New("you must set role") + } + kubernetesAuth, err = kubernetes.NewKubernetesAuth( opts.Role, loginOptions..., diff --git a/cas/vaultcas/auth/kubernetes/kubernetes_test.go b/cas/vaultcas/auth/kubernetes/kubernetes_test.go index 604f1898..55be904d 100644 --- a/cas/vaultcas/auth/kubernetes/kubernetes_test.go +++ b/cas/vaultcas/auth/kubernetes/kubernetes_test.go @@ -1,21 +1,149 @@ package kubernetes import ( + "context" "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "net/url" "path" "path/filepath" "runtime" "testing" + + vault "github.com/hashicorp/vault/api" ) -func TestKubernetes_NewKubernetesAuthMethod(t *testing.T) { - _, filename, _, _ := runtime.Caller(0) - tokenPath := filepath.Join(path.Dir(filename), "token") - mountPath := "kubernetes" - raw := `{"role": "SomeRoleName", "tokenPath": "` + tokenPath + `"}` +func testCAHelper(t *testing.T) (*url.URL, *vault.Client) { + t.Helper() - _, err := NewKubernetesAuthMethod(mountPath, json.RawMessage(raw)) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.RequestURI == "/v1/auth/kubernetes/login": + w.WriteHeader(http.StatusOK) + fmt.Fprintf(w, `{ + "auth": { + "client_token": "hvs.0000" + } + }`) + case r.RequestURI == "/v1/auth/custom-kubernetes/login": + w.WriteHeader(http.StatusOK) + fmt.Fprintf(w, `{ + "auth": { + "client_token": "hvs.9999" + } + }`) + default: + w.WriteHeader(http.StatusNotFound) + fmt.Fprintf(w, `{"error":"not found"}`) + } + })) + t.Cleanup(func() { + srv.Close() + }) + u, err := url.Parse(srv.URL) if err != nil { + srv.Close() t.Fatal(err) } + + config := vault.DefaultConfig() + config.Address = srv.URL + + client, err := vault.NewClient(config) + if err != nil { + srv.Close() + t.Fatal(err) + } + + return u, client +} + +func TestApprole_LoginMountPaths(t *testing.T) { + caURL, _ := testCAHelper(t) + _, filename, _, _ := runtime.Caller(0) + tokenPath := filepath.Join(path.Dir(filename), "token") + + config := vault.DefaultConfig() + config.Address = caURL.String() + client, _ := vault.NewClient(config) + + tests := []struct { + name string + mountPath string + token string + }{ + { + name: "ok default mount path", + mountPath: "", + token: "hvs.0000", + }, + { + name: "ok explicit mount path", + mountPath: "kubernetes", + token: "hvs.0000", + }, + { + name: "ok custom mount path", + mountPath: "custom-kubernetes", + token: "hvs.9999", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + method, err := NewKubernetesAuthMethod(tt.mountPath, json.RawMessage(`{"role": "SomeRoleName", "tokenPath": "`+tokenPath+`"}`)) + if err != nil { + t.Errorf("NewApproleAuthMethod() error = %v", err) + return + } + + secret, err := client.Auth().Login(context.Background(), method) + if err != nil { + t.Errorf("Login() error = %v", err) + return + } + + token, _ := secret.TokenID() + if token != tt.token { + t.Errorf("Token error got %v, expected %v", token, tt.token) + return + } + }) + } +} + +func TestApprole_NewApproleAuthMethod(t *testing.T) { + _, filename, _, _ := runtime.Caller(0) + tokenPath := filepath.Join(path.Dir(filename), "token") + + tests := []struct { + name string + mountPath string + raw string + wantErr bool + }{ + { + "ok secret-id string", + "", + `{"role": "SomeRoleName", "tokenPath": "` + tokenPath + `"}`, + false, + }, + { + "fail mandatory role", + "", + `{}`, + true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := NewKubernetesAuthMethod(tt.mountPath, json.RawMessage(tt.raw)) + if (err != nil) != tt.wantErr { + t.Errorf("Kubernetes.NewKubernetesAuthMethod() error = %v, wantErr %v", err, tt.wantErr) + return + } + }) + } } diff --git a/cas/vaultcas/vaultcas_test.go b/cas/vaultcas/vaultcas_test.go index 3c1f09a3..0ea0c4b1 100644 --- a/cas/vaultcas/vaultcas_test.go +++ b/cas/vaultcas/vaultcas_test.go @@ -182,8 +182,6 @@ func TestNew_register(t *testing.T) { CertificateAuthority: caURL.String(), CertificateAuthorityFingerprint: testRootFingerprint, Config: json.RawMessage(`{ - "PKIMountPath": "pki", - "PKIRoleDefault": "pki-role", "AuthType": "approle", "AuthOptions": {"RoleID":"roleID","SecretID":"secretID","IsWrappingToken":false} }`), @@ -204,8 +202,6 @@ func TestVaultCAS_CreateCertificate(t *testing.T) { PKIRoleRSA: "rsa", PKIRoleEC: "ec", PKIRoleEd25519: "ed25519", - AuthType: "approle", - AuthOptions: json.RawMessage(`{"RoleID":"roleID","SecretID":"secretID","IsWrappingToken":false}`), } type fields struct { @@ -336,8 +332,6 @@ func TestVaultCAS_RevokeCertificate(t *testing.T) { PKIRoleRSA: "rsa", PKIRoleEC: "ec", PKIRoleEd25519: "ed25519", - AuthType: "approle", - AuthOptions: json.RawMessage(`{"RoleID":"roleID","SecretID":"secretID","IsWrappingToken":false}`), } type fields struct { @@ -406,8 +400,6 @@ func TestVaultCAS_RenewCertificate(t *testing.T) { PKIRoleRSA: "rsa", PKIRoleEC: "ec", PKIRoleEd25519: "ed25519", - AuthType: "approle", - AuthOptions: json.RawMessage(`{"RoleID":"roleID","SecretID":"secretID","IsWrappingToken":false}`), } type fields struct { From 9ec154aab02f25fad6ee47a545a8250ed76e3345 Mon Sep 17 00:00:00 2001 From: Erik De Lamarter Date: Tue, 17 May 2022 22:13:11 +0200 Subject: [PATCH 24/25] rewrite and improve secret-id config --- cas/vaultcas/auth/approle/approle.go | 9 +++++---- cas/vaultcas/auth/approle/approle_test.go | 24 +++++++++++++++++++++++ 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/cas/vaultcas/auth/approle/approle.go b/cas/vaultcas/auth/approle/approle.go index d842bae0..118afb10 100644 --- a/cas/vaultcas/auth/approle/approle.go +++ b/cas/vaultcas/auth/approle/approle.go @@ -41,19 +41,20 @@ func NewApproleAuthMethod(mountPath string, options json.RawMessage) (*approle.A } var sid approle.SecretID - if opts.SecretID != "" { + switch { + case opts.SecretID != "" && opts.SecretIDFile == "" && opts.SecretIDEnv == "": sid = approle.SecretID{ FromString: opts.SecretID, } - } else if opts.SecretIDFile != "" { + case opts.SecretIDFile != "" && opts.SecretID == "" && opts.SecretIDEnv == "": sid = approle.SecretID{ FromFile: opts.SecretIDFile, } - } else if opts.SecretIDEnv != "" { + case opts.SecretIDEnv != "" && opts.SecretIDFile == "" && opts.SecretID == "": sid = approle.SecretID{ FromEnv: opts.SecretIDEnv, } - } else { + default: return nil, errors.New("you must set one of secretID, secretIDFile or secretIDEnv") } diff --git a/cas/vaultcas/auth/approle/approle_test.go b/cas/vaultcas/auth/approle/approle_test.go index ec4d523f..28b7b7f7 100644 --- a/cas/vaultcas/auth/approle/approle_test.go +++ b/cas/vaultcas/auth/approle/approle_test.go @@ -158,6 +158,30 @@ func TestApprole_NewApproleAuthMethod(t *testing.T) { `{"RoleID": "0000-0000-0000-0000"}`, true, }, + { + "fail multiple secret-id types id and env", + "", + `{"RoleID": "0000-0000-0000-0000", "SecretID": "0000-0000-0000-0000", "SecretIDEnv": "VAULT_APPROLE_SECRETID"}`, + true, + }, + { + "fail multiple secret-id types id and file", + "", + `{"RoleID": "0000-0000-0000-0000", "SecretID": "0000-0000-0000-0000", "SecretIDFile": "./secret-id"}`, + true, + }, + { + "fail multiple secret-id types env and file", + "", + `{"RoleID": "0000-0000-0000-0000", "SecretIDFile": "./secret-id", "SecretIDEnv": "VAULT_APPROLE_SECRETID"}`, + true, + }, + { + "fail multiple secret-id types all", + "", + `{"RoleID": "0000-0000-0000-0000", "SecretID": "0000-0000-0000-0000", "SecretIDFile": "./secret-id", "SecretIDEnv": "VAULT_APPROLE_SECRETID"}`, + true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 07984a968fba1eedfa514d80e088a41e1f59651f Mon Sep 17 00:00:00 2001 From: Erik DeLamarter Date: Sat, 21 May 2022 21:00:50 +0200 Subject: [PATCH 25/25] better error messages Co-authored-by: Mariano Cano --- cas/vaultcas/vaultcas.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cas/vaultcas/vaultcas.go b/cas/vaultcas/vaultcas.go index 02c814b7..a5658620 100644 --- a/cas/vaultcas/vaultcas.go +++ b/cas/vaultcas/vaultcas.go @@ -84,15 +84,15 @@ func New(ctx context.Context, opts apiv1.Options) (*VaultCAS, error) { case "approle": method, err = approle.NewApproleAuthMethod(vc.AuthMountPath, vc.AuthOptions) default: - return nil, fmt.Errorf("unknown auth type: %v", vc.AuthType) + return nil, fmt.Errorf("unknown auth type: %s, only 'kubernetes' and 'approle' currently supported", vc.AuthType) } if err != nil { - return nil, fmt.Errorf("unable to configure auth method: %w", err) + return nil, fmt.Errorf("unable to configure %s auth method: %w", vc.AuthType, err) } authInfo, err := client.Auth().Login(ctx, method) if err != nil { - return nil, fmt.Errorf("unable to login to Kubernetes auth method: %w", err) + return nil, fmt.Errorf("unable to login to %s auth method: %w", vc.AuthType, err) } if authInfo == nil { return nil, errors.New("no auth info was returned after login")