diff --git a/authority/provisioner/aws.go b/authority/provisioner/aws.go index f63b9ced..2ff8ade9 100644 --- a/authority/provisioner/aws.go +++ b/authority/provisioner/aws.go @@ -18,7 +18,6 @@ import ( "github.com/pkg/errors" "github.com/smallstep/certificates/errs" - "github.com/smallstep/certificates/policy" "go.step.sm/crypto/jose" "go.step.sm/crypto/sshutil" "go.step.sm/crypto/x509util" @@ -268,8 +267,8 @@ type AWS struct { claimer *Claimer config *awsConfig audiences Audiences - x509Policy policy.X509NamePolicyEngine - sshPolicy policy.SSHNamePolicyEngine + x509Policy x509PolicyEngine + sshHostPolicy *hostPolicyEngine } // GetID returns the provisioner unique identifier. @@ -433,8 +432,8 @@ func (p *AWS) Init(config Config) (err error) { return err } - // Initialize the SSH allow/deny policy engine - if p.sshPolicy, err = newSSHPolicyEngine(p.Options.GetSSHOptions()); err != nil { + // Initialize the SSH allow/deny policy engine for host certificates + if p.sshHostPolicy, err = newSSHHostPolicyEngine(p.Options.GetSSHOptions()); err != nil { return err } @@ -774,6 +773,6 @@ func (p *AWS) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption, // Require all the fields in the SSH certificate &sshCertDefaultValidator{}, // Ensure that all principal names are allowed - newSSHNamePolicyValidator(p.sshPolicy), + newSSHNamePolicyValidator(p.sshHostPolicy, nil), ), nil } diff --git a/authority/provisioner/azure.go b/authority/provisioner/azure.go index 5ccdc06b..40b7d3f5 100644 --- a/authority/provisioner/azure.go +++ b/authority/provisioner/azure.go @@ -14,7 +14,6 @@ import ( "github.com/pkg/errors" "github.com/smallstep/certificates/errs" - "github.com/smallstep/certificates/policy" "go.step.sm/crypto/jose" "go.step.sm/crypto/sshutil" "go.step.sm/crypto/x509util" @@ -99,8 +98,8 @@ type Azure struct { config *azureConfig oidcConfig openIDConfiguration keyStore *keyStore - x509Policy policy.X509NamePolicyEngine - sshPolicy policy.SSHNamePolicyEngine + x509Policy x509PolicyEngine + sshHostPolicy *hostPolicyEngine } // GetID returns the provisioner unique identifier. @@ -229,8 +228,8 @@ func (p *Azure) Init(config Config) (err error) { return err } - // Initialize the SSH allow/deny policy engine - if p.sshPolicy, err = newSSHPolicyEngine(p.Options.GetSSHOptions()); err != nil { + // Initialize the SSH allow/deny policy engine for host certificates + if p.sshHostPolicy, err = newSSHHostPolicyEngine(p.Options.GetSSHOptions()); err != nil { return err } @@ -411,7 +410,7 @@ func (p *Azure) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOptio // Require all the fields in the SSH certificate &sshCertDefaultValidator{}, // Ensure that all principal names are allowed - newSSHNamePolicyValidator(p.sshPolicy), + newSSHNamePolicyValidator(p.sshHostPolicy, nil), ), nil } diff --git a/authority/provisioner/gcp.go b/authority/provisioner/gcp.go index 590c32e2..e56c0729 100644 --- a/authority/provisioner/gcp.go +++ b/authority/provisioner/gcp.go @@ -15,7 +15,6 @@ import ( "github.com/pkg/errors" "github.com/smallstep/certificates/errs" - "github.com/smallstep/certificates/policy" "go.step.sm/crypto/jose" "go.step.sm/crypto/sshutil" "go.step.sm/crypto/x509util" @@ -93,8 +92,8 @@ type GCP struct { config *gcpConfig keyStore *keyStore audiences Audiences - x509Policy policy.X509NamePolicyEngine - sshPolicy policy.SSHNamePolicyEngine + x509Policy x509PolicyEngine + sshHostPolicy *hostPolicyEngine } // GetID returns the provisioner unique identifier. The name should uniquely @@ -224,8 +223,8 @@ func (p *GCP) Init(config Config) error { return err } - // Initialize the SSH allow/deny policy engine - if p.sshPolicy, err = newSSHPolicyEngine(p.Options.GetSSHOptions()); err != nil { + // Initialize the SSH allow/deny policy engine for host certificates + if p.sshHostPolicy, err = newSSHHostPolicyEngine(p.Options.GetSSHOptions()); err != nil { return err } @@ -453,6 +452,6 @@ func (p *GCP) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption, // Require all the fields in the SSH certificate &sshCertDefaultValidator{}, // Ensure that all principal names are allowed - newSSHNamePolicyValidator(p.sshPolicy), + newSSHNamePolicyValidator(p.sshHostPolicy, nil), ), nil } diff --git a/authority/provisioner/jwk.go b/authority/provisioner/jwk.go index 081fbb90..a129a536 100644 --- a/authority/provisioner/jwk.go +++ b/authority/provisioner/jwk.go @@ -8,7 +8,6 @@ import ( "github.com/pkg/errors" "github.com/smallstep/certificates/errs" - "github.com/smallstep/certificates/policy" "go.step.sm/crypto/jose" "go.step.sm/crypto/sshutil" "go.step.sm/crypto/x509util" @@ -29,17 +28,18 @@ type stepPayload struct { // signature requests. type JWK struct { *base - ID string `json:"-"` - Type string `json:"type"` - Name string `json:"name"` - Key *jose.JSONWebKey `json:"key"` - EncryptedKey string `json:"encryptedKey,omitempty"` - Claims *Claims `json:"claims,omitempty"` - Options *Options `json:"options,omitempty"` - claimer *Claimer - audiences Audiences - x509Policy policy.X509NamePolicyEngine - sshPolicy policy.SSHNamePolicyEngine + ID string `json:"-"` + Type string `json:"type"` + Name string `json:"name"` + Key *jose.JSONWebKey `json:"key"` + EncryptedKey string `json:"encryptedKey,omitempty"` + Claims *Claims `json:"claims,omitempty"` + Options *Options `json:"options,omitempty"` + claimer *Claimer + audiences Audiences + x509Policy x509PolicyEngine + sshHostPolicy *hostPolicyEngine + sshUserPolicy *userPolicyEngine } // GetID returns the provisioner unique identifier. The name and credential id @@ -111,8 +111,13 @@ func (p *JWK) Init(config Config) (err error) { return err } - // Initialize the SSH allow/deny policy engine - if p.sshPolicy, err = newSSHPolicyEngine(p.Options.GetSSHOptions()); err != nil { + // Initialize the SSH allow/deny policy engine for user certificates + if p.sshUserPolicy, err = newSSHUserPolicyEngine(p.Options.GetSSHOptions()); err != nil { + return err + } + + // Initialize the SSH allow/deny policy engine for host certificates + if p.sshHostPolicy, err = newSSHHostPolicyEngine(p.Options.GetSSHOptions()); err != nil { return err } @@ -294,7 +299,7 @@ func (p *JWK) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption, // Require and validate all the default fields in the SSH certificate. &sshCertDefaultValidator{}, // Ensure that all principal names are allowed - newSSHNamePolicyValidator(p.sshPolicy), + newSSHNamePolicyValidator(p.sshHostPolicy, p.sshUserPolicy), ), nil } diff --git a/authority/provisioner/k8sSA.go b/authority/provisioner/k8sSA.go index d52f0d12..be55f114 100644 --- a/authority/provisioner/k8sSA.go +++ b/authority/provisioner/k8sSA.go @@ -11,7 +11,6 @@ import ( "github.com/pkg/errors" "github.com/smallstep/certificates/errs" - "github.com/smallstep/certificates/policy" "go.step.sm/crypto/jose" "go.step.sm/crypto/pemutil" "go.step.sm/crypto/sshutil" @@ -52,9 +51,10 @@ type K8sSA struct { claimer *Claimer audiences Audiences //kauthn kauthn.AuthenticationV1Interface - pubKeys []interface{} - x509Policy policy.X509NamePolicyEngine - sshPolicy policy.SSHNamePolicyEngine + pubKeys []interface{} + x509Policy x509PolicyEngine + sshHostPolicy *hostPolicyEngine + sshUserPolicy *userPolicyEngine } // GetID returns the provisioner unique identifier. The name and credential id @@ -152,8 +152,13 @@ func (p *K8sSA) Init(config Config) (err error) { return err } - // Initialize the SSH allow/deny policy engine - if p.sshPolicy, err = newSSHPolicyEngine(p.Options.GetSSHOptions()); err != nil { + // Initialize the SSH allow/deny policy engine for user certificates + if p.sshUserPolicy, err = newSSHUserPolicyEngine(p.Options.GetSSHOptions()); err != nil { + return err + } + + // Initialize the SSH allow/deny policy engine for host certificates + if p.sshHostPolicy, err = newSSHHostPolicyEngine(p.Options.GetSSHOptions()); err != nil { return err } @@ -305,7 +310,7 @@ func (p *K8sSA) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOptio // Require and validate all the default fields in the SSH certificate. &sshCertDefaultValidator{}, // Ensure that all principal names are allowed - newSSHNamePolicyValidator(p.sshPolicy), + newSSHNamePolicyValidator(p.sshHostPolicy, p.sshUserPolicy), ), nil } diff --git a/authority/provisioner/k8sSA_test.go b/authority/provisioner/k8sSA_test.go index 3ccce461..c63a32bc 100644 --- a/authority/provisioner/k8sSA_test.go +++ b/authority/provisioner/k8sSA_test.go @@ -371,7 +371,8 @@ func TestK8sSA_AuthorizeSSHSign(t *testing.T) { case *sshDefaultDuration: assert.Equals(t, v.Claimer, tc.p.claimer) case *sshNamePolicyValidator: - assert.Equals(t, nil, v.policyEngine) + assert.Equals(t, nil, v.userPolicyEngine) + assert.Equals(t, nil, v.hostPolicyEngine) default: assert.FatalError(t, errors.Errorf("unexpected sign option of type %T", v)) } diff --git a/authority/provisioner/nebula.go b/authority/provisioner/nebula.go index 6c31dbc5..39980cc8 100644 --- a/authority/provisioner/nebula.go +++ b/authority/provisioner/nebula.go @@ -11,7 +11,6 @@ import ( "github.com/pkg/errors" nebula "github.com/slackhq/nebula/cert" "github.com/smallstep/certificates/errs" - "github.com/smallstep/certificates/policy" "go.step.sm/crypto/jose" "go.step.sm/crypto/sshutil" "go.step.sm/crypto/x25519" @@ -35,17 +34,18 @@ const ( // https://signal.org/docs/specifications/xeddsa/#xeddsa and implemented by // go.step.sm/crypto/x25519. type Nebula struct { - ID string `json:"-"` - Type string `json:"type"` - Name string `json:"name"` - Roots []byte `json:"roots"` - Claims *Claims `json:"claims,omitempty"` - Options *Options `json:"options,omitempty"` - claimer *Claimer - caPool *nebula.NebulaCAPool - audiences Audiences - x509Policy policy.X509NamePolicyEngine - sshPolicy policy.SSHNamePolicyEngine + ID string `json:"-"` + Type string `json:"type"` + Name string `json:"name"` + Roots []byte `json:"roots"` + Claims *Claims `json:"claims,omitempty"` + Options *Options `json:"options,omitempty"` + claimer *Claimer + caPool *nebula.NebulaCAPool + audiences Audiences + x509Policy x509PolicyEngine + sshHostPolicy *hostPolicyEngine + sshUserPolicy *userPolicyEngine } // Init verifies and initializes the Nebula provisioner. @@ -76,8 +76,13 @@ func (p *Nebula) Init(config Config) error { return err } - // Initialize the SSH allow/deny policy engine - if p.sshPolicy, err = newSSHPolicyEngine(p.Options.GetSSHOptions()); err != nil { + // Initialize the SSH allow/deny policy engine for user certificates + if p.sshUserPolicy, err = newSSHUserPolicyEngine(p.Options.GetSSHOptions()); err != nil { + return err + } + + // Initialize the SSH allow/deny policy engine for host certificates + if p.sshHostPolicy, err = newSSHHostPolicyEngine(p.Options.GetSSHOptions()); err != nil { return err } @@ -275,7 +280,7 @@ func (p *Nebula) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOpti // Require all the fields in the SSH certificate &sshCertDefaultValidator{}, // Ensure that all principal names are allowed - newSSHNamePolicyValidator(p.sshPolicy), + newSSHNamePolicyValidator(p.sshHostPolicy, p.sshUserPolicy), ), nil } diff --git a/authority/provisioner/oidc.go b/authority/provisioner/oidc.go index e4fe8090..60bb5cf1 100644 --- a/authority/provisioner/oidc.go +++ b/authority/provisioner/oidc.go @@ -13,7 +13,6 @@ import ( "github.com/pkg/errors" "github.com/smallstep/certificates/errs" - "github.com/smallstep/certificates/policy" "go.step.sm/crypto/jose" "go.step.sm/crypto/sshutil" "go.step.sm/crypto/x509util" @@ -95,8 +94,9 @@ type OIDC struct { keyStore *keyStore claimer *Claimer getIdentityFunc GetIdentityFunc - x509Policy policy.X509NamePolicyEngine - sshPolicy policy.SSHNamePolicyEngine + x509Policy x509PolicyEngine + sshHostPolicy *hostPolicyEngine + sshUserPolicy *userPolicyEngine } func sanitizeEmail(email string) string { @@ -216,8 +216,13 @@ func (o *OIDC) Init(config Config) (err error) { return err } - // Initialize the SSH allow/deny policy engine - if o.sshPolicy, err = newSSHPolicyEngine(o.Options.GetSSHOptions()); err != nil { + // Initialize the SSH allow/deny policy engine for user certificates + if o.sshUserPolicy, err = newSSHUserPolicyEngine(o.Options.GetSSHOptions()); err != nil { + return err + } + + // Initialize the SSH allow/deny policy engine for host certificates + if o.sshHostPolicy, err = newSSHHostPolicyEngine(o.Options.GetSSHOptions()); err != nil { return err } @@ -468,7 +473,7 @@ func (o *OIDC) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption // Require all the fields in the SSH certificate &sshCertDefaultValidator{}, // Ensure that all principal names are allowed - newSSHNamePolicyValidator(o.sshPolicy), + newSSHNamePolicyValidator(o.sshHostPolicy, o.sshUserPolicy), ), nil } diff --git a/authority/provisioner/policy.go b/authority/provisioner/policy.go index 8a69e1e5..b9740e39 100644 --- a/authority/provisioner/policy.go +++ b/authority/provisioner/policy.go @@ -1,11 +1,38 @@ package provisioner import ( + "fmt" + "github.com/smallstep/certificates/policy" + "golang.org/x/crypto/ssh" +) + +type sshPolicyEngineType string + +const ( + userPolicyEngineType sshPolicyEngineType = "user" + hostPolicyEngineType sshPolicyEngineType = "host" ) +var certTypeToPolicyEngineType = map[uint32]sshPolicyEngineType{ + uint32(ssh.UserCert): userPolicyEngineType, + uint32(ssh.HostCert): hostPolicyEngineType, +} + +type x509PolicyEngine interface { + policy.X509NamePolicyEngine +} + +type userPolicyEngine struct { + policy.SSHNamePolicyEngine +} + +type hostPolicyEngine struct { + policy.SSHNamePolicyEngine +} + // newX509PolicyEngine creates a new x509 name policy engine -func newX509PolicyEngine(x509Opts *X509Options) (policy.X509NamePolicyEngine, error) { +func newX509PolicyEngine(x509Opts *X509Options) (x509PolicyEngine, error) { if x509Opts == nil { return nil, nil @@ -38,16 +65,66 @@ func newX509PolicyEngine(x509Opts *X509Options) (policy.X509NamePolicyEngine, er return policy.New(options...) } +// newSSHUserPolicyEngine creates a new SSH user certificate policy engine +func newSSHUserPolicyEngine(sshOpts *SSHOptions) (*userPolicyEngine, error) { + policyEngine, err := newSSHPolicyEngine(sshOpts, userPolicyEngineType) + if err != nil { + return nil, err + } + // ensure we're not wrapping a nil engine + if policyEngine == nil { + return nil, nil + } + return &userPolicyEngine{ + SSHNamePolicyEngine: policyEngine, + }, nil +} + +// newSSHHostPolicyEngine create a new SSH host certificate policy engine +func newSSHHostPolicyEngine(sshOpts *SSHOptions) (*hostPolicyEngine, error) { + policyEngine, err := newSSHPolicyEngine(sshOpts, hostPolicyEngineType) + if err != nil { + return nil, err + } + // ensure we're not wrapping a nil engine + if policyEngine == nil { + return nil, nil + } + return &hostPolicyEngine{ + SSHNamePolicyEngine: policyEngine, + }, nil +} + // newSSHPolicyEngine creates a new SSH name policy engine -func newSSHPolicyEngine(sshOpts *SSHOptions) (policy.SSHNamePolicyEngine, error) { +func newSSHPolicyEngine(sshOpts *SSHOptions, typ sshPolicyEngineType) (policy.SSHNamePolicyEngine, error) { if sshOpts == nil { return nil, nil } + var ( + allowed *SSHNameOptions + denied *SSHNameOptions + ) + + // TODO: embed the type in the policy engine itself for reference? + switch typ { + case userPolicyEngineType: + if sshOpts.User != nil { + allowed = sshOpts.User.GetAllowedNameOptions() + denied = sshOpts.User.GetDeniedNameOptions() + } + case hostPolicyEngineType: + if sshOpts.Host != nil { + allowed = sshOpts.Host.AllowedNames + denied = sshOpts.Host.DeniedNames + } + default: + return nil, fmt.Errorf("unknown SSH policy engine type %s provided", typ) + } + options := []policy.NamePolicyOption{} - allowed := sshOpts.GetAllowedNameOptions() if allowed != nil && allowed.HasNames() { options = append(options, policy.WithPermittedDNSDomains(allowed.DNSDomains), @@ -57,7 +134,6 @@ func newSSHPolicyEngine(sshOpts *SSHOptions) (policy.SSHNamePolicyEngine, error) ) } - denied := sshOpts.GetDeniedNameOptions() if denied != nil && denied.HasNames() { options = append(options, policy.WithExcludedDNSDomains(denied.DNSDomains), @@ -67,5 +143,14 @@ func newSSHPolicyEngine(sshOpts *SSHOptions) (policy.SSHNamePolicyEngine, error) ) } + // Return nil, because there's no policy to execute. This is + // important, because the logic that determines user vs. host certs + // are allowed depends on this fact. The two policy engines are + // not aware of eachother, so this check is performed in the + // SSH name validator, instead. + if len(options) == 0 { + return nil, nil + } + return policy.New(options...) } diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go index 7ca6cec4..3327310b 100644 --- a/authority/provisioner/sign_options.go +++ b/authority/provisioner/sign_options.go @@ -16,7 +16,6 @@ import ( "github.com/pkg/errors" "github.com/smallstep/certificates/errs" - "github.com/smallstep/certificates/policy" "go.step.sm/crypto/keyutil" "go.step.sm/crypto/x509util" ) @@ -408,11 +407,11 @@ func (v *validityValidator) Valid(cert *x509.Certificate, o SignOptions) error { // x509NamePolicyValidator validates that the certificate (to be signed) // contains only allowed SANs. type x509NamePolicyValidator struct { - policyEngine policy.X509NamePolicyEngine + policyEngine x509PolicyEngine } // newX509NamePolicyValidator return a new SANs allow/deny validator. -func newX509NamePolicyValidator(engine policy.X509NamePolicyEngine) *x509NamePolicyValidator { +func newX509NamePolicyValidator(engine x509PolicyEngine) *x509NamePolicyValidator { return &x509NamePolicyValidator{ policyEngine: engine, } @@ -424,7 +423,6 @@ func (v *x509NamePolicyValidator) Valid(cert *x509.Certificate, _ SignOptions) e if v.policyEngine == nil { return nil } - _, err := v.policyEngine.AreCertificateNamesAllowed(cert) return err } diff --git a/authority/provisioner/sign_ssh_options.go b/authority/provisioner/sign_ssh_options.go index 374bd65c..8f9cf466 100644 --- a/authority/provisioner/sign_ssh_options.go +++ b/authority/provisioner/sign_ssh_options.go @@ -4,13 +4,13 @@ import ( "crypto/rsa" "encoding/binary" "encoding/json" + "fmt" "math/big" "strings" "time" "github.com/pkg/errors" "github.com/smallstep/certificates/errs" - "github.com/smallstep/certificates/policy" "go.step.sm/crypto/keyutil" "golang.org/x/crypto/ssh" ) @@ -448,24 +448,55 @@ func (v sshDefaultPublicKeyValidator) Valid(cert *ssh.Certificate, o SignSSHOpti // sshNamePolicyValidator validates that the certificate (to be signed) // contains only allowed principals. type sshNamePolicyValidator struct { - policyEngine policy.SSHNamePolicyEngine + hostPolicyEngine *hostPolicyEngine + userPolicyEngine *userPolicyEngine } // newSSHNamePolicyValidator return a new SSH allow/deny validator. -func newSSHNamePolicyValidator(engine policy.SSHNamePolicyEngine) *sshNamePolicyValidator { +func newSSHNamePolicyValidator(host *hostPolicyEngine, user *userPolicyEngine) *sshNamePolicyValidator { return &sshNamePolicyValidator{ - policyEngine: engine, + hostPolicyEngine: host, + userPolicyEngine: user, } } // Valid validates validates that the certificate (to be signed) // contains only allowed principals. func (v *sshNamePolicyValidator) Valid(cert *ssh.Certificate, _ SignSSHOptions) error { - if v.policyEngine == nil { + if v.hostPolicyEngine == nil && v.userPolicyEngine == nil { + // no policy configured at all; allow anything return nil } - _, err := v.policyEngine.ArePrincipalsAllowed(cert) - return err + + // Check the policy type to execute based on type of the certificate. + // We don't allow user certs if only a host policy engine is configured and + // the same for host certs: if only a user policy engine is configured, host + // certs are denied. When both policy engines are configured, the type of + // cert determines which policy engine is used. + policyType, ok := certTypeToPolicyEngineType[cert.CertType] + if !ok { + return fmt.Errorf("unexpected SSH cert type %d", cert.CertType) + } + switch policyType { + case hostPolicyEngineType: + // when no host policy engine is configured, but a user policy engine is + // configured, we don't allow the host certificate. + if v.hostPolicyEngine == nil && v.userPolicyEngine != nil { + return errors.New("SSH host certificate not authorized") // TODO: include principals in message? + } + _, err := v.hostPolicyEngine.ArePrincipalsAllowed(cert) + return err + case userPolicyEngineType: + // when no user policy engine is configured, but a host policy engine is + // configured, we don't allow the user certificate. + if v.userPolicyEngine == nil && v.hostPolicyEngine != nil { + return errors.New("SSH user certificate not authorized") // TODO: include principals in message? + } + _, err := v.userPolicyEngine.ArePrincipalsAllowed(cert) + return err + default: + return fmt.Errorf("unexpected policy engine type %q", policyType) // satisfy return; shouldn't happen + } } // sshCertTypeUInt32 diff --git a/authority/provisioner/ssh_options.go b/authority/provisioner/ssh_options.go index 91ce7126..dacafc80 100644 --- a/authority/provisioner/ssh_options.go +++ b/authority/provisioner/ssh_options.go @@ -34,6 +34,15 @@ type SSHOptions struct { // templates. TemplateData json.RawMessage `json:"templateData,omitempty"` + // User contains SSH user certificate options. + User *SSHUserCertificateOptions `json:"user,omitempty"` + + // Host contains SSH host certificate options. + Host *SSHHostCertificateOptions `json:"host,omitempty"` +} + +// SSHUserCertificateOptions is a collection of SSH user certificate options. +type SSHUserCertificateOptions struct { // AllowedNames contains the names the provisioner is authorized to sign AllowedNames *SSHNameOptions `json:"allow,omitempty"` @@ -41,6 +50,11 @@ type SSHOptions struct { DeniedNames *SSHNameOptions `json:"deny,omitempty"` } +// SSHHostCertificateOptions is a collection of SSH host certificate options. +// It's an alias of SSHUserCertificateOptions, as the options are the same +// for both types of certificates. +type SSHHostCertificateOptions SSHUserCertificateOptions + // SSHNameOptions models the SSH name policy configuration. type SSHNameOptions struct { DNSDomains []string `json:"dns,omitempty"` @@ -56,7 +70,7 @@ func (o *SSHOptions) HasTemplate() bool { // GetAllowedNameOptions returns the AllowedSSHNameOptions, which models the // names that a provisioner is authorized to sign SSH certificates for. -func (o *SSHOptions) GetAllowedNameOptions() *SSHNameOptions { +func (o *SSHUserCertificateOptions) GetAllowedNameOptions() *SSHNameOptions { if o == nil { return nil } @@ -65,7 +79,7 @@ func (o *SSHOptions) GetAllowedNameOptions() *SSHNameOptions { // GetDeniedNameOptions returns the DeniedSSHNameOptions, which models the // names that a provisioner is NOT authorized to sign SSH certificates for. -func (o *SSHOptions) GetDeniedNameOptions() *SSHNameOptions { +func (o *SSHUserCertificateOptions) GetDeniedNameOptions() *SSHNameOptions { if o == nil { return nil } diff --git a/authority/provisioner/x5c.go b/authority/provisioner/x5c.go index 434fc576..850fc752 100644 --- a/authority/provisioner/x5c.go +++ b/authority/provisioner/x5c.go @@ -9,7 +9,6 @@ import ( "github.com/pkg/errors" "github.com/smallstep/certificates/errs" - "github.com/smallstep/certificates/policy" "go.step.sm/crypto/jose" "go.step.sm/crypto/sshutil" "go.step.sm/crypto/x509util" @@ -27,17 +26,18 @@ type x5cPayload struct { // signature requests. type X5C struct { *base - ID string `json:"-"` - Type string `json:"type"` - Name string `json:"name"` - Roots []byte `json:"roots"` - Claims *Claims `json:"claims,omitempty"` - Options *Options `json:"options,omitempty"` - claimer *Claimer - audiences Audiences - rootPool *x509.CertPool - x509Policy policy.X509NamePolicyEngine - sshPolicy policy.SSHNamePolicyEngine + ID string `json:"-"` + Type string `json:"type"` + Name string `json:"name"` + Roots []byte `json:"roots"` + Claims *Claims `json:"claims,omitempty"` + Options *Options `json:"options,omitempty"` + claimer *Claimer + audiences Audiences + rootPool *x509.CertPool + x509Policy x509PolicyEngine + sshHostPolicy *hostPolicyEngine + sshUserPolicy *userPolicyEngine } // GetID returns the provisioner unique identifier. The name and credential id @@ -133,8 +133,13 @@ func (p *X5C) Init(config Config) error { return err } - // Initialize the SSH allow/deny policy engine - if p.sshPolicy, err = newSSHPolicyEngine(p.Options.GetSSHOptions()); err != nil { + // Initialize the SSH allow/deny policy engine for user certificates + if p.sshUserPolicy, err = newSSHUserPolicyEngine(p.Options.GetSSHOptions()); err != nil { + return err + } + + // Initialize the SSH allow/deny policy engine for host certificates + if p.sshHostPolicy, err = newSSHHostPolicyEngine(p.Options.GetSSHOptions()); err != nil { return err } @@ -326,6 +331,6 @@ func (p *X5C) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption, // Require all the fields in the SSH certificate &sshCertDefaultValidator{}, // Ensure that all principal names are allowed - newSSHNamePolicyValidator(p.sshPolicy), + newSSHNamePolicyValidator(p.sshHostPolicy, p.sshUserPolicy), ), nil } diff --git a/authority/provisioner/x5c_test.go b/authority/provisioner/x5c_test.go index 5d2a3566..b91ca2ea 100644 --- a/authority/provisioner/x5c_test.go +++ b/authority/provisioner/x5c_test.go @@ -780,7 +780,8 @@ func TestX5C_AuthorizeSSHSign(t *testing.T) { case *sshCertValidityValidator: assert.Equals(t, v.Claimer, tc.p.claimer) case *sshNamePolicyValidator: - assert.Equals(t, nil, v.policyEngine) + assert.Equals(t, nil, v.userPolicyEngine) + assert.Equals(t, nil, v.hostPolicyEngine) case *sshDefaultPublicKeyValidator, *sshCertDefaultValidator, sshCertificateOptionsFunc: default: assert.FatalError(t, errors.Errorf("unexpected sign option of type %T", v)) diff --git a/policy/engine.go b/policy/engine.go index 42d4f303..e9038dd0 100755 --- a/policy/engine.go +++ b/policy/engine.go @@ -216,11 +216,11 @@ func (e *NamePolicyEngine) IsIPAllowed(ip net.IP) (bool, error) { // ArePrincipalsAllowed verifies that all principals in an SSH certificate are allowed. func (e *NamePolicyEngine) ArePrincipalsAllowed(cert *ssh.Certificate) (bool, error) { - dnsNames, ips, emails, usernames, err := splitSSHPrincipals(cert) + dnsNames, ips, emails, principals, err := splitSSHPrincipals(cert) if err != nil { return false, err } - if err := e.validateNames(dnsNames, ips, emails, []*url.URL{}, usernames); err != nil { + if err := e.validateNames(dnsNames, ips, emails, []*url.URL{}, principals); err != nil { return false, err } return true, nil @@ -243,32 +243,26 @@ func appendSubjectCommonName(subject pkix.Name, dnsNames *[]string, ips *[]net.I } // splitPrincipals splits SSH certificate principals into DNS names, emails and usernames. -func splitSSHPrincipals(cert *ssh.Certificate) (dnsNames []string, ips []net.IP, emails, usernames []string, err error) { +func splitSSHPrincipals(cert *ssh.Certificate) (dnsNames []string, ips []net.IP, emails, principals []string, err error) { dnsNames = []string{} ips = []net.IP{} emails = []string{} - usernames = []string{} + principals = []string{} var uris []*url.URL switch cert.CertType { case ssh.HostCert: dnsNames, ips, emails, uris = x509util.SplitSANs(cert.ValidPrincipals) - switch { - case len(emails) > 0: - err = fmt.Errorf("Email(-like) principals %v not expected in SSH Host certificate ", emails) - case len(uris) > 0: - err = fmt.Errorf("URL principals %v not expected in SSH Host certificate ", uris) + if len(uris) > 0 { + err = fmt.Errorf("URL principals %v not expected in SSH host certificate ", uris) } case ssh.UserCert: // re-using SplitSANs results in anything that can't be parsed as an IP, URI or email - // to be considered a username. This allows usernames like h.slatman to be present + // to be considered a username principal. This allows usernames like h.slatman to be present // in the SSH certificate. We're exluding IPs and URIs, because they can be confusing // when used in a SSH user certificate. - usernames, ips, emails, uris = x509util.SplitSANs(cert.ValidPrincipals) - switch { - case len(ips) > 0: - err = fmt.Errorf("IP principals %v not expected in SSH User certificate ", ips) - case len(uris) > 0: - err = fmt.Errorf("URL principals %v not expected in SSH User certificate ", uris) + principals, ips, emails, uris = x509util.SplitSANs(cert.ValidPrincipals) + if len(uris) > 0 { + err = fmt.Errorf("URL principals %v not expected in SSH user certificate ", uris) } default: err = fmt.Errorf("unexpected SSH certificate type %d", cert.CertType) @@ -280,7 +274,7 @@ func splitSSHPrincipals(cert *ssh.Certificate) (dnsNames []string, ips []net.IP, // validateNames verifies that all names are allowed. // Its logic follows that of (a large part of) the (c *Certificate) isValid() function // in https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go -func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailAddresses []string, uris []*url.URL, usernames []string) error { +func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailAddresses []string, uris []*url.URL, principals []string) error { // nothing to compare against; return early if e.totalNumberOfConstraints == 0 { @@ -400,15 +394,15 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA } } - for _, username := range usernames { + for _, principal := range principals { if e.numberOfPrincipalConstraints == 0 && e.totalNumberOfPermittedConstraints > 0 { return NamePolicyError{ Reason: NotAuthorizedForThisName, - Detail: fmt.Sprintf("username principal %q is not explicitly permitted by any constraint", username), + Detail: fmt.Sprintf("username principal %q is not explicitly permitted by any constraint", principal), } } // TODO: some validation? I.e. allowed characters? - if err := checkNameConstraints("username", username, username, + if err := checkNameConstraints("principal", principal, principal, func(parsedName, constraint interface{}) (bool, error) { return matchUsernameConstraint(parsedName.(string), constraint.(string)) }, e.permittedPrincipals, e.excludedPrincipals); err != nil { diff --git a/policy/engine_test.go b/policy/engine_test.go index 1f8be691..0259e8de 100755 --- a/policy/engine_test.go +++ b/policy/engine_test.go @@ -2749,18 +2749,6 @@ func Test_splitSSHPrincipals(t *testing.T) { wantErr: true, } }, - "fail/user-ip": func(t *testing.T) test { - r := emptyResult() - r.wantIps = []net.IP{net.ParseIP("127.0.0.1")} // this will still be in the result - return test{ - cert: &ssh.Certificate{ - CertType: ssh.UserCert, - ValidPrincipals: []string{"127.0.0.1"}, - }, - r: r, - wantErr: true, - } - }, "fail/user-uri": func(t *testing.T) test { r := emptyResult() return test{ @@ -2772,18 +2760,6 @@ func Test_splitSSHPrincipals(t *testing.T) { wantErr: true, } }, - "fail/host-email": func(t *testing.T) test { - r := emptyResult() - r.wantEmails = []string{"ops@work"} // this will still be in the result - return test{ - cert: &ssh.Certificate{ - CertType: ssh.HostCert, - ValidPrincipals: []string{"ops@work"}, - }, - r: r, - wantErr: true, - } - }, "fail/host-uri": func(t *testing.T) test { r := emptyResult() return test{ @@ -2817,6 +2793,18 @@ func Test_splitSSHPrincipals(t *testing.T) { r: r, } }, + "ok/host-email": func(t *testing.T) test { + r := emptyResult() + r.wantEmails = []string{"ops@work"} + return test{ + cert: &ssh.Certificate{ + CertType: ssh.HostCert, + ValidPrincipals: []string{"ops@work"}, + }, + r: r, + wantErr: false, + } + }, "ok/user-localhost": func(t *testing.T) test { r := emptyResult() r.wantUsernames = []string{"localhost"} // when type is User cert, this is considered a username; not a DNS @@ -2839,6 +2827,18 @@ func Test_splitSSHPrincipals(t *testing.T) { r: r, } }, + "ok/user-ip": func(t *testing.T) test { + r := emptyResult() + r.wantIps = []net.IP{net.ParseIP("127.0.0.1")} + return test{ + cert: &ssh.Certificate{ + CertType: ssh.UserCert, + ValidPrincipals: []string{"127.0.0.1"}, + }, + r: r, + wantErr: false, + } + }, "ok/user-maillike": func(t *testing.T) test { r := emptyResult() r.wantEmails = []string{"ops@work"}