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

not_before_duration added to SSH #15250

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions builtin/logical/ssh/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1682,6 +1682,100 @@ func TestBackend_DefExtTemplatingDisabled(t *testing.T) {
}
}

func TestSSHBackend_ValidateNotBeforeDuration(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("testing", map[string]interface{}{
"key_type": "ca",
"allow_host_certificates": true,
"allowed_domains": "example.com,example.org",
"allow_subdomains": true,
"default_critical_options": map[string]interface{}{
"option": "value",
},
"default_extensions": map[string]interface{}{
"extension": "extended",
},
"not_before_duration": "300s",
}),

signCertificateStep("testing", "vault-root-22608f5ef173aabf700797cb95c5641e792698ec6380e8e1eb55523e39aa5e51", ssh.HostCert, []string{"dummy.example.org", "second.example.com"}, map[string]string{
"option": "value",
}, map[string]string{
"extension": "extended",
},
2*time.Hour+5*time.Minute-30*time.Second, map[string]interface{}{
"public_key": publicKey2,
"ttl": "2h",
"cert_type": "host",
"valid_principals": "dummy.example.org,second.example.com",
}),
Copy link
Contributor

@cipherboy cipherboy May 11, 2022

Choose a reason for hiding this comment

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

As an aside: wow, I know that passes make fmt but that's kinda weird formatting. (Not suggesting you change it or anything, just find it funny).

Copy link
Contributor Author

@Gabrielopesantos Gabrielopesantos May 11, 2022

Choose a reason for hiding this comment

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

Not sure how is the formatting weird there but it seems to be in accordance with go fmt so I don't know how could it be improved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm misreading this diff, it looks like there's an extra level of indentation. It starts by placing all parameters on the same line, indents for the map. Next map goes to the same indentation level as the function invocation (fine), but then we indent for a non-map parameter, add the next map parameter, and indent that again!

Seems like we should outdent 1716 and line everything up nicely, or expand the function invocation so all parameters are on the same level (indented one like 1716), forcing all map's k=v entries to be on the level of 1717 instead of 1712.

But like you said, go fmt is happy with it and other tests in the same file look like that, so I don't really care :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thanks for pointing it out.


createRoleStep("testing", map[string]interface{}{
"key_type": "ca",
"allow_host_certificates": true,
"allowed_domains": "example.com,example.org",
"allow_subdomains": true,
"default_critical_options": map[string]interface{}{
"option": "value",
},
"default_extensions": map[string]interface{}{
"extension": "extended",
},
"not_before_duration": "2h",
}),

signCertificateStep("testing", "vault-root-22608f5ef173aabf700797cb95c5641e792698ec6380e8e1eb55523e39aa5e51", ssh.HostCert, []string{"dummy.example.org", "second.example.com"}, map[string]string{
"option": "value",
}, map[string]string{
"extension": "extended",
},
4*time.Hour-30*time.Second, map[string]interface{}{
"public_key": publicKey2,
"ttl": "2h",
"cert_type": "host",
"valid_principals": "dummy.example.org,second.example.com",
}),
createRoleStep("testing", map[string]interface{}{
"key_type": "ca",
"allow_host_certificates": true,
"allowed_domains": "example.com,example.org",
"allow_subdomains": true,
"default_critical_options": map[string]interface{}{
"option": "value",
},
"default_extensions": map[string]interface{}{
"extension": "extended",
},
"not_before_duration": "30s",
}),

signCertificateStep("testing", "vault-root-22608f5ef173aabf700797cb95c5641e792698ec6380e8e1eb55523e39aa5e51", ssh.HostCert, []string{"dummy.example.org", "second.example.com"}, map[string]string{
"option": "value",
}, map[string]string{
"extension": "extended",
},
2*time.Hour, map[string]interface{}{
"public_key": publicKey2,
"ttl": "2h",
"cert_type": "host",
"valid_principals": "dummy.example.org,second.example.com",
}),
},
}

logicaltest.Test(t, testCase)
}

func getSshCaTestCluster(t *testing.T, userIdentity string) (*vault.TestCluster, string) {
coreConfig := &vault.CoreConfig{
CredentialBackends: map[string]logical.Factory{
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 @@ -255,6 +255,7 @@ func TestSSH_ConfigCAKeyTypes(t *testing.T) {
"allowed_users": "*",
"key_type": "ca",
"ttl": "30s",
"not_before_duration": "2h",
}
roleReq := &logical.Request{
Operation: logical.UpdateOperation,
Expand Down
21 changes: 20 additions & 1 deletion builtin/logical/ssh/path_roles.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const (
// 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 = 2
roleEntryVersion = 3
)

// Structure that represents a role in SSH backend. This is a common role structure
Expand Down Expand Up @@ -65,6 +65,7 @@ type sshRole struct {
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"`
NotBeforeDuration time.Duration `mapstructure:"not_before_duration" json:"not_before_duration"`
}

func pathListRoles(b *backend) *framework.Path {
Expand Down Expand Up @@ -363,6 +364,16 @@ func pathRoles(b *backend) *framework.Path {
Name: "Signing Algorithm",
},
},
"not_before_duration": {
Type: framework.TypeDurationSecond,
Default: 30,
Description: `
The duration that the SSH certificate should be backdated by at issuance.`,
DisplayAttrs: &framework.DisplayAttributes{
Name: "Not before duration",
Value: 30,
},
},
},

Callbacks: map[logical.Operation]framework.OperationFunc{
Expand Down Expand Up @@ -555,6 +566,7 @@ func (b *backend) createCARole(allowedUsers, defaultUser, signer string, data *f
KeyType: KeyTypeCA,
AlgorithmSigner: signer,
Version: roleEntryVersion,
NotBeforeDuration: time.Duration(data.Get("not_before_duration").(int)) * time.Second,
}

if !role.AllowUserCertificates && !role.AllowHostCertificates {
Expand Down Expand Up @@ -672,6 +684,12 @@ func (b *backend) checkUpgrade(ctx context.Context, s logical.Storage, n string,
err = nil
}

if result.Version < 3 {
modified = true
result.NotBeforeDuration = 30 * time.Second
cipherboy marked this conversation as resolved.
Show resolved Hide resolved
result.Version = 3
}

// Add new migrations just before here.
//
// Condition copied from PKI builtin.
Expand Down Expand Up @@ -739,6 +757,7 @@ func (b *backend) parseRole(role *sshRole) (map[string]interface{}, error) {
"default_extensions_template": role.DefaultExtensionsTemplate,
"allowed_user_key_lengths": role.AllowedUserKeyTypesLengths,
"algorithm_signer": role.AlgorithmSigner,
"not_before_duration": role.NotBeforeDuration,
}
case KeyTypeDynamic:
result = map[string]interface{}{
Expand Down
2 changes: 1 addition & 1 deletion builtin/logical/ssh/path_sign.go
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,7 @@ func (b *creationBundle) sign() (retCert *ssh.Certificate, retErr error) {
Key: b.PublicKey,
KeyId: b.KeyID,
ValidPrincipals: b.ValidPrincipals,
ValidAfter: uint64(now.Add(-30 * time.Second).In(time.UTC).Unix()),
ValidAfter: uint64(now.Add(-b.Role.NotBeforeDuration).In(time.UTC).Unix()),
ValidBefore: uint64(now.Add(b.TTL).In(time.UTC).Unix()),
CertType: b.CertificateType,
Permissions: ssh.Permissions{
Expand Down
3 changes: 3 additions & 0 deletions changelog/15250.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
secrets/ssh: Support for `add_before_duration` in SSH
```
1 change: 1 addition & 0 deletions ui/app/models/role-ssh.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const CA_FIELDS = [
'allowSubdomains',
'allowUserKeyIds',
'keyIdFormat',
'notBeforeDuration'
];

export default Model.extend({
Expand Down
2 changes: 2 additions & 0 deletions website/content/api-docs/secret/ssh.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -831,6 +831,8 @@ to the restrictions contained in the role named in the endpoint.
- `extensions` `(map<string|string>: "")` – Specifies a map of the extensions
that the certificate should be signed for. Defaults to none.

- `not_before_duration` `(duration: "30s")` – Specifies the duration by which to backdate the `ValidAfter` property.

### Sample Payload

```json
Expand Down