-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
SSH secrets engine - Enabled creation of key pairs (CA Mode) #15561
Merged
cipherboy
merged 16 commits into
hashicorp:main
from
Gabrielopesantos:ssh-secrets-engine-create-key-pairs
Jun 10, 2022
Merged
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
e2ded84
Handle func
Gabrielopesantos 2a4802b
Update - check if key_type and key_bits are allowed
Gabrielopesantos 1c6ef93
Update - fields
Gabrielopesantos 61970f0
Generating keys based on provided key_type and key_bits
Gabrielopesantos 4fb2b9d
Returning signed key
Gabrielopesantos b4ad75b
Refactor
Gabrielopesantos 529c494
Refactor update to common logic function
Gabrielopesantos c859f33
Descriptions
Gabrielopesantos d897316
Tests added
Gabrielopesantos e69b586
Suggested changes and tests added and refactored
Gabrielopesantos 5d74f7b
Suggested changes and fmt run
Gabrielopesantos 14fcc5e
File refactoring
Gabrielopesantos 5fd4e39
Changelog file
Gabrielopesantos 0c89092
Update changelog/15561.txt
Gabrielopesantos 545791e
Suggested changes - consistent returns and additional info to test me…
Gabrielopesantos 7d1a812
ssh issue key pair documentation
Gabrielopesantos File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1776,6 +1776,50 @@ func TestSSHBackend_ValidateNotBeforeDuration(t *testing.T) { | |
logicaltest.Test(t, testCase) | ||
} | ||
|
||
func TestSSHBackend_IssueSign(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": "otp", | ||
"default_user": "user", | ||
}), | ||
// Key pair not issued with invalid role key type | ||
issueSSHKeyPairStep("testing", "rsa", 0, true, "role key type 'otp' not allowed to issue key pairs"), | ||
|
||
createRoleStep("testing", map[string]interface{}{ | ||
"key_type": "ca", | ||
"allow_user_key_ids": false, | ||
"allow_user_certificates": true, | ||
"allowed_user_key_lengths": map[string]interface{}{ | ||
"ssh-rsa": []int{2048, 3072, 4096}, | ||
"ecdsa-sha2-nistp521": 0, | ||
"ed25519": 0, | ||
}, | ||
}), | ||
// Key_type not in allowed_user_key_types_lengths | ||
issueSSHKeyPairStep("testing", "ec", 256, true, "provided key_type value not in allowed_user_key_types"), | ||
// Key_bits not in allowed_user_key_types_lengths for provided key_type | ||
issueSSHKeyPairStep("testing", "rsa", 2560, true, "provided key_bits value not in list of role's allowed_user_key_types"), | ||
// key_type `rsa` and key_bits `2048` successfully created | ||
issueSSHKeyPairStep("testing", "rsa", 2048, false, ""), | ||
// key_type `ed22519` and key_bits `0` successfully created | ||
issueSSHKeyPairStep("testing", "ed25519", 0, false, ""), | ||
}, | ||
} | ||
|
||
logicaltest.Test(t, testCase) | ||
} | ||
|
||
func getSshCaTestCluster(t *testing.T, userIdentity string) (*vault.TestCluster, string) { | ||
coreConfig := &vault.CoreConfig{ | ||
CredentialBackends: map[string]logical.Factory{ | ||
|
@@ -1847,7 +1891,8 @@ func getSshCaTestCluster(t *testing.T, userIdentity string) (*vault.TestCluster, | |
} | ||
|
||
func testAllowedUsersTemplate(t *testing.T, testAllowedUsersTemplate string, | ||
expectedValidPrincipal string, testEntityMetadata map[string]string) { | ||
expectedValidPrincipal string, testEntityMetadata map[string]string, | ||
) { | ||
cluster, userpassToken := getSshCaTestCluster(t, testUserName) | ||
defer cluster.Cleanup() | ||
client := cluster.Cores[0].Client | ||
|
@@ -1926,7 +1971,8 @@ func signCertificateStep( | |
role, keyID string, certType int, validPrincipals []string, | ||
criticalOptionPermissions, extensionPermissions map[string]string, | ||
ttl time.Duration, | ||
requestParameters map[string]interface{}) logicaltest.TestStep { | ||
requestParameters map[string]interface{}, | ||
) logicaltest.TestStep { | ||
return logicaltest.TestStep{ | ||
Operation: logical.UpdateOperation, | ||
Path: "sign/" + role, | ||
|
@@ -1955,6 +2001,39 @@ func signCertificateStep( | |
} | ||
} | ||
|
||
func issueSSHKeyPairStep(role, keyType string, keyBits int, expectError bool, errorMsg string) logicaltest.TestStep { | ||
return logicaltest.TestStep{ | ||
Operation: logical.UpdateOperation, | ||
Path: "issue/" + role, | ||
Data: map[string]interface{}{ | ||
"key_type": keyType, | ||
"key_bits": keyBits, | ||
}, | ||
ErrorOk: true, | ||
// Returns like nil, err break this | ||
Check: func(resp *logical.Response) error { | ||
if expectError { | ||
var err error | ||
if resp.Data["error"] != errorMsg { | ||
err = fmt.Errorf("actual error message \"%s\" different from expected error message \"%s\"", resp.Data["error"], errorMsg) | ||
} | ||
|
||
return err | ||
} | ||
|
||
if resp.Data["private_key_type"] != keyType { | ||
return errors.New("private key type does not match provided key_type") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Could we include expected vs actual within this error please to again help to diagnose future test failures? |
||
} | ||
|
||
if resp.Data["signed_key"] == "" { | ||
return errors.New("certificate/signed_key should not be empty") | ||
} | ||
|
||
return nil | ||
}, | ||
} | ||
} | ||
|
||
func validateSSHCertificate(cert *ssh.Certificate, keyID string, certType int, validPrincipals []string, criticalOptionPermissions, extensionPermissions map[string]string, | ||
ttl time.Duration, | ||
) error { | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,180 @@ | ||
package ssh | ||
|
||
import ( | ||
"context" | ||
"crypto/rand" | ||
"errors" | ||
"fmt" | ||
|
||
"github.com/hashicorp/vault/sdk/framework" | ||
"github.com/hashicorp/vault/sdk/logical" | ||
) | ||
|
||
type keySpecs struct { | ||
Type string | ||
Bits int | ||
} | ||
|
||
func pathIssue(b *backend) *framework.Path { | ||
return &framework.Path{ | ||
Pattern: "issue/" + framework.GenericNameWithAtRegex("role"), | ||
|
||
Operations: map[logical.Operation]framework.OperationHandler{ | ||
logical.UpdateOperation: &framework.PathOperation{ | ||
Callback: b.pathIssue, | ||
}, | ||
}, | ||
Fields: map[string]*framework.FieldSchema{ | ||
"role": { | ||
Type: framework.TypeString, | ||
Description: `The desired role with configuration for this request.`, | ||
}, | ||
"key_type": { | ||
Type: framework.TypeString, | ||
Description: "Specifies the desired key type; must be `rsa`, `ed25519` or `ec`", | ||
Default: "rsa", | ||
}, | ||
"key_bits": { | ||
Type: framework.TypeInt, | ||
Description: "Specifies the number of bits to use for the generated keys.", | ||
Default: 0, | ||
}, | ||
"ttl": { | ||
Type: framework.TypeDurationSecond, | ||
Description: `The requested Time To Live for the SSH certificate; | ||
sets the expiration date. If not specified | ||
the role default, backend default, or system | ||
default TTL is used, in that order. Cannot | ||
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.`, | ||
}, | ||
"cert_type": { | ||
Type: framework.TypeString, | ||
Description: `Type of certificate to be created; either "user" or "host".`, | ||
Default: "user", | ||
}, | ||
"key_id": { | ||
Type: framework.TypeString, | ||
Description: `Key id that the created certificate should have. If not specified, the display name of the token will be used.`, | ||
}, | ||
"critical_options": { | ||
Type: framework.TypeMap, | ||
Description: `Critical options that the certificate should be signed for.`, | ||
}, | ||
"extensions": { | ||
Type: framework.TypeMap, | ||
Description: `Extensions that the certificate should be signed for.`, | ||
}, | ||
}, | ||
HelpSynopsis: pathIssueHelpSyn, | ||
HelpDescription: pathIssueHelpDesc, | ||
} | ||
} | ||
|
||
func (b *backend) pathIssue(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { | ||
// Get the role | ||
roleName := data.Get("role").(string) | ||
role, err := b.getRole(ctx, req.Storage, roleName) | ||
if err != nil { | ||
return nil, err | ||
} | ||
if role == nil { | ||
return logical.ErrorResponse(fmt.Sprintf("unknown role: %s", roleName)), nil | ||
} | ||
|
||
if role.KeyType != "ca" { | ||
return logical.ErrorResponse("role key type '%s' not allowed to issue key pairs", role.KeyType), nil | ||
} | ||
|
||
// Validate and extract key specifications | ||
keySpecs, err := extractKeySpecs(role, data) | ||
if err != nil { | ||
return logical.ErrorResponse(err.Error()), nil | ||
} | ||
|
||
// Issue certificate | ||
return b.pathIssueCertificate(ctx, req, data, role, keySpecs) | ||
} | ||
|
||
func (b *backend) pathIssueCertificate(ctx context.Context, req *logical.Request, data *framework.FieldData, role *sshRole, keySpecs *keySpecs) (*logical.Response, error) { | ||
publicKey, privateKey, err := generateSSHKeyPair(rand.Reader, keySpecs.Type, keySpecs.Bits) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
// Sign key | ||
userPublicKey, err := parsePublicSSHKey(publicKey) | ||
if err != nil { | ||
return logical.ErrorResponse(fmt.Sprintf("failed to parse public_key as SSH key: %s", err)), nil | ||
} | ||
|
||
response, err := b.pathSignIssueCertificateHelper(ctx, req, data, role, userPublicKey) | ||
if err != nil { | ||
return logical.ErrorResponse(err.Error()), nil | ||
} | ||
|
||
// Additional to sign response | ||
response.Data["private_key"] = privateKey | ||
response.Data["private_key_type"] = keySpecs.Type | ||
|
||
return response, nil | ||
} | ||
|
||
func extractKeySpecs(role *sshRole, data *framework.FieldData) (*keySpecs, error) { | ||
keyType := data.Get("key_type").(string) | ||
keyBits := data.Get("key_bits").(int) | ||
keySpecs := keySpecs{ | ||
Type: keyType, | ||
Bits: keyBits, | ||
} | ||
|
||
keyTypeToMapKey := createKeyTypeToMapKey(keyType, keyBits) | ||
|
||
if len(role.AllowedUserKeyTypesLengths) != 0 { | ||
var keyAllowed bool | ||
var bitsAllowed bool | ||
|
||
keyTypeAliasesLoop: | ||
for _, keyTypeAlias := range keyTypeToMapKey[keyType] { | ||
allowedValues, allowed := role.AllowedUserKeyTypesLengths[keyTypeAlias] | ||
if !allowed { | ||
continue | ||
} | ||
keyAllowed = true | ||
|
||
for _, value := range allowedValues { | ||
if value == keyBits { | ||
bitsAllowed = true | ||
break keyTypeAliasesLoop | ||
} | ||
} | ||
} | ||
|
||
if !keyAllowed { | ||
return nil, errors.New("provided key_type value not in allowed_user_key_types") | ||
} | ||
|
||
if !bitsAllowed { | ||
return nil, errors.New("provided key_bits value not in list of role's allowed_user_key_types") | ||
} | ||
} | ||
|
||
return &keySpecs, nil | ||
} | ||
|
||
const pathIssueHelpSyn = ` | ||
Request a certificate using a certain role with the provided details. | ||
` | ||
|
||
const pathIssueHelpDesc = ` | ||
This path allows requesting a certificate to be issued according to the | ||
policy of the given role. The certificate will only be issued if the | ||
requested details are allowed by the role policy. | ||
|
||
This path returns a certificate and a private key. If you want a workflow | ||
that does not expose a private key, generate a CSR locally and use the | ||
sign path instead. | ||
` |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because ErrorOk is set to true above, this check function should perhaps validate that resp is not in an error state when expectError is false? At least it would help to diagnose test failures if we returned the error in resp if it was unexpected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback, I agree with the suggestion. Would something like the following after the
if expectError
block be enough to handle it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup that's perfect, just an FYI the following is a little more common (but don't bother changing it)