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

SSH: report signing error reason, and clarify docs re. non-RSA CA keys #11036

Merged
merged 2 commits into from
Oct 15, 2021

Conversation

candlerb
Copy link
Contributor

@candlerb candlerb commented Mar 3, 2021

See #10067

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 3, 2021

CLA assistant check
All committers have signed the CLA.

@candlerb
Copy link
Contributor Author

candlerb commented Mar 3, 2021

CircleCI build failures appear to be unrelated to this PR

values are `ssh-rsa`, `rsa-sha2-256`, and `rsa-sha2-512`. Note that `ssh-rsa`
is now considered insecure and is not supported by current OpenSSH versions.
If not specified, it will use the signer's default algorithm.
values when the CA has an RSA key are `ssh-rsa`, `rsa-sha2-256`, and
Copy link
Contributor

Choose a reason for hiding this comment

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

This part of the comment (i.e., "when the CA has an RSA key") seems redundant as the signing algorithms clearly mentions RSA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this amendment, it could be construed that that ssh-rsa, rsa-sha2-256 and rsa-sha2-512 are the only permitted signing algorithms - and therefore that only RSA signing keys are supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

The last sentence ("This value must be left blank for CA key types other than RSA.") suffices for such clarification.

Comment on lines 647 to 648
you can add it to your configuration. The key generated will be RSA, but
other types of CA key can be imported if `generate_signing_key` is false.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
you can add it to your configuration. The key generated will be RSA, but
other types of CA key can be imported if `generate_signing_key` is false.
you can add it to your configuration. If `generate_signing_key` is true, the generated signing key is of type
RSA. If `generate_signing_key` is false, the stored CA key is used which can be of any valid signing key type.

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've changed the wording here, what do you think?

  • generate_signing_key (bool: true) – Specifies if Vault should generate
    the signing key pair internally. If true, an RSA key pair is generated, and
    the generated public key returned so you can add it to your configuration.
    If false, then you must provide private_key and public_key, but these
    can be of any valid signing key type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

@hghaf099
Copy link
Contributor

@candlerb would you please merge in the latest changes in main so that the checks and tests pass?

@candlerb
Copy link
Contributor Author

Patch rebased, and slightly different wording proposed for algorithm_signer.

@candlerb
Copy link
Contributor Author

CI failed, looks like an issue cloning the github repo:

Using SSH Config Dir '/home/circleci/.ssh'
git version 2.14.2
Cloning git repository
Cloning into '.'...
Warning: Permanently added the RSA host key for IP address '140.82.114.3' to the list of known hosts.
remote: Enumerating objects: 209481, done.        
remote: Counting objects: 100% (3606/3606), done.        
remote: Compressing objects: 100% (1816/1816), done.        
Write failed: Broken pipe28173/209481), 20.71 MiB | 19.00 KiB/s   
fatal: The remote end hung up unexpectedly
fatal: early EOF
fatal: index-pack failed

exit status 128
CircleCI received exit code 128

Copy link
Contributor

@taoism4504 taoism4504 left a comment

Choose a reason for hiding this comment

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

one slight suggestion but other LTGM.

Co-authored-by: Loann Le <84412881+taoism4504@users.noreply.github.com>
@candlerb
Copy link
Contributor Author

For reference, originally this PR included the signing error reason:

--- a/builtin/logical/ssh/path_sign.go
+++ b/builtin/logical/ssh/path_sign.go
@@ -532,7 +532,7 @@ func (b *creationBundle) sign() (retCert *ssh.Certificate, retErr error) {
        algo := b.Role.AlgorithmSigner
        sig, err := sshAlgorithmSigner.SignWithAlgorithm(rand.Reader, certificateBytes, algo)
        if err != nil {
-               return nil, fmt.Errorf("failed to generate signed SSH key: sign error")
+               return nil, fmt.Errorf("failed to generate signed SSH key: sign error: %w", err)
        }

        certificate.Signature = sig

However that has already been applied in commits 6960b76 and 7ca2caf.

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.

4 participants