SSH secrets engine - Enabled creation of key pairs (CA Mode) #15561

Gabrielopesantos · 2022-05-21T17:24:42Z

Description

Issue: #10879
Created a new endpoint (/ssh/issue/:name) that does not take a public_key (as /ssh/sign/:name does), and instead generates a new public/private key pair internally which is returned in the response.

cipherboy

Looking like a great start @Gabrielopesantos!

A few nits here and there around how validation of type/bits parameters occurs, that's tricky but expected :-) In retrospect, I really made that more complicated than I intended to.

You might be able to refactor the existing logic into taking a keyType/keyBits parameter and hoisting the type/bits extraction to a separate method.

And regarding your comment:

Also, for now everything is in a single function but I could also refactor the existing pathSignCertificate to support this new case.

I think I'd maybe take the common logic (124 and down in the new implementation?) and refactor it to a shared call. So three functions (pathSignCertificate, pathIssueCertificate, and perhaps pathSignIssueCertificateHelper or something of the sort).

Just a thought. :-)

Also, as a heads up, this likely won't make RC1 milestone for 1.11 which means it'll likely be slated for 1.12, in case you were wondering :-)

builtin/logical/ssh/path_issue.go

Gabrielopesantos · 2022-05-24T17:35:25Z

Hello @cipherboy,
just pushed a version that should cover most requested changes/comments.

There are two files path_issue.go and path_sign.go using stuff from each other, maybe joining everything a path_issue_sign.go as in the pki package is the way to go.

What is the behavior expected if no value are defined? Currently, the key_type and key_bits do not have any default values set. So, if not filled, key_type will be an empty string and key_bits 0. I think its ok to leave key_bits as it is handled in generateSSHKeyPair, its set to different values depending on the key_type. However, in the case of key_type, if not defined it returns an error. Took a look at other endpoints that have a key_type field and they seem to have "rsa" as default. Is it ok to also define "rsa" in this endpoint?

sgmiller · 2022-05-27T20:28:10Z

Hello @cipherboy, just pushed a version that should cover most requested changes/comments.

There are two files path_issue.go and path_sign.go using stuff from each other, maybe joining everything a path_issue_sign.go as in the pki package is the way to go.

What is the behavior expected if no value are defined? Currently, the key_type and key_bits do not have any default values set. So, if not filled, key_type will be an empty string and key_bits 0. I think its ok to leave key_bits as it is handled in generateSSHKeyPair, its set to different values depending on the key_type. However, in the case of key_type, if not defined it returns an error. Took a look at other endpoints that have a key_type field and they seem to have "rsa" as default. Is it ok to also define "rsa" in this endpoint?

I'm fine with either an error or an implicit default. If the goal is to make the call as simple as possible to make then the default works.

Gabrielopesantos · 2022-05-30T21:51:23Z

Hello, added a few tests which should cover which should the implementation of this new endpoint, as most of it is reusing the existing ssh/sign. Let me know if I am missing any case. With this, I am opening the pull request for review.

Also, while testing found something that I am not sure if it expected. I defined a allowed_user_key_lengths as:
"ssh-rsa": int{2048, 3072, 4096}, "ecdsa-sha2-nistp521": 0, however it seems that it is created as the following.
AllowedUserKeyTypesLengths: map[ecdsa-sha2-nistp521:[0 0] ssh-rsa:[0 0 0 2048 3072 4096]]
Is this expected?

cipherboy

This looks like rather clean, well-written code! The refactor I think helped a fair bit here. I think most of these are rather minor nits.

Oh and a change log entry :-)

builtin/logical/ssh/path_issue.go

builtin/logical/ssh/backend_test.go

Gabrielopesantos · 2022-06-02T16:11:05Z

Hello @cipherboy, made a commit that addresses both comments and proposes a solution for this #15561 (comment) .

There are a few things that still need to be addressed such as help messages, error messages, organizing functions in correct files and creating the changelog. Let me know if am forgetting anything.

builtin/logical/ssh/path_issue.go

cipherboy · 2022-06-06T19:40:32Z

Hey @Gabrielopesantos Sorry for the delay in getting back to this. I think you need to run make fmt but otherwise (minus what you identified in your last comment) this looks good :)

builtin/logical/ssh/path_issue.go

Gabrielopesantos · 2022-06-06T22:13:22Z

No worries, I know this improvement is set for a 1.12.*** milestone so isn't a priority.

I did not include the make fmt changes as it did not touch any of the files I worked on, not sure if should have added anyway.

changelog/15561.txt

cipherboy

Love the refactor! I'll have a colleague check this out before merging though.

stevendpclark

Overall this is really great! I have some small nits but nothing preventing this from merging.

An api documentation update is required for this change though, if you do not feel comfortable writing the updated documentation, we can handle that in a separate PR.

stevendpclark · 2022-06-08T14:11:39Z

builtin/logical/ssh/backend_test.go

+		ErrorOk: true,
+		// Returns like nil, err break this
+		Check: func(resp *logical.Response) error {
+			if expectError {


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?

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?

if resp.Error() != nil { return fmt.Errorf("unexpected error response returned: %v", resp.Error()) }

Yup that's perfect, just an FYI the following is a little more common (but don't bother changing it)

if resp.IsError() { return fmt.Errorf("unexpected error response returned: %v", resp.Error()) }

stevendpclark · 2022-06-08T14:13:34Z

builtin/logical/ssh/backend_test.go

+			}
+
+			if resp.Data["private_key_type"] != keyType {
+				return errors.New("private key type does not match provided key_type")


nit: Could we include expected vs actual within this error please to again help to diagnose future test failures?

stevendpclark · 2022-06-08T14:39:45Z

builtin/logical/ssh/path_issue_sign.go

+	// them as 4xx values
+	keyID, err := b.calculateKeyID(data, req, role, publicKey)
+	if err != nil {
+		return logical.ErrorResponse(err.Error()), err


nit: It's a tad odd that we return an error response and an err, should we not be consistent and return nil, err ?

~~Not to nitpick your nitpick, but this was just a copy/paste refactor of the existing code:~~

vault/builtin/logical/ssh/path_sign.go

Lines 125 to 128 in 7c72d9d

keyID, err := b.calculateKeyID(data, req, role, userPublicKey)

if err != nil {

return logical.ErrorResponse(err.Error()), nil

}

~~And worst, it seems consistently used that way.~~

Never mind, I agree that returning both the formatted stuff and the err is weird. I'd return one or the other like the existing code did.

Actually there is a behavior change here, as the callers are wrapping any error into a logical.ErrorResponse so some existing error scenarios we did return a 500 will be switched to a 400.

Also makes sense, I remember making this change during development because was having some kind of error. The latest commit reverts these returns to how they were initially (also not consistent). If consistency in the return statements is what we are looking for, I can make the change to return nil, err as suggested.

Thanks! What you have changed it too, reverting to what it was, is the correct approach, much appreciated.

Co-authored-by: Alexander Scheel <[email protected]>

…ssages

Gabrielopesantos · 2022-06-08T20:40:23Z

Pushed a commit with possible fixes to the suggestions made, let me know if there is anything that still should be altered.

Regarding the documentation, is it setting up the documentation for the endpoint, like this https://www.vaultproject.io/api-docs/secret/pki#generate-certificate but for the ssh? If yes, I can give it a try.

stevendpclark · 2022-06-08T20:50:07Z

Regarding the documentation, is it setting up the documentation for the endpoint, like this https://www.vaultproject.io/api-docs/secret/pki#generate-certificate but for the ssh? If yes, I can give it a try.

Correct the file to update would be https://github.com/hashicorp/vault/blob/main/website/content/api-docs/secret/ssh.mdx, adding a new section for the new api endpoint added. It should include a short description, parameters, sample payload, request and response sections as well.

Gabrielopesantos · 2022-06-10T10:36:12Z

Pushed a commit with the documentation proposal, it is basically a copy of the existing endpoint (with some minor changes), not sure if it enough or might require extra information.

I think I also found an issue with a previous PR (made by me), the documentation of the ssh/sign endpoint has a field (not_before_duration) that is not supported, as, given the current implementation, it is only in the ssh/roles endpoint.

cipherboy · 2022-06-10T13:47:55Z

This looks excellent @Gabrielopesantos! Going to go ahead and merge this, and I'll get a fix opened for that docs change you noted (needs to be a separate PR as it goes back to 1.11) :)

…7694) Extend the documentation the API endpoint '/ssh/issue/:name' (added in #15561 with v1.12.0) and '/ssh/issue/:name': - Be more specific that the issued certificate uses the defaults given of the role at the given endpoint; and that it is subject to the limitations configured in this role. - Note that the endpoint /ssh/issue/:name is available with v1.12+. - Make it more clear that the generated credentials are only returned but not stored by Vault (not just the generated private key).

Gabrielopesantos added 5 commits May 18, 2022 23:36

Handle func

e2ded84

Update - check if key_type and key_bits are allowed

2a4802b

Update - fields

1c6ef93

Generating keys based on provided key_type and key_bits

61970f0

Returning signed key

4fb2b9d

cipherboy reviewed May 23, 2022

View reviewed changes

builtin/logical/ssh/path_issue.go Outdated Show resolved Hide resolved

builtin/logical/ssh/path_issue.go Outdated Show resolved Hide resolved

builtin/logical/ssh/path_issue.go Outdated Show resolved Hide resolved

cipherboy reviewed May 23, 2022

View reviewed changes

builtin/logical/ssh/path_issue.go Outdated Show resolved Hide resolved

cipherboy requested a review from a team May 23, 2022 12:21

cipherboy added enhancement secret/ssh labels May 23, 2022

cipherboy added this to the 1.12.0-rc1 milestone May 23, 2022

Refactor

b4ad75b

Gabrielopesantos commented May 23, 2022

View reviewed changes

builtin/logical/ssh/path_issue.go Outdated Show resolved Hide resolved

Refactor update to common logic function

529c494

Descriptions

c859f33

Tests added

d897316

Gabrielopesantos marked this pull request as ready for review May 30, 2022 21:51

cipherboy mentioned this pull request May 31, 2022

bug(parseutil): SafeParseIntSlice leading zeros hashicorp/go-secure-stdlib#40

Merged

cipherboy reviewed Jun 1, 2022

View reviewed changes

builtin/logical/ssh/path_issue.go Outdated Show resolved Hide resolved

builtin/logical/ssh/path_issue.go Outdated Show resolved Hide resolved

builtin/logical/ssh/backend_test.go Outdated Show resolved Hide resolved

Suggested changes and tests added and refactored

e69b586

cipherboy reviewed Jun 6, 2022

View reviewed changes

builtin/logical/ssh/path_issue.go Outdated Show resolved Hide resolved

cipherboy reviewed Jun 6, 2022

View reviewed changes

builtin/logical/ssh/path_issue.go Outdated Show resolved Hide resolved

cipherboy reviewed Jun 6, 2022

View reviewed changes

builtin/logical/ssh/path_issue.go Outdated Show resolved Hide resolved

Gabrielopesantos added 2 commits June 6, 2022 23:05

Suggested changes and fmt run

5d74f7b

File refactoring

14fcc5e

Changelog file

5fd4e39

cipherboy reviewed Jun 8, 2022

View reviewed changes

changelog/15561.txt Outdated Show resolved Hide resolved

cipherboy approved these changes Jun 8, 2022

View reviewed changes

cipherboy requested a review from a team June 8, 2022 13:39

stevendpclark approved these changes Jun 8, 2022

View reviewed changes

Gabrielopesantos and others added 2 commits June 8, 2022 19:05

Update changelog/15561.txt

0c89092

Co-authored-by: Alexander Scheel <[email protected]>

Suggested changes - consistent returns and additional info to test me…

545791e

…ssages

ssh issue key pair documentation

7d1a812

Gabrielopesantos requested a review from taoism4504 as a code owner June 10, 2022 10:20

cipherboy merged commit f2af7f1 into hashicorp:main Jun 10, 2022

bernhardkaindl mentioned this pull request Oct 27, 2022

website: Update api-docs for /ssh/sign/:name and /ssh/issue/:name #17694

Merged

hc-github-team-secure-vault-core mentioned this pull request Oct 27, 2022

Backport of website: Update api-docs for /ssh/sign/:name and /ssh/issue/:name into release/1.12.x #17702

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSH secrets engine - Enabled creation of key pairs (CA Mode) #15561

SSH secrets engine - Enabled creation of key pairs (CA Mode) #15561

Gabrielopesantos commented May 21, 2022 •

edited

Loading

cipherboy left a comment •

edited

Loading

Gabrielopesantos commented May 24, 2022 •

edited

Loading

sgmiller commented May 27, 2022

Gabrielopesantos commented May 30, 2022

cipherboy left a comment •

edited

Loading

Gabrielopesantos commented Jun 2, 2022 •

edited

Loading

cipherboy commented Jun 6, 2022

Gabrielopesantos commented Jun 6, 2022 •

edited

Loading

cipherboy left a comment

stevendpclark left a comment

stevendpclark Jun 8, 2022

Gabrielopesantos Jun 8, 2022 •

edited

Loading

stevendpclark Jun 8, 2022

stevendpclark Jun 8, 2022

stevendpclark Jun 8, 2022

cipherboy Jun 8, 2022 •

edited

Loading

stevendpclark Jun 8, 2022

Gabrielopesantos Jun 8, 2022

stevendpclark Jun 8, 2022

Gabrielopesantos commented Jun 8, 2022 •

edited

Loading

stevendpclark commented Jun 8, 2022

Gabrielopesantos commented Jun 10, 2022 •

edited

Loading

cipherboy commented Jun 10, 2022

	keyID, err := b.calculateKeyID(data, req, role, userPublicKey)
	if err != nil {
	return logical.ErrorResponse(err.Error()), nil
	}

SSH secrets engine - Enabled creation of key pairs (CA Mode) #15561

SSH secrets engine - Enabled creation of key pairs (CA Mode) #15561

Conversation

Gabrielopesantos commented May 21, 2022 • edited Loading

Description

cipherboy left a comment • edited Loading

Choose a reason for hiding this comment

Gabrielopesantos commented May 24, 2022 • edited Loading

sgmiller commented May 27, 2022

Gabrielopesantos commented May 30, 2022

cipherboy left a comment • edited Loading

Choose a reason for hiding this comment

Gabrielopesantos commented Jun 2, 2022 • edited Loading

cipherboy commented Jun 6, 2022

Gabrielopesantos commented Jun 6, 2022 • edited Loading

cipherboy left a comment

Choose a reason for hiding this comment

stevendpclark left a comment

Choose a reason for hiding this comment

stevendpclark Jun 8, 2022

Choose a reason for hiding this comment

Gabrielopesantos Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

stevendpclark Jun 8, 2022

Choose a reason for hiding this comment

stevendpclark Jun 8, 2022

Choose a reason for hiding this comment

stevendpclark Jun 8, 2022

Choose a reason for hiding this comment

cipherboy Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

stevendpclark Jun 8, 2022

Choose a reason for hiding this comment

Gabrielopesantos Jun 8, 2022

Choose a reason for hiding this comment

stevendpclark Jun 8, 2022

Choose a reason for hiding this comment

Gabrielopesantos commented Jun 8, 2022 • edited Loading

stevendpclark commented Jun 8, 2022

Gabrielopesantos commented Jun 10, 2022 • edited Loading

cipherboy commented Jun 10, 2022

Gabrielopesantos commented May 21, 2022 •

edited

Loading

cipherboy left a comment •

edited

Loading

Gabrielopesantos commented May 24, 2022 •

edited

Loading

cipherboy left a comment •

edited

Loading

Gabrielopesantos commented Jun 2, 2022 •

edited

Loading

Gabrielopesantos commented Jun 6, 2022 •

edited

Loading

Gabrielopesantos Jun 8, 2022 •

edited

Loading

cipherboy Jun 8, 2022 •

edited

Loading

Gabrielopesantos commented Jun 8, 2022 •

edited

Loading

Gabrielopesantos commented Jun 10, 2022 •

edited

Loading