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

Allow specifying multiple allowed SSH key lengths #13991

Merged
merged 5 commits into from
Feb 17, 2022

Conversation

cipherboy
Copy link
Contributor

@cipherboy cipherboy commented Feb 9, 2022

In the ssh secrets engine, only a single allowed key length was allowed
for each algorithm type. However, many algorithms have multiple safe
values (such as RSA and ECDSA); allowing a single role to have multiple
values for a single algorithm is thus helpful.

On creation or update, roles can now specify multiple types using a list
or comma separated string of allowed values:

allowed_user_key_lengths: map[string][]int{"rsa": []int{2048, 4096}}

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>


As mentioned by @stevendpclark, there was some discussion of map[string][]string to support named curves, but with the exception of x448 (which behaves like x25519 and takes no length), support in cryptography libraries for other named curves besides the 4 we support (+ P-224 which we don't) isn't broad. There's little value in adding them and most all should be happy with the three NIST P-curves and x25519 (+/- x448 if/when we add it).

Since the PKI engine already takes curves-as-bits, I'm fine using this behavior. My 2c.

@cipherboy cipherboy requested a review from a team February 9, 2022 21:38
@cipherboy cipherboy force-pushed the cipherboy-switch-ssh-allowed-key-lengths-to-slice branch from bbb3951 to 7e7e3da Compare February 9, 2022 21:40
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 9, 2022 21:40 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 9, 2022 21:40 Inactive
@cipherboy
Copy link
Contributor Author

I've proposed the parsing in go-secure-stdlib as we probably we prefer that: hashicorp/go-secure-stdlib#28

builtin/logical/ssh/path_roles.go Outdated Show resolved Hide resolved
@cipherboy cipherboy force-pushed the cipherboy-switch-ssh-allowed-key-lengths-to-slice branch from 7e7e3da to c5d7bbf Compare February 10, 2022 18:50
@vercel vercel bot temporarily deployed to Preview – vault February 10, 2022 18:50 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 10, 2022 18:50 Inactive
@cipherboy cipherboy force-pushed the cipherboy-switch-ssh-allowed-key-lengths-to-slice branch from c5d7bbf to 59970c5 Compare February 10, 2022 18:50
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 10, 2022 18:50 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 10, 2022 18:50 Inactive
@sgmiller sgmiller self-requested a review February 14, 2022 18:00
@cipherboy cipherboy requested a review from taoism4504 February 14, 2022 18:13
In the ssh secrets engine, only a single allowed key length was allowed
for each algorithm type. However, many algorithms have multiple safe
values (such as RSA and ECDSA); allowing a single role to have multiple
values for a single algorithm is thus helpful.

On creation or update, roles can now specify multiple types using a list
or comma separated string of allowed values:

    allowed_user_key_lengths: map[string][]int{"rsa": []int{2048, 4096}}

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
    go get -u github.com/hashicorp/go-secure-stdlib/parseutil
    go mod tidy

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
@cipherboy cipherboy force-pushed the cipherboy-switch-ssh-allowed-key-lengths-to-slice branch from 59970c5 to d42d72a Compare February 17, 2022 18:03
@vercel vercel bot temporarily deployed to Preview – vault February 17, 2022 18:03 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 17, 2022 18:03 Inactive
The newly introduced parseutil.ParseIntSlice handles the more
complicated optional int-like slice logic for us.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
@cipherboy cipherboy force-pushed the cipherboy-switch-ssh-allowed-key-lengths-to-slice branch from d42d72a to 93de2c6 Compare February 17, 2022 18:06
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 17, 2022 18:06 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 17, 2022 18:06 Inactive
if kbits == value {
for _, value := range allowed_values {
switch kstr {
case "rsa":
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically correct, but we are now incongruent on the key names we accept for "key types" from PR#14008 and what the names/values we allow for this key. Not the end of the world just pointing it out.

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'll be addressing that in a follow-up PR, its a bit more subtle than I was hoping to do in this PR.

@cipherboy
Copy link
Contributor Author

Thanks all!

@cipherboy cipherboy merged commit 00c3e8f into main Feb 17, 2022
@cipherboy cipherboy deleted the cipherboy-switch-ssh-allowed-key-lengths-to-slice branch February 24, 2022 20:24
@cipherboy cipherboy added this to the 1.10 milestone Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants