Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make reception of an empty valid principals configurable based on a role flag. #28466

Merged
merged 8 commits into from
Sep 23, 2024
106 changes: 97 additions & 9 deletions builtin/logical/ssh/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1298,6 +1298,80 @@
logicaltest.Test(t, testCase)
}

func TestBackend_EmptyPrincipals(t *testing.T) {

Check failure on line 1301 in builtin/logical/ssh/backend_test.go

View workflow job for this annotation

GitHub Actions / Code checks

Test TestBackend_EmptyPrincipals is missing a go doc
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()

Expand All @@ -1315,6 +1389,7 @@
"allowed_user_key_lengths": map[string]interface{}{
"rsa": 4096,
},
"allowed_users": "guest",
}),
{
Operation: logical.UpdateOperation,
Expand All @@ -1336,21 +1411,24 @@
"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
{
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 {
Expand All @@ -1366,21 +1444,24 @@
"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
{
Operation: logical.UpdateOperation,
Path: "sign/multikey",
Data: map[string]interface{}{
"public_key": publicKey4096,
"public_key": publicKey4096,
"valid_principals": "guest",
},
},
// Fail with 3072-bit key
Expand All @@ -1403,7 +1484,8 @@
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 {
Expand All @@ -1420,29 +1502,33 @@
"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
{
Operation: logical.UpdateOperation,
Path: "sign/ectypes",
Data: map[string]interface{}{
"public_key": publicKeyECDSA521,
"public_key": publicKeyECDSA521,
"valid_principals": "guest",
},
},
// Fail with RSA key
{
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 {
Expand Down Expand Up @@ -1896,6 +1982,7 @@
"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"),
Expand Down Expand Up @@ -2726,13 +2813,14 @@
_, 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)
Expand Down
1 change: 1 addition & 0 deletions builtin/logical/ssh/path_config_ca_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion builtin/logical/ssh/path_issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
13 changes: 11 additions & 2 deletions builtin/logical/ssh/path_issue_sign.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions builtin/logical/ssh/path_roles.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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 {
Expand Down
3 changes: 3 additions & 0 deletions changelog/28466.txt
Original file line number Diff line number Diff line change
@@ -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.
```
16 changes: 13 additions & 3 deletions website/content/api-docs/secret/ssh.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if the absence of the empty line matters or not. The Vercel step was skipped by the PR build, so I couldn't verify it.

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
Expand All @@ -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
Expand Down Expand Up @@ -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".
Expand Down Expand Up @@ -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".
Expand Down Expand Up @@ -926,4 +936,4 @@ $ curl \
"warnings": null,
"auth": null
}
```
```
Loading