From 297dd7f4f0ac49d13bd5fbc61700765f37f0505c Mon Sep 17 00:00:00 2001 From: Scott Miller Date: Mon, 23 Sep 2024 22:20:11 +0000 Subject: [PATCH] backport of commit 12f03b073a808c657ae92b1d90200a4113a19c4e --- builtin/logical/ssh/backend_test.go | 106 +++++++++++++++++++-- builtin/logical/ssh/path_config_ca_test.go | 1 + builtin/logical/ssh/path_issue.go | 2 +- builtin/logical/ssh/path_issue_sign.go | 13 ++- builtin/logical/ssh/path_roles.go | 7 ++ changelog/28466.txt | 3 + website/content/api-docs/secret/ssh.mdx | 16 +++- 7 files changed, 133 insertions(+), 15 deletions(-) create mode 100644 changelog/28466.txt diff --git a/builtin/logical/ssh/backend_test.go b/builtin/logical/ssh/backend_test.go index ad2c048d5e95..80f3300e4a90 100644 --- a/builtin/logical/ssh/backend_test.go +++ b/builtin/logical/ssh/backend_test.go @@ -1298,6 +1298,80 @@ func TestBackend_OptionsOverrideDefaults(t *testing.T) { logicaltest.Test(t, testCase) } +func TestBackend_EmptyPrincipals(t *testing.T) { + config := logical.TestBackendConfig() + + b, err := Factory(context.Background(), config) + if err != nil { + t.Fatalf("Cannot create backend: %s", err) + } + testCase := logicaltest.TestCase{ + LogicalBackend: b, + Steps: []logicaltest.TestStep{ + configCaStep(testCAPublicKey, testCAPrivateKey), + createRoleStep("no_user_principals", map[string]interface{}{ + "key_type": "ca", + "allow_user_certificates": true, + "allowed_user_key_lengths": map[string]interface{}{ + "rsa": 2048, + }, + "allowed_users": "no_principals", + }), + { + Operation: logical.UpdateOperation, + Path: "sign/no_user_principals", + Data: map[string]interface{}{ + "public_key": testCAPublicKey, + }, + ErrorOk: true, + Check: func(resp *logical.Response) error { + if resp.Data["error"] != "empty valid principals not allowed by role" { + return errors.New("expected empty valid principals not allowed by role") + } + return nil + }, + }, + createRoleStep("no_host_principals", map[string]interface{}{ + "key_type": "ca", + "allow_host_certificates": true, + "allowed_domains": "*", + }), + { + Operation: logical.UpdateOperation, + Path: "sign/no_host_principals", + Data: map[string]interface{}{ + "cert_type": "host", + "public_key": testCAPublicKeyEd25519, + }, + ErrorOk: true, + Check: func(resp *logical.Response) error { + if resp.Data["error"] != "empty valid principals not allowed by role" { + return errors.New("expected empty valid principals not allowed by role") + } + return nil + }, + }, + { + Operation: logical.UpdateOperation, + Path: "sign/no_host_principals", + Data: map[string]interface{}{ + "cert_type": "host", + "public_key": testCAPublicKeyEd25519, + "valid_principals": "example.com", + }, + ErrorOk: true, + Check: func(resp *logical.Response) error { + if resp.Data["error"] != nil { + return errors.New("expected no error") + } + return nil + }, + }, + }, + } + logicaltest.Test(t, testCase) +} + func TestBackend_AllowedUserKeyLengths(t *testing.T) { config := logical.TestBackendConfig() @@ -1315,6 +1389,7 @@ func TestBackend_AllowedUserKeyLengths(t *testing.T) { "allowed_user_key_lengths": map[string]interface{}{ "rsa": 4096, }, + "allowed_users": "guest", }), { Operation: logical.UpdateOperation, @@ -1336,13 +1411,15 @@ func TestBackend_AllowedUserKeyLengths(t *testing.T) { "allowed_user_key_lengths": map[string]interface{}{ "rsa": 2048, }, + "allowed_users": "guest", }), // Pass with 2048 key { Operation: logical.UpdateOperation, Path: "sign/stdkey", Data: map[string]interface{}{ - "public_key": testCAPublicKey, + "public_key": testCAPublicKey, + "valid_principals": "guest", }, }, // Fail with 4096 key @@ -1350,7 +1427,8 @@ func TestBackend_AllowedUserKeyLengths(t *testing.T) { Operation: logical.UpdateOperation, Path: "sign/stdkey", Data: map[string]interface{}{ - "public_key": publicKey4096, + "public_key": publicKey4096, + "valid_principals": "guest", }, ErrorOk: true, Check: func(resp *logical.Response) error { @@ -1366,13 +1444,15 @@ func TestBackend_AllowedUserKeyLengths(t *testing.T) { "allowed_user_key_lengths": map[string]interface{}{ "rsa": []int{2048, 4096}, }, + "allowed_users": "guest", }), // Pass with 2048-bit key { Operation: logical.UpdateOperation, Path: "sign/multikey", Data: map[string]interface{}{ - "public_key": testCAPublicKey, + "public_key": testCAPublicKey, + "valid_principals": "guest", }, }, // Pass with 4096-bit key @@ -1380,7 +1460,8 @@ func TestBackend_AllowedUserKeyLengths(t *testing.T) { Operation: logical.UpdateOperation, Path: "sign/multikey", Data: map[string]interface{}{ - "public_key": publicKey4096, + "public_key": publicKey4096, + "valid_principals": "guest", }, }, // Fail with 3072-bit key @@ -1403,7 +1484,8 @@ func TestBackend_AllowedUserKeyLengths(t *testing.T) { Operation: logical.UpdateOperation, Path: "sign/multikey", Data: map[string]interface{}{ - "public_key": publicKeyECDSA256, + "public_key": publicKeyECDSA256, + "valid_principals": "guest", }, ErrorOk: true, Check: func(resp *logical.Response) error { @@ -1420,13 +1502,15 @@ func TestBackend_AllowedUserKeyLengths(t *testing.T) { "ec": []int{256}, "ecdsa-sha2-nistp521": 0, }, + "allowed_users": "guest", }), // Pass with ECDSA P-256 { Operation: logical.UpdateOperation, Path: "sign/ectypes", Data: map[string]interface{}{ - "public_key": publicKeyECDSA256, + "public_key": publicKeyECDSA256, + "valid_principals": "guest", }, }, // Pass with ECDSA P-521 @@ -1434,7 +1518,8 @@ func TestBackend_AllowedUserKeyLengths(t *testing.T) { Operation: logical.UpdateOperation, Path: "sign/ectypes", Data: map[string]interface{}{ - "public_key": publicKeyECDSA521, + "public_key": publicKeyECDSA521, + "valid_principals": "guest", }, }, // Fail with RSA key @@ -1442,7 +1527,8 @@ func TestBackend_AllowedUserKeyLengths(t *testing.T) { Operation: logical.UpdateOperation, Path: "sign/ectypes", Data: map[string]interface{}{ - "public_key": publicKey3072, + "public_key": publicKey3072, + "valid_principals": "guest", }, ErrorOk: true, Check: func(resp *logical.Response) error { @@ -1896,6 +1982,7 @@ func TestSSHBackend_IssueSign(t *testing.T) { "ecdsa-sha2-nistp521": 0, "ed25519": 0, }, + "allow_empty_principals": true, }), // Key_type not in allowed_user_key_types_lengths issueSSHKeyPairStep("testing", "ec", 256, true, "provided key_type value not in allowed_user_key_types"), @@ -2726,13 +2813,14 @@ func TestProperAuthing(t *testing.T) { _, err = client.Logical().WriteWithContext(ctx, "ssh/roles/test-ca", map[string]interface{}{ "key_type": "ca", "allow_user_certificates": true, + "allowed_users": "toor", }) if err != nil { t.Fatal(err) } _, err = client.Logical().WriteWithContext(ctx, "ssh/issue/test-ca", map[string]interface{}{ - "username": "toor", + "valid_principals": "toor", }) if err != nil { t.Fatal(err) diff --git a/builtin/logical/ssh/path_config_ca_test.go b/builtin/logical/ssh/path_config_ca_test.go index a096073c6963..269af3631e5d 100644 --- a/builtin/logical/ssh/path_config_ca_test.go +++ b/builtin/logical/ssh/path_config_ca_test.go @@ -259,6 +259,7 @@ func TestSSH_ConfigCAKeyTypes(t *testing.T) { "key_type": "ca", "ttl": "30s", "not_before_duration": "2h", + "allow_empty_principals": true, } roleReq := &logical.Request{ Operation: logical.UpdateOperation, diff --git a/builtin/logical/ssh/path_issue.go b/builtin/logical/ssh/path_issue.go index 422f6621ee0b..54741e30ab21 100644 --- a/builtin/logical/ssh/path_issue.go +++ b/builtin/logical/ssh/path_issue.go @@ -58,7 +58,7 @@ be later than the role max TTL.`, }, "valid_principals": { Type: framework.TypeString, - Description: `Valid principals, either usernames or hostnames, that the certificate should be signed for.`, + Description: `Valid principals, either usernames or hostnames, that the certificate should be signed for. Must be non-empty unless allow_empty_principals=true (not recommended) or a value for DefaultUser has been set in the role`, }, "cert_type": { Type: framework.TypeString, diff --git a/builtin/logical/ssh/path_issue_sign.go b/builtin/logical/ssh/path_issue_sign.go index 91fca5a40855..60c6d44f7ace 100644 --- a/builtin/logical/ssh/path_issue_sign.go +++ b/builtin/logical/ssh/path_issue_sign.go @@ -189,10 +189,19 @@ func (b *backend) calculateValidPrincipals(data *framework.FieldData, req *logic allowedPrincipals = strutil.RemoveDuplicates(strutil.ParseStringSlice(principalsAllowedByRole, ","), false) } + if len(parsedPrincipals) == 0 && defaultPrincipal != "" { + // defaultPrincipal will either be the defaultUser or a rendered defaultUserTemplate + parsedPrincipals = []string{defaultPrincipal} + } + switch { case len(parsedPrincipals) == 0: - // There is nothing to process - return nil, nil + if role.AllowEmptyPrincipals { + // There is nothing to process + return nil, nil + } else { + return nil, fmt.Errorf("empty valid principals not allowed by role") + } case len(allowedPrincipals) == 0: // User has requested principals to be set, but role is not configured // with any principals diff --git a/builtin/logical/ssh/path_roles.go b/builtin/logical/ssh/path_roles.go index 5e1a00194a95..28fa9a92486d 100644 --- a/builtin/logical/ssh/path_roles.go +++ b/builtin/logical/ssh/path_roles.go @@ -66,6 +66,7 @@ type sshRole struct { AlgorithmSigner string `mapstructure:"algorithm_signer" json:"algorithm_signer"` Version int `mapstructure:"role_version" json:"role_version"` NotBeforeDuration time.Duration `mapstructure:"not_before_duration" json:"not_before_duration"` + AllowEmptyPrincipals bool `mapstructure:"allow_empty_principals" json:"allow_empty_principals"` } func pathListRoles(b *backend) *framework.Path { @@ -363,6 +364,11 @@ func pathRoles(b *backend) *framework.Path { Value: 30, }, }, + "allow_empty_principals": { + Type: framework.TypeBool, + Description: `Whether to allow issuing certificates with no valid principals (meaning any valid principal). Exists for backwards compatibility only, the default of false is highly recommended.`, + Default: false, + }, }, Callbacks: map[logical.Operation]framework.OperationFunc{ @@ -499,6 +505,7 @@ func (b *backend) createCARole(allowedUsers, defaultUser, signer string, data *f AlgorithmSigner: signer, Version: roleEntryVersion, NotBeforeDuration: time.Duration(data.Get("not_before_duration").(int)) * time.Second, + AllowEmptyPrincipals: data.Get("allow_empty_principals").(bool), } if !role.AllowUserCertificates && !role.AllowHostCertificates { diff --git a/changelog/28466.txt b/changelog/28466.txt new file mode 100644 index 000000000000..be9016ec05e8 --- /dev/null +++ b/changelog/28466.txt @@ -0,0 +1,3 @@ +```release-note:change +secrets/ssh: Add a flag, `allow_empty_principals` to allow keys or certs to apply to any user/principal. +``` diff --git a/website/content/api-docs/secret/ssh.mdx b/website/content/api-docs/secret/ssh.mdx index 0a1bee7c04a1..5b60ef5cd293 100644 --- a/website/content/api-docs/secret/ssh.mdx +++ b/website/content/api-docs/secret/ssh.mdx @@ -171,6 +171,10 @@ This endpoint creates or updates a named role. ~> **Note**: In FIPS 140-2 mode, the following algorithms are not certified and thus should not be used: `ed25519`. +- `allow_empty_principals` `(bool: false)` - Allow signing certificates with + no valid principals (e.g. any valid principal). For backwards + compatibility only. The default of false is highly recommended. + - `algorithm_signer` `(string: "default")` - Algorithm to sign keys with. Valid values are `ssh-rsa`, `rsa-sha2-256`, `rsa-sha2-512`, or `default`. This value may also be left blank to use the signer's default algorithm, and must @@ -190,6 +194,10 @@ This endpoint creates or updates a named role. - `not_before_duration` `(duration: "30s")` – Specifies the duration by which to backdate the `ValidAfter` property. Uses [duration format strings](/vault/docs/concepts/duration-format). +- `allow_empty_principals` `(bool: false)` - If true, allows certificates + to be issued against all principals. Highly recommended to use the default of + false. + ### Sample payload ```json @@ -744,7 +752,8 @@ parameters of the issued certificate can be further customized in this API call. set. - `valid_principals` `(string: "")` – Specifies valid principals, either - usernames or hostnames, that the certificate should be signed for. + usernames or hostnames, that the certificate should be signed for. Required + unless the role has specified allow_empty_principals. - `cert_type` `(string: "user")` – Specifies the type of certificate to be created; either "user" or "host". @@ -830,7 +839,8 @@ parameters of the issued certificate can be further customized in this API call. set. - `valid_principals` `(string: "")` – Specifies valid principals, either - usernames or hostnames, that the certificate should be signed for. + usernames or hostnames, that the certificate should be signed for. Required + unless the role has specified allow_empty_principals. - `cert_type` `(string: "user")` – Specifies the type of certificate to be created; either "user" or "host". @@ -926,4 +936,4 @@ $ curl \ "warnings": null, "auth": null } -``` +``` \ No newline at end of file