If generating an SSH CA signing key - return the public part #2483
Conversation
So that the user can actually use the SSH CA, by adding the public key to their respective sshd_config/authorized_keys, etc.
|
|
||
| if generateSigningKey { | ||
| response := &logical.Response{ | ||
| Data: map[string]interface{}{ |
There was a problem hiding this comment.
I wonder if we should have same response both when generateSigningKey is true and when its false. Another thing to notice here is that the response format of the public key here is different than the output of ssh/public_key endpoint (HTTPRawBody in the latter).
@jefferai @briankassouf thoughts?
There was a problem hiding this comment.
I think this is a useful enhancement, and I also think adding a read endpoint to the config/ca path that returns the public key as a normal API object would be good too.
There was a problem hiding this comment.
Sure. I think both keeping this and adding read behavior would be valuable.
There was a problem hiding this comment.
@jefferai I agree, a read operation on the config/ca path would be very useful and logical.
There was a problem hiding this comment.
@vishalnayak I haven't used the other ssh endpoints, cause up until the CA they weren't useful to me. You're right that HTTPRawBody is probably the better way to return this, easier for consistency, scripts, etc. Ultimately, whichever return format is chosen is fine with me.
There was a problem hiding this comment.
We already have an endpoint that exposes it in raw form; we should have it in a normal Vault API response too, so this is useful as-is.
If configured/generated.
| } | ||
|
|
||
| if publicKeyEntry == nil { | ||
| return nil, fmt.Errorf("keys haven't been configured yet") |
There was a problem hiding this comment.
This will cause Vault to spit out 500 errors. Use
return logical.ErrorResponse("....."), nil
instead. Up above since it'd be an error during the read it's probably fine to keep as-is and return a 500.
There was a problem hiding this comment.
Thanks, fixed in dbb66c2
| @@ -228,6 +251,17 @@ func (b *backend) pathConfigCAUpdate(req *logical.Request, data *framework.Field | |||
|
|
|||
| return nil, err | |||
| } | |||
|
|
|||
| if generateSigningKey { | |||
There was a problem hiding this comment.
@jefferai should we remove this check to make the response consistent?
There was a problem hiding this comment.
I don't care either way -- if it's not being generated you already have the public key (and have probably distributed it already too). If you feel strongly about it, then sure, change it.
There was a problem hiding this comment.
I guess I don't feel strongly about it too. Thanks!
| @@ -305,8 +305,10 @@ The first thing to do is to get Vault to generate the key pair that will be | |||
| used to sign any SSH keys: | |||
|
|
|||
| ```text | |||
| $ vault write -f ssh/config/ca | |||
| Success! Data written to: ssh/config/ca | |||
| $ vault write -f ssh/config/ca generate_signing_key=true | |||
There was a problem hiding this comment.
generate_signing_key will be true by default and we don't need to specify it.
There was a problem hiding this comment.
Thanks, fixed in 86849ae
|
LGTM -- @vishalnayak if you agree please merge! |
|
Thanks guys. |
|
Thank you! |
So that the user can actually use the SSH CA, by adding the public key
to their respective sshd_config/authorized_keys, etc.