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

Add SSH secret backend CA resource #163

Merged

Conversation

arnogeurts
Copy link
Contributor

This pull request partly implements #104

It can be used to provision Vault as a certificate authority:
https://www.vaultproject.io/docs/secrets/ssh/signed-ssh-certificates.html

@ghost ghost added the size/L label Aug 29, 2018
@ghost ghost added the size/L label Aug 30, 2018
Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

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

@arnogeurts thanks for this contribution, it's very helpful! The code looks great, just needs master merged in and a couple minor tweaks due to the way the parameters are handled in Vault.

generateSigningKey := d.Get("generate_signing_key").(bool)

data := map[string]interface{}{
"generate_signing_key": generateSigningKey,
Copy link
Contributor

Choose a reason for hiding this comment

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

I was looking at the code this feeds into here and it actually behaves differently based on whether the generate_signing_key flag was explicitly set to false or simply omitted. So, I think that for this code to behave as expected, we would probably want to only include this flag if it were set.

That would mean that above, we'd want to do:

generateSigningKey, ok := d.Get("generate_signing_key").(bool)
if ok {
    data["generate_signing_key"] = generateSigningKey
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tyrannosaurus-becks thank you very much for your feedback. I added the conditional check to the generate_signing_key property, as suggested above.

data := map[string]interface{}{
"generate_signing_key": generateSigningKey,
}
if !generateSigningKey {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this if shouldn't exist so the code linked above will be fed the way it wants to be.

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 removed this check, so all values are provided to the Vault API if they are provided in the configuration. The Vault API / SDK can decide on whether a given configuration is correct.

privateKey := d.Get("private_key").(string)
publicKey := d.Get("public_key").(string)
if privateKey == "" || publicKey == "" {
return fmt.Errorf("When generate_signing_key is false, both public_key and private_keys should be specified")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this error needed in light of how the code linked above actually does allow this case?

It's useful, though, because we'd only want to populate the public and private key if they were actually provided, in this particular case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See previous comment.

@tyrannosaurus-becks tyrannosaurus-becks self-assigned this Sep 24, 2018
…d-ca

# Conflicts:
#	vault/provider.go
#	website/vault.erb
+ removed context based checks
@ghost ghost added size/XL and removed size/L labels Sep 30, 2018
Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

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

@arnogeurts thank you! This looks great!

@tyrannosaurus-becks tyrannosaurus-becks merged commit 04893e7 into hashicorp:master Oct 5, 2018
dandandy pushed a commit to dandandy/terraform-provider-vault that referenced this pull request Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants