Skip to content

Commit

Permalink
Allow specifying multiple allowed SSH key lengths (#13991)
Browse files Browse the repository at this point in the history 
* Allow specifying multiple allowed SSH key lengths

In the ssh secrets engine, only a single allowed key length was allowed
for each algorithm type. However, many algorithms have multiple safe
values (such as RSA and ECDSA); allowing a single role to have multiple
values for a single algorithm is thus helpful.

On creation or update, roles can now specify multiple types using a list
or comma separated string of allowed values:

    allowed_user_key_lengths: map[string][]int{"rsa": []int{2048, 4096}}

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add changelog entry

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Break out ssh upgrade logic into separate function

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Update parseutil for optional lists of integers

    go get -u github.com/hashicorp/go-secure-stdlib/parseutil
    go mod tidy

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Simplify parse logic using new parseutil

The newly introduced parseutil.ParseIntSlice handles the more
complicated optional int-like slice logic for us.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
  • Loading branch information
@cipherboy
cipherboy authored Feb 17, 2022
1 parent 8f38d79 commit 00c3e8f
Show file tree
Hide file tree
Showing 8 changed files with 188 additions and 65 deletions.
46 changes: 42 additions & 4 deletions builtin/logical/ssh/backend_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,10 @@ import (
"bytes"
"context"
"encoding/base64"
"encoding/json"
"errors"
"fmt"
"net"
"reflect"
"strconv"
"strings"
"testing"
"time"
Expand Down Expand Up @@ -76,6 +74,8 @@ oOyBJU/HMVvBfv4g+OVFLVgSwwm6owwsouZ0+D/LasbuHqYyqYqdyPJQYzWA2Y+F
publicKey2 = `AAAAB3NzaC1yc2EAAAADAQABAAABAQDArgK0ilRRfk8E7HIsjz5l3BuxmwpDd8DHRCVfOhbZ4gOSVxjEOOqBwWGjygdboBIZwFXmwDlU6sWX0hBJAgpQz0Cjvbjxtq/NjkvATrYPgnrXUhTaEn2eQO0PsqRNSFH46SK/oJfTp0q8/WgojxWJ2L7FUV8PO8uIk49DzqAqPV7WXU63vFsjx+3WQOX/ILeQvHCvaqs3dWjjzEoDudRWCOdUqcHEOshV9azIzPrXlQVzRV3QAKl6u7pC+/Secorpwt6IHpMKoVPGiR0tMMuNOVH8zrAKzIxPGfy2WmNDpJopbXMTvSOGAqNcp49O4SKOQl9Fzfq2HEevJamKLrMB
`

publicKey3072 = `ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQDlsMr3K1d0nzE1TjUULPRuVjEGETmOqHtWq4gVPq3HiuNVHE/e/BJnkXc40BoClQ2Z5ZZPJZ6izF9PnlzNDjpq8DrILUrn/6KrzCHvRwnkYMAXbfM/Br09z5QGptbOe1EMLeVe0b/udmUicbYAGPxMruZk+ljyr4vXkO+gOAIrxeSIQSdMVLU4g0pCPQuDCOx5IQpDYSlOB3091frpN8npfMueKPflNYzxnqqYgAVeDKAIqMCGOMOHUeIZJ7A7HuynEAVOsOkJwC9nesy9D6ppdWNduGl42IkzlwVdDMZtUAEznMUT/dnHNG1Krx9SuNZ/S9fGjxGVsT+jzUmizrWB9/6XIEHDxPBzcqlWFuwYTGz1OL8bfZ+HldOGPcnqZn9hKntWwjUc3whcvWt+NCmXpHSVLSxf+WN8pdmfEsCqn8mpvo2MXa+iJrtAVPX4i0u8AQUuqC3NuXHv4Cn0LNwtziBT544UjgbWkAZqzFZJREYA09OHscc3akEIrTnPehk= demo@example.com`

publicKey4096 = `ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQC54Oj4YCFDYxYv69Q9KfU6rWYtUB1eByQdUW0nXFi/vr98QUIV77sEeUVhaQzZcuCojAi/GrloW7ta0Z2DaEv5jOQMAnGpXBcqLJsz3KdrHbpvl93MPNdmNaGPU0GnUEsjBVuDVn9HdIUa8CNrxShvPu7/VqoaRHKLqphGgzFb37vi4qvnQ+5VYAO/TzyVYMD6qJX6I/9Pw8d74jCfEdOh2yGKkP7rXWOghreyIl8H2zTJKg9KoZuPq9F5M8nNt7Oi3rf+DwQiYvamzIqlDP4s5oFVTZW0E9lwWvYDpyiJnUrkQqksebBK/rcyfiFG3onb4qLo2WVWXeK3si8IhGik/TEzprScyAWIf9RviT8O+l5hTA2/c+ctn3MVCLRNfez2lKpdxCoprv1MbIcySGWblTJEcY6RA+aauVJpu7FMtRxHHtZKtMpep8cLu8GKbiP6Ifq2JXBtXtNxDeIgo2MkNoMh/NHAsACJniE/dqV/+u9HvhvgrTbJ69ell0nE4ivzA7O4kZgbR/4MHlLgLFvaqC8RrWRLY6BdFagPIMxghWha7Qw16zqoIjRnolvRzUWvSXanJVg8Z6ua1VxwgirNaAH1ivmJhUh2+4lNxCX6jmZyR3zjJsWY03gjJTairvI762opjjalF8fH6Xrs15mB14JiAlNbk6+5REQcvXlGqw== dummy@example.com`

testCAPrivateKey = `-----BEGIN RSA PRIVATE KEY-----
Expand Down Expand Up @@ -1198,7 +1198,7 @@ func TestBackend_AllowedUserKeyLengths(t *testing.T) {
"key_type": "ca",
"allow_user_certificates": true,
"allowed_user_key_lengths": map[string]interface{}{
"rsa": json.Number(strconv.FormatInt(4096, 10)),
"rsa": 4096,
},
}),
{
Expand All @@ -1219,7 +1219,7 @@ func TestBackend_AllowedUserKeyLengths(t *testing.T) {
"key_type": "ca",
"allow_user_certificates": true,
"allowed_user_key_lengths": map[string]interface{}{
"rsa": json.Number(strconv.FormatInt(2048, 10)),
"rsa": 2048,
},
}),
// Pass with 2048 key
Expand All @@ -1245,6 +1245,44 @@ func TestBackend_AllowedUserKeyLengths(t *testing.T) {
return nil
},
},
createRoleStep("multikey", map[string]interface{}{
"key_type": "ca",
"allow_user_certificates": true,
"allowed_user_key_lengths": map[string]interface{}{
"rsa": []int{2048, 4096},
},
}),
// Pass with 2048-bit key
{
Operation: logical.UpdateOperation,
Path: "sign/multikey",
Data: map[string]interface{}{
"public_key": testCAPublicKey,
},
},
// Pass with 4096-bit key
{
Operation: logical.UpdateOperation,
Path: "sign/multikey",
Data: map[string]interface{}{
"public_key": publicKey4096,
},
},
// Fail with 3072-bit key
{
Operation: logical.UpdateOperation,
Path: "sign/multikey",
Data: map[string]interface{}{
"public_key": publicKey3072,
},
ErrorOk: true,
Check: func(resp *logical.Response) error {
if resp.Data["error"] != "public_key failed to meet the key requirements: key is of an invalid size: 3072" {
return errors.New("a larger key (3072) was allowed, when the size was set for 2048")
}
return nil
},
},
},
}

Expand Down
128 changes: 97 additions & 31 deletions builtin/logical/ssh/path_roles.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/hashicorp/go-secure-stdlib/parseutil"
"github.com/hashicorp/vault/sdk/framework"
"github.com/hashicorp/vault/sdk/helper/cidrutil"
"github.com/hashicorp/vault/sdk/helper/consts"
"github.com/hashicorp/vault/sdk/logical"
"golang.org/x/crypto/ssh"
)
Expand All @@ -20,40 +21,47 @@ const (
KeyTypeDynamic = "dynamic"
// KeyTypeCA is an key of type CA
KeyTypeCA = "ca"

// Present version of the sshRole struct; when adding a new field or are
// needing to perform a migration, increment this struct and read the note
// in checkUpgrade(...).
roleEntryVersion = 1
)

// Structure that represents a role in SSH backend. This is a common role structure
// for both OTP and Dynamic roles. Not all the fields are mandatory for both type.
// Some are applicable for one and not for other. It doesn't matter.
type sshRole struct {
KeyType string `mapstructure:"key_type" json:"key_type"`
KeyName string `mapstructure:"key" json:"key"`
KeyBits int `mapstructure:"key_bits" json:"key_bits"`
AdminUser string `mapstructure:"admin_user" json:"admin_user"`
DefaultUser string `mapstructure:"default_user" json:"default_user"`
CIDRList string `mapstructure:"cidr_list" json:"cidr_list"`
ExcludeCIDRList string `mapstructure:"exclude_cidr_list" json:"exclude_cidr_list"`
Port int `mapstructure:"port" json:"port"`
InstallScript string `mapstructure:"install_script" json:"install_script"`
AllowedUsers string `mapstructure:"allowed_users" json:"allowed_users"`
AllowedUsersTemplate bool `mapstructure:"allowed_users_template" json:"allowed_users_template"`
AllowedDomains string `mapstructure:"allowed_domains" json:"allowed_domains"`
KeyOptionSpecs string `mapstructure:"key_option_specs" json:"key_option_specs"`
MaxTTL string `mapstructure:"max_ttl" json:"max_ttl"`
TTL string `mapstructure:"ttl" json:"ttl"`
DefaultCriticalOptions map[string]string `mapstructure:"default_critical_options" json:"default_critical_options"`
DefaultExtensions map[string]string `mapstructure:"default_extensions" json:"default_extensions"`
DefaultExtensionsTemplate bool `mapstructure:"default_extensions_template" json:"default_extensions_template"`
AllowedCriticalOptions string `mapstructure:"allowed_critical_options" json:"allowed_critical_options"`
AllowedExtensions string `mapstructure:"allowed_extensions" json:"allowed_extensions"`
AllowUserCertificates bool `mapstructure:"allow_user_certificates" json:"allow_user_certificates"`
AllowHostCertificates bool `mapstructure:"allow_host_certificates" json:"allow_host_certificates"`
AllowBareDomains bool `mapstructure:"allow_bare_domains" json:"allow_bare_domains"`
AllowSubdomains bool `mapstructure:"allow_subdomains" json:"allow_subdomains"`
AllowUserKeyIDs bool `mapstructure:"allow_user_key_ids" json:"allow_user_key_ids"`
KeyIDFormat string `mapstructure:"key_id_format" json:"key_id_format"`
AllowedUserKeyLengths map[string]int `mapstructure:"allowed_user_key_lengths" json:"allowed_user_key_lengths"`
AlgorithmSigner string `mapstructure:"algorithm_signer" json:"algorithm_signer"`
KeyType string `mapstructure:"key_type" json:"key_type"`
KeyName string `mapstructure:"key" json:"key"`
KeyBits int `mapstructure:"key_bits" json:"key_bits"`
AdminUser string `mapstructure:"admin_user" json:"admin_user"`
DefaultUser string `mapstructure:"default_user" json:"default_user"`
CIDRList string `mapstructure:"cidr_list" json:"cidr_list"`
ExcludeCIDRList string `mapstructure:"exclude_cidr_list" json:"exclude_cidr_list"`
Port int `mapstructure:"port" json:"port"`
InstallScript string `mapstructure:"install_script" json:"install_script"`
AllowedUsers string `mapstructure:"allowed_users" json:"allowed_users"`
AllowedUsersTemplate bool `mapstructure:"allowed_users_template" json:"allowed_users_template"`
AllowedDomains string `mapstructure:"allowed_domains" json:"allowed_domains"`
KeyOptionSpecs string `mapstructure:"key_option_specs" json:"key_option_specs"`
MaxTTL string `mapstructure:"max_ttl" json:"max_ttl"`
TTL string `mapstructure:"ttl" json:"ttl"`
DefaultCriticalOptions map[string]string `mapstructure:"default_critical_options" json:"default_critical_options"`
DefaultExtensions map[string]string `mapstructure:"default_extensions" json:"default_extensions"`
DefaultExtensionsTemplate bool `mapstructure:"default_extensions_template" json:"default_extensions_template"`
AllowedCriticalOptions string `mapstructure:"allowed_critical_options" json:"allowed_critical_options"`
AllowedExtensions string `mapstructure:"allowed_extensions" json:"allowed_extensions"`
AllowUserCertificates bool `mapstructure:"allow_user_certificates" json:"allow_user_certificates"`
AllowHostCertificates bool `mapstructure:"allow_host_certificates" json:"allow_host_certificates"`
AllowBareDomains bool `mapstructure:"allow_bare_domains" json:"allow_bare_domains"`
AllowSubdomains bool `mapstructure:"allow_subdomains" json:"allow_subdomains"`
AllowUserKeyIDs bool `mapstructure:"allow_user_key_ids" json:"allow_user_key_ids"`
KeyIDFormat string `mapstructure:"key_id_format" json:"key_id_format"`
OldAllowedUserKeyLengths map[string]int `mapstructure:"allowed_user_key_lengths" json:"allowed_user_key_lengths,omitempty"`
AllowedUserKeyTypesLengths map[string][]int `mapstructure:"allowed_user_key_types_lengths" json:"allowed_user_key_types_lengths"`
AlgorithmSigner string `mapstructure:"algorithm_signer" json:"algorithm_signer"`
Version int `mapstructure:"role_version" json:"role_version"`
}

func pathListRoles(b *backend) *framework.Path {
Expand Down Expand Up @@ -431,6 +439,7 @@ func (b *backend) pathRoleWrite(ctx context.Context, req *logical.Request, d *fr
KeyType: KeyTypeOTP,
Port: port,
AllowedUsers: allowedUsers,
Version: roleEntryVersion,
}
} else if keyType == KeyTypeDynamic {
defaultUser := d.Get("default_user").(string)
Expand Down Expand Up @@ -484,6 +493,7 @@ func (b *backend) pathRoleWrite(ctx context.Context, req *logical.Request, d *fr
InstallScript: installScript,
AllowedUsers: allowedUsers,
KeyOptionSpecs: keyOptionSpecs,
Version: roleEntryVersion,
}
} else if keyType == KeyTypeCA {
algorithmSigner := ""
Expand Down Expand Up @@ -539,6 +549,7 @@ func (b *backend) createCARole(allowedUsers, defaultUser, signer string, data *f
KeyIDFormat: data.Get("key_id_format").(string),
KeyType: KeyTypeCA,
AlgorithmSigner: signer,
Version: roleEntryVersion,
}

if !role.AllowUserCertificates && !role.AllowHostCertificates {
Expand All @@ -547,7 +558,7 @@ func (b *backend) createCARole(allowedUsers, defaultUser, signer string, data *f

defaultCriticalOptions := convertMapToStringValue(data.Get("default_critical_options").(map[string]interface{}))
defaultExtensions := convertMapToStringValue(data.Get("default_extensions").(map[string]interface{}))
allowedUserKeyLengths, err := convertMapToIntValue(data.Get("allowed_user_key_lengths").(map[string]interface{}))
allowedUserKeyLengths, err := convertMapToIntSlice(data.Get("allowed_user_key_lengths").(map[string]interface{}))
if err != nil {
return nil, logical.ErrorResponse(fmt.Sprintf("error processing allowed_user_key_lengths: %s", err.Error()))
}
Expand All @@ -562,7 +573,7 @@ func (b *backend) createCARole(allowedUsers, defaultUser, signer string, data *f
role.MaxTTL = maxTTL.String()
role.DefaultCriticalOptions = defaultCriticalOptions
role.DefaultExtensions = defaultExtensions
role.AllowedUserKeyLengths = allowedUserKeyLengths
role.AllowedUserKeyTypesLengths = allowedUserKeyLengths

return role, nil
}
Expand All @@ -581,9 +592,64 @@ func (b *backend) getRole(ctx context.Context, s logical.Storage, n string) (*ss
return nil, err
}

if err := b.checkUpgrade(ctx, s, n, &result); err != nil {
return nil, err
}

return &result, nil
}

func (b *backend) checkUpgrade(ctx context.Context, s logical.Storage, n string, result *sshRole) error {
modified := false

// NOTE: When introducing a new migration, increment roleEntryVersion and
// check if the version is less than the version this change was introduced
// at and perform the change. At the end, set modified and update the
// version to the version this migration was introduced at! Additionally,
// add new migrations after all existing migrations.
//
// Otherwise, past or future migrations may not execute!
if result.Version == roleEntryVersion {
return nil
}

// Role version introduced at version 1, migrating OldAllowedUserKeyLengths
// to the newer AllowedUserKeyTypesLengths field.
if result.Version < 1 {
// Only migrate if we have old data and no new data to avoid clobbering.
//
// This change introduced the first role version, value of 1.
if len(result.OldAllowedUserKeyLengths) > 0 && len(result.AllowedUserKeyTypesLengths) == 0 {
result.AllowedUserKeyTypesLengths = make(map[string][]int)
for k, v := range result.OldAllowedUserKeyLengths {
result.AllowedUserKeyTypesLengths[k] = []int{v}
}
result.OldAllowedUserKeyLengths = nil
}

result.Version = 1
modified = true
}

// Add new migrations just before here.
//
// Condition copied from PKI builtin.
if modified && (b.System().LocalMount() || !b.System().ReplicationState().HasState(consts.ReplicationPerformanceSecondary)) {
jsonEntry, err := logical.StorageEntryJSON("roles/"+n, &result)
if err != nil {
return err
}
if err := s.Put(ctx, jsonEntry); err != nil {
// Only perform upgrades on replication primary
if !strings.Contains(err.Error(), logical.ErrReadOnly.Error()) {
return err
}
}
}

return nil
}

// parseRole converts a sshRole object into its map[string]interface representation,
// with appropriate values for each KeyType. If the KeyType is invalid, it will return
// an error.
Expand Down Expand Up @@ -630,7 +696,7 @@ func (b *backend) parseRole(role *sshRole) (map[string]interface{}, error) {
"default_critical_options": role.DefaultCriticalOptions,
"default_extensions": role.DefaultExtensions,
"default_extensions_template": role.DefaultExtensionsTemplate,
"allowed_user_key_lengths": role.AllowedUserKeyLengths,
"allowed_user_key_lengths": role.AllowedUserKeyTypesLengths,
"algorithm_signer": role.AlgorithmSigner,
}
case KeyTypeDynamic:
Expand Down
41 changes: 23 additions & 18 deletions builtin/logical/ssh/path_sign.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ func (b *backend) calculateTTL(data *framework.FieldData, role *sshRole) (time.D
}

func (b *backend) validateSignedKeyRequirements(publickey ssh.PublicKey, role *sshRole) error {
if len(role.AllowedUserKeyLengths) != 0 {
if len(role.AllowedUserKeyTypesLengths) != 0 {
var kstr string
var kbits int

Expand All @@ -473,31 +473,36 @@ func (b *backend) validateSignedKeyRequirements(publickey ssh.PublicKey, role *s
return fmt.Errorf("pubkey not suitable for crypto (expected ssh.CryptoPublicKey but found %T)", k)
}

if value, ok := role.AllowedUserKeyLengths[kstr]; ok {
if allowed_values, ok := role.AllowedUserKeyTypesLengths[kstr]; ok {
var pass bool
switch kstr {
case "rsa":
if kbits == value {
pass = true
}
case "dsa":
if kbits == value {
pass = true
}
case "ecdsa":
if kbits == value {
for _, value := range allowed_values {
switch kstr {
case "rsa":
if kbits == value {
pass = true
break
}
case "dsa":
if kbits == value {
pass = true
break
}
case "ecdsa":
if kbits == value {
pass = true
break
}
case "ed25519":
// ed25519 public keys are always 256 bits in length,
// so there is no need to inspect their value
pass = true
break
}
case "ed25519":
// ed25519 public keys are always 256 bits in length,
// so there is no need to inspect their value
pass = true
}

if !pass {
return fmt.Errorf("key is of an invalid size: %v", kbits)
}

} else {
return fmt.Errorf("key type of %s is not allowed", kstr)
}
Expand Down
14 changes: 10 additions & 4 deletions builtin/logical/ssh/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,15 +220,21 @@ func convertMapToStringValue(initial map[string]interface{}) map[string]string {
return result
}

func convertMapToIntValue(initial map[string]interface{}) (map[string]int, error) {
result := map[string]int{}
func convertMapToIntSlice(initial map[string]interface{}) (map[string][]int, error) {
result := map[string][]int{}

for key, value := range initial {
v, err := parseutil.ParseInt(value)
sliced, err := parseutil.ParseIntSlice(value)
if err != nil {
return nil, err
}
result[key] = int(v)

result[key] = make([]int, 0, len(sliced))
for _, value := range sliced {
result[key] = append(result[key], int(value))
}
}

return result, nil
}

Expand Down
3 changes: 3 additions & 0 deletions changelog/13991.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
secrets/ssh: Allow specifying multiple approved key lengths for a single algorithm
```
6 changes: 3 additions & 3 deletions go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,10 @@ require (
github.com/hashicorp/go-secure-stdlib/gatedwriter v0.1.1
github.com/hashicorp/go-secure-stdlib/kv-builder v0.1.1
github.com/hashicorp/go-secure-stdlib/mlock v0.1.1
github.com/hashicorp/go-secure-stdlib/parseutil v0.1.2
github.com/hashicorp/go-secure-stdlib/parseutil v0.1.3
github.com/hashicorp/go-secure-stdlib/password v0.1.1
github.com/hashicorp/go-secure-stdlib/reloadutil v0.1.1
github.com/hashicorp/go-secure-stdlib/strutil v0.1.1
github.com/hashicorp/go-secure-stdlib/strutil v0.1.2
github.com/hashicorp/go-secure-stdlib/tlsutil v0.1.1
github.com/hashicorp/go-sockaddr v1.0.2
github.com/hashicorp/go-syslog v1.0.0
Expand Down Expand Up @@ -138,7 +138,7 @@ require (
github.com/mitchellh/go-testing-interface v1.14.0
github.com/mitchellh/go-wordwrap v1.0.0
github.com/mitchellh/gox v1.0.1
github.com/mitchellh/mapstructure v1.4.2
github.com/mitchellh/mapstructure v1.4.3
github.com/mitchellh/reflectwalk v1.0.2
github.com/mongodb/go-client-mongodb-atlas v0.1.2
github.com/natefinch/atomic v0.0.0-20150920032501-a62ce929ffcc
Expand Down
Loading

0 comments on commit 00c3e8f

Please sign in to comment.