From 7378ed27ac0ccc1b0c0c23a8edd0f882592b409d Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 19 Mar 2019 15:10:52 -0700 Subject: [PATCH 1/3] Refactor claims so they can be totally omitted if only the parent is set. --- authority/config.go | 7 +-- authority/provisioner/claims.go | 80 +++++++++++++++++------------ authority/provisioner/jwk.go | 14 +++-- authority/provisioner/jwk_test.go | 7 ++- authority/provisioner/oidc.go | 14 ++--- authority/provisioner/oidc_test.go | 7 ++- authority/provisioner/utils_test.go | 10 ++++ 7 files changed, 85 insertions(+), 54 deletions(-) diff --git a/authority/config.go b/authority/config.go index 406fd437..d05ec519 100644 --- a/authority/config.go +++ b/authority/config.go @@ -60,7 +60,6 @@ type AuthConfig struct { // Validate validates the authority configuration. func (c *AuthConfig) Validate(audiences []string) error { - var err error if c == nil { return errors.New("authority cannot be undefined") } @@ -68,13 +67,15 @@ func (c *AuthConfig) Validate(audiences []string) error { return errors.New("authority.provisioners cannot be empty") } - if c.Claims, err = c.Claims.Init(&globalProvisionerClaims); err != nil { + // Merge global and configuration claims + claimer, err := provisioner.NewClaimer(c.Claims, globalProvisionerClaims) + if err != nil { return err } // Initialize provisioners config := provisioner.Config{ - Claims: *c.Claims, + Claims: claimer.Claims(), Audiences: audiences, } for _, p := range c.Provisioners { diff --git a/authority/provisioner/claims.go b/authority/provisioner/claims.go index 2fc68397..1109e0c7 100644 --- a/authority/provisioner/claims.go +++ b/authority/provisioner/claims.go @@ -8,76 +8,90 @@ import ( // Claims so that individual provisioners can override global claims. type Claims struct { - globalClaims *Claims MinTLSDur *Duration `json:"minTLSCertDuration,omitempty"` MaxTLSDur *Duration `json:"maxTLSCertDuration,omitempty"` DefaultTLSDur *Duration `json:"defaultTLSCertDuration,omitempty"` DisableRenewal *bool `json:"disableRenewal,omitempty"` } -// Init initializes and validates the individual provisioner claims. -func (pc *Claims) Init(global *Claims) (*Claims, error) { - if pc == nil { - pc = &Claims{} +// Claimer is the type that controls claims. It provides an interface around the +// current claim and the global one. +type Claimer struct { + global Claims + claims *Claims +} + +// NewClaimer initializes a new claimer with the given claims. +func NewClaimer(claims *Claims, global Claims) (*Claimer, error) { + c := &Claimer{global: global, claims: claims} + return c, c.Validate() +} + +// Claims returns the merge of the inner and global claims. +func (c *Claimer) Claims() Claims { + disableRenewal := c.IsDisableRenewal() + return Claims{ + MinTLSDur: &Duration{c.MinTLSCertDuration()}, + MaxTLSDur: &Duration{c.MaxTLSCertDuration()}, + DefaultTLSDur: &Duration{c.DefaultTLSCertDuration()}, + DisableRenewal: &disableRenewal, } - pc.globalClaims = global - return pc, pc.Validate() } // DefaultTLSCertDuration returns the default TLS cert duration for the // provisioner. If the default is not set within the provisioner, then the global // default from the authority configuration will be used. -func (pc *Claims) DefaultTLSCertDuration() time.Duration { - if pc.DefaultTLSDur == nil || pc.DefaultTLSDur.Duration == 0 { - return pc.globalClaims.DefaultTLSCertDuration() +func (c *Claimer) DefaultTLSCertDuration() time.Duration { + if c.claims == nil || c.claims.DefaultTLSDur == nil { + return c.global.DefaultTLSDur.Duration } - return pc.DefaultTLSDur.Duration + return c.claims.DefaultTLSDur.Duration } // MinTLSCertDuration returns the minimum TLS cert duration for the provisioner. // If the minimum is not set within the provisioner, then the global // minimum from the authority configuration will be used. -func (pc *Claims) MinTLSCertDuration() time.Duration { - if pc.MinTLSDur == nil || pc.MinTLSDur.Duration == 0 { - return pc.globalClaims.MinTLSCertDuration() +func (c *Claimer) MinTLSCertDuration() time.Duration { + if c.claims == nil || c.claims.MinTLSDur == nil { + return c.global.MinTLSDur.Duration } - return pc.MinTLSDur.Duration + return c.claims.MinTLSDur.Duration } // MaxTLSCertDuration returns the maximum TLS cert duration for the provisioner. // If the maximum is not set within the provisioner, then the global // maximum from the authority configuration will be used. -func (pc *Claims) MaxTLSCertDuration() time.Duration { - if pc.MaxTLSDur == nil || pc.MaxTLSDur.Duration == 0 { - return pc.globalClaims.MaxTLSCertDuration() +func (c *Claimer) MaxTLSCertDuration() time.Duration { + if c.claims == nil || c.claims.MaxTLSDur == nil { + return c.global.MaxTLSDur.Duration } - return pc.MaxTLSDur.Duration + return c.claims.MaxTLSDur.Duration } // IsDisableRenewal returns if the renewal flow is disabled for the // provisioner. If the property is not set within the provisioner, then the // global value from the authority configuration will be used. -func (pc *Claims) IsDisableRenewal() bool { - if pc.DisableRenewal == nil { - return pc.globalClaims.IsDisableRenewal() +func (c *Claimer) IsDisableRenewal() bool { + if c.claims == nil || c.claims.DisableRenewal == nil { + return *c.global.DisableRenewal } - return *pc.DisableRenewal + return *c.claims.DisableRenewal } // Validate validates and modifies the Claims with default values. -func (pc *Claims) Validate() error { +func (c *Claimer) Validate() error { var ( - min = pc.MinTLSCertDuration() - max = pc.MaxTLSCertDuration() - def = pc.DefaultTLSCertDuration() + min = c.MinTLSCertDuration() + max = c.MaxTLSCertDuration() + def = c.DefaultTLSCertDuration() ) switch { - case min == 0: - return errors.Errorf("claims: MinTLSCertDuration cannot be empty") - case max == 0: - return errors.Errorf("claims: MaxTLSCertDuration cannot be empty") - case def == 0: - return errors.Errorf("claims: DefaultTLSCertDuration cannot be empty") + case min <= 0: + return errors.Errorf("claims: MinTLSCertDuration must be greater than 0") + case max <= 0: + return errors.Errorf("claims: MaxTLSCertDuration must be greater than 0") + case def <= 0: + return errors.Errorf("claims: DefaultTLSCertDuration must be greater than 0") case max < min: return errors.Errorf("claims: MaxCertDuration cannot be less "+ "than MinCertDuration: MaxCertDuration - %v, MinCertDuration - %v", max, min) diff --git a/authority/provisioner/jwk.go b/authority/provisioner/jwk.go index 4575ca47..6c1813c5 100644 --- a/authority/provisioner/jwk.go +++ b/authority/provisioner/jwk.go @@ -23,6 +23,7 @@ type JWK struct { Key *jose.JSONWebKey `json:"key"` EncryptedKey string `json:"encryptedKey,omitempty"` Claims *Claims `json:"claims,omitempty"` + claimer *Claimer audiences []string } @@ -57,7 +58,12 @@ func (p *JWK) Init(config Config) (err error) { case p.Key == nil: return errors.New("provisioner key cannot be empty") } - p.Claims, err = p.Claims.Init(&config.Claims) + + // Update claims with global ones + if p.claimer, err = NewClaimer(p.Claims, config.Claims); err != nil { + return err + } + p.audiences = config.Audiences return err } @@ -104,15 +110,15 @@ func (p *JWK) Authorize(token string) ([]SignOption, error) { commonNameValidator(claims.Subject), dnsNamesValidator(dnsNames), ipAddressesValidator(ips), - profileDefaultDuration(p.Claims.DefaultTLSCertDuration()), + profileDefaultDuration(p.claimer.DefaultTLSCertDuration()), newProvisionerExtensionOption(TypeJWK, p.Name, p.Key.KeyID), - newValidityValidator(p.Claims.MinTLSCertDuration(), p.Claims.MaxTLSCertDuration()), + newValidityValidator(p.claimer.MinTLSCertDuration(), p.claimer.MaxTLSCertDuration()), }, nil } // AuthorizeRenewal returns an error if the renewal is disabled. func (p *JWK) AuthorizeRenewal(cert *x509.Certificate) error { - if p.Claims.IsDisableRenewal() { + if p.claimer.IsDisableRenewal() { return errors.Errorf("renew is disabled for provisioner %s", p.GetID()) } return nil diff --git a/authority/provisioner/jwk_test.go b/authority/provisioner/jwk_test.go index 53260313..f2ddd94d 100644 --- a/authority/provisioner/jwk_test.go +++ b/authority/provisioner/jwk_test.go @@ -201,10 +201,9 @@ func TestJWK_AuthorizeRenewal(t *testing.T) { // disable renewal disable := true - p2.Claims = &Claims{ - globalClaims: &globalProvisionerClaims, - DisableRenewal: &disable, - } + p2.Claims = &Claims{DisableRenewal: &disable} + p2.claimer, err = NewClaimer(p2.Claims, globalProvisionerClaims) + assert.FatalError(t, err) type args struct { cert *x509.Certificate diff --git a/authority/provisioner/oidc.go b/authority/provisioner/oidc.go index 831a6e8b..76d154a3 100644 --- a/authority/provisioner/oidc.go +++ b/authority/provisioner/oidc.go @@ -55,6 +55,7 @@ type OIDC struct { Claims *Claims `json:"claims,omitempty"` configuration openIDConfiguration keyStore *keyStore + claimer *Claimer } // IsAdmin returns true if the given email is in the Admins whitelist, false @@ -111,9 +112,10 @@ func (o *OIDC) Init(config Config) (err error) { } // Update claims with global ones - if o.Claims, err = o.Claims.Init(&config.Claims); err != nil { + if o.claimer, err = NewClaimer(o.Claims, config.Claims); err != nil { return err } + // Decode and validate openid-configuration endpoint if err := getAndDecode(o.ConfigurationEndpoint, &o.configuration); err != nil { return err @@ -202,23 +204,23 @@ func (o *OIDC) Authorize(token string) ([]SignOption, error) { // Admins should be able to authorize any SAN if o.IsAdmin(claims.Email) { return []SignOption{ - profileDefaultDuration(o.Claims.DefaultTLSCertDuration()), + profileDefaultDuration(o.claimer.DefaultTLSCertDuration()), newProvisionerExtensionOption(TypeOIDC, o.Name, o.ClientID), - newValidityValidator(o.Claims.MinTLSCertDuration(), o.Claims.MaxTLSCertDuration()), + newValidityValidator(o.claimer.MinTLSCertDuration(), o.claimer.MaxTLSCertDuration()), }, nil } return []SignOption{ emailOnlyIdentity(claims.Email), - profileDefaultDuration(o.Claims.DefaultTLSCertDuration()), + profileDefaultDuration(o.claimer.DefaultTLSCertDuration()), newProvisionerExtensionOption(TypeOIDC, o.Name, o.ClientID), - newValidityValidator(o.Claims.MinTLSCertDuration(), o.Claims.MaxTLSCertDuration()), + newValidityValidator(o.claimer.MinTLSCertDuration(), o.claimer.MaxTLSCertDuration()), }, nil } // AuthorizeRenewal returns an error if the renewal is disabled. func (o *OIDC) AuthorizeRenewal(cert *x509.Certificate) error { - if o.Claims.IsDisableRenewal() { + if o.claimer.IsDisableRenewal() { return errors.Errorf("renew is disabled for provisioner %s", o.GetID()) } return nil diff --git a/authority/provisioner/oidc_test.go b/authority/provisioner/oidc_test.go index eddf27fa..39dcbe29 100644 --- a/authority/provisioner/oidc_test.go +++ b/authority/provisioner/oidc_test.go @@ -241,10 +241,9 @@ func TestOIDC_AuthorizeRenewal(t *testing.T) { // disable renewal disable := true - p2.Claims = &Claims{ - globalClaims: &globalProvisionerClaims, - DisableRenewal: &disable, - } + p2.Claims = &Claims{DisableRenewal: &disable} + p2.claimer, err = NewClaimer(p2.Claims, globalProvisionerClaims) + assert.FatalError(t, err) type args struct { cert *x509.Certificate diff --git a/authority/provisioner/utils_test.go b/authority/provisioner/utils_test.go index e53b42d3..04d06f10 100644 --- a/authority/provisioner/utils_test.go +++ b/authority/provisioner/utils_test.go @@ -109,6 +109,10 @@ func generateJWK() (*JWK, error) { if err != nil { return nil, err } + claimer, err := NewClaimer(nil, globalProvisionerClaims) + if err != nil { + return nil, err + } return &JWK{ Name: name, Type: "JWK", @@ -116,6 +120,7 @@ func generateJWK() (*JWK, error) { EncryptedKey: encrypted, Claims: &globalProvisionerClaims, audiences: testAudiences, + claimer: claimer, }, nil } @@ -136,6 +141,10 @@ func generateOIDC() (*OIDC, error) { if err != nil { return nil, err } + claimer, err := NewClaimer(nil, globalProvisionerClaims) + if err != nil { + return nil, err + } return &OIDC{ Name: name, Type: "OIDC", @@ -150,6 +159,7 @@ func generateOIDC() (*OIDC, error) { keySet: jose.JSONWebKeySet{Keys: []jose.JSONWebKey{*jwk}}, expiry: time.Now().Add(24 * time.Hour), }, + claimer: claimer, }, nil } From 76618558ae93c610f3f713153da71c57397f292b Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 19 Mar 2019 15:27:41 -0700 Subject: [PATCH 2/3] Improve unit tests. --- authority/provisioner/jwk_test.go | 6 ++++++ authority/provisioner/oidc_test.go | 4 ++++ 2 files changed, 10 insertions(+) diff --git a/authority/provisioner/jwk_test.go b/authority/provisioner/jwk_test.go index f2ddd94d..7a2dc50c 100644 --- a/authority/provisioner/jwk_test.go +++ b/authority/provisioner/jwk_test.go @@ -78,6 +78,12 @@ func TestJWK_Init(t *testing.T) { err: errors.New("provisioner key cannot be empty"), } }, + "fail-bad-claims": func(t *testing.T) ProvisionerValidateTest { + return ProvisionerValidateTest{ + p: &JWK{Name: "foo", Type: "bar", Key: &jose.JSONWebKey{}, audiences: testAudiences, Claims: &Claims{DefaultTLSDur: &Duration{0}}}, + err: errors.New("claims: DefaultTLSCertDuration must be greater than 0"), + } + }, "ok": func(t *testing.T) ProvisionerValidateTest { return ProvisionerValidateTest{ p: &JWK{Name: "foo", Type: "bar", Key: &jose.JSONWebKey{}, audiences: testAudiences}, diff --git a/authority/provisioner/oidc_test.go b/authority/provisioner/oidc_test.go index 39dcbe29..b70f44e4 100644 --- a/authority/provisioner/oidc_test.go +++ b/authority/provisioner/oidc_test.go @@ -64,6 +64,9 @@ func TestOIDC_Init(t *testing.T) { config := Config{ Claims: globalProvisionerClaims, } + badClaims := &Claims{ + DefaultTLSDur: &Duration{0}, + } type fields struct { Type string @@ -93,6 +96,7 @@ func TestOIDC_Init(t *testing.T) { {"no-client-id", fields{"oidc", "name", "", "client-secret", srv.URL + "/openid-configuration", nil, nil, nil}, args{config}, true}, {"no-configuration", fields{"oidc", "name", "client-id", "client-secret", "", nil, nil, nil}, args{config}, true}, {"bad-configuration", fields{"oidc", "name", "client-id", "client-secret", srv.URL, nil, nil, nil}, args{config}, true}, + {"bad-claims", fields{"oidc", "name", "client-id", "client-secret", srv.URL + "/openid-configuration", badClaims, nil, nil}, args{config}, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 68ff077ea9e8893ce7d02fad8687fb06687e0f5e Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 19 Mar 2019 15:31:14 -0700 Subject: [PATCH 3/3] Improve tests. --- authority/config_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/authority/config_test.go b/authority/config_test.go index ca5de829..b5a57998 100644 --- a/authority/config_test.go +++ b/authority/config_test.go @@ -272,6 +272,17 @@ func TestAuthConfigValidate(t *testing.T) { err: errors.New("provisioner type cannot be empty"), } }, + "fail-invalid-claims": func(t *testing.T) AuthConfigValidateTest { + return AuthConfigValidateTest{ + ac: &AuthConfig{ + Provisioners: p, + Claims: &provisioner.Claims{ + MinTLSDur: &provisioner.Duration{-1}, + }, + }, + err: errors.New("claims: MinTLSCertDuration must be greater than 0"), + } + }, "ok-empty-asn1dn-template": func(t *testing.T) AuthConfigValidateTest { return AuthConfigValidateTest{ ac: &AuthConfig{