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

docs: add detailed instructions for setting up external root CA #12393

Closed
wants to merge 5 commits into from

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Feb 18, 2022

Branched from #11910, this PR adds detailed instructions for how to use the new feature.

@dnephin dnephin added the pr/no-changelog PR does not need a corresponding .changelog entry label Feb 18, 2022
@dnephin dnephin requested a review from a team as a code owner February 18, 2022 20:48
@github-actions github-actions bot added the type/docs Documentation needs to be created/updated/clarified label Feb 18, 2022
@dnephin dnephin requested a review from a team February 18, 2022 20:55
Base automatically changed from dnephin/ca-provider-interface-for-ica-in-primary to main February 22, 2022 18:14
Copy link
Contributor

@karl-cardenas-coding karl-cardenas-coding left a comment

Choose a reason for hiding this comment

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

@trujillo-adam this looks good to me but let's tag @webdog and @danielehc to review this as they recently interacted heavily with this doc page.

Copy link
Contributor

@danielehc danielehc left a comment

Choose a reason for hiding this comment

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

LGTM. I like the tables to show the different options and the explanation on the different necessity of each field.

Copy link

@webdog webdog left a comment

Choose a reason for hiding this comment

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

I'm loving this improvement! I've left some comments, some grammatical fixes, and some feedback on content. Overall looking great!

website/content/docs/connect/ca/vault.mdx Outdated Show resolved Hide resolved
Comment on lines 84 to 98
| API Key, Agent Key | Description | Type | Required |
| :-- | :-- | :-- | :-- |
| `Address`, <br/>`address` | Specifies the address of the Vault server. | String | Required |
| `Token`, <br/>`token` | Specifies an ACL token for accessing Vault. <br/>The value specified in the configuration is write-only and is not exposed when Vault reads the CA configuration. <br/>The token must be linked to a policy that provides access to the PKI path (see [Vault ACL Policies](#vault-acl-policies)). <br/>If the [`renewable`](https://www.vaultproject.io/api-docs/auth/token#renewable) flag is enabled for the token, Consul will periodically attempt to renew its lease after half of the duration has passed. <br/>_You must either provide a token or configure an `AuthMethod`, `auth_method`_. | String | Required unless `AuthMethod`, `auth_method` is configured. |
| `AuthMethod`, <br/>`auth_method` | Specifies a Vault auth method for logging into Vault. <br/>If an auth method is configured, Consul obtains a new token from Vault when the current token can no longer be renewed. <br/>See [Auth Methods](#auth-methods) for details. | Map | Required unless `Token`, `token` is configured. |
| `RootPKIPath`, <br/>`root_pki_path` | Specifies the path to a PKI secrets engine so that Consul can access the root certificate. See [Root and Intermediate PKI Paths](#root-and-intermediate-pki-paths) for details. | String | Optional |
| `IntermediatePKIPath`, <br/>`intermediate_pki_path` | Specifies the path to a PKI secrets engine for the generated intermediate certificate. <br/>The certificate is signed by the secrets engine specified with the `RootPKIPath`, `root_pki_path` parameter. If the engine is not specified, Consul will attempt to mount and configure the engine automatically. <br/> When WAN Federation is enabled, every secondary datacenter must specify a unique `intermediate_pki_path`. <br/>See [Root and Intermediate PKI Paths](#root-and-intermediate-pki-paths) for additional information. | String | Required |
| `CAFile`, <br/>`ca_file` | Specifies a path to the CA certificate used for Vault communication. <br/>If unspecified, the default system CA bundle is used (refer to the documentation for your OS and version). | String | Optional |
| `CAPath`, <br/>`ca_path` | Specifies a path to a folder containing CA certificates to be used for Vault communication. If unspecified, the default system CA bundle is used (refer to the documentation for your OS and version). | String | Optional |
| `CertFile`, <br/>`cert_file` | Specifies the path to the certificate used for Vault communication. | String | Required when `KeyFile`, `key_file` is configured |
| `KeyFile`, <br/>`key_file` | Specifies the path to the private key used for Vault communication. | String | Required when `CertFile`, `cert_file` is configured. |
| `TLSServerName`, <br/>`tls_server_name` | Specifies the SNI host when connecting to Vault via TLS. | String | Optional |
| `TLSSkipVerify`, <br/>`tls_skip_verify` | Enables SSL peer validation enforcement. Default is `false`. | Boolean | Optional |
| `Namespace`, <br/>`namespace` | Specifies the Vault namespace that the `Token` and PKI certificates are a part of. Requries Vault Enterprise (see [Namespaces](https://www.vaultproject.io/docs/enterprise/namespaces) for additional information). | String | Optional |

Copy link

Choose a reason for hiding this comment

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

I really like this table, it helps practitioners understand all of the Vault CA configuration options 😸

I think you could consider renaming the first column's headers, for simplicity and clarity for the practitioner. The informally agreed-on definition of an API Key in software is an authentication identifier or mechanism of some type (Searching for API Key on google yields similar feedback). I think a practitioner may have the opportunity to get mixed up here when building their mental model of the configuration options available to them.

I see that we do use a callout for similar language in the docs/connectca/awsconfiguration section:

https://github.com/hashicorp/consul/blame/main/website/content/docs/connect/ca/aws.mdx#L92L93

Note: The first key is the value used in API calls, and the second key
(after the /) is used if you are adding the configuration to the agent's
configuration file.

I think there's an opportunity here for a bit more clarity. In thinking of RESTful APIs, nouns are referred to as a resource. Could "Consul API Resource" make sense? @karl-cardenas-coding @blake any thoughts here?

Similarly "Agent Key' might benefit from a rename. "Configuration parameter" as a suggestion?

In any circumstance, I'd recommend splitting the column into two unique columns, as each value represents a unique idea regarding configuration. This could simplify the Required column by then only referring to the API Key/Resource, saving some whitespace for the reader in the browser :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the construct you called out was what I was trying to get away from. I can see how the term "API Key" could be confusing. I would still like to keep them in the same column to reinforce the idea that the keywords perform the same function. How about "Parameter (API, Agent)"? Or just "API, Agent"? "Resource, Agent"? I have such a love/hate relationship with md tables.

Copy link
Member

Choose a reason for hiding this comment

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

The API takes a JSON object as input. In JSON, the keys of an object are referred to as a field.

Would it make sense to break these into two separate columns with the following headings?

Configuration parameter API field Description Type Required
Address address Specifies the address of the Vault server. String Yes

website/content/docs/connect/ca/vault.mdx Outdated Show resolved Hide resolved
website/content/docs/connect/ca/vault.mdx Outdated Show resolved Hide resolved
website/content/docs/connect/ca/vault.mdx Show resolved Hide resolved
website/content/docs/connect/ca/vault.mdx Outdated Show resolved Hide resolved
Comment on lines +155 to +156
1. Enable the PKI secrets engine at the path (`connect_root`) that will be used as
`RootPKIPath`. Optionally you may also tune the path using `vault secrets tune`.
Copy link

Choose a reason for hiding this comment

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

These two sentences could benefit from clarity:

  • What does "that will be used as RootPKIPath mean to the practitioner? Is the path stored as that value for that resource/configuration?

  • What benefit does tuning the secrets path provider to the practitioner in context to this guide?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. Enable the PKI secrets engine at the path (`connect_root`) that will be used as
`RootPKIPath`. Optionally you may also tune the path using `vault secrets tune`.
1. Enable the PKI secrets engine at the path (`connect_root`) that will be used as
`RootPKIPath` in the Connect CA configuration. Optionally you may also tune the path using `vault secrets tune`.

Copy link
Member

Choose a reason for hiding this comment

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

Tune apparently allows customizing details of the PKI path. https://www.vaultproject.io/docs/commands/secrets/tune

Comment on lines +168 to +170
1. Use the external root CA to sign the CSR. The exact steps depend on where the root
CA is stored. If you are using vault to store the root CA use
[sign-intermediate](https://www.vaultproject.io/api-docs/secret/pki#sign-intermediate).
Copy link

Choose a reason for hiding this comment

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

Where was the external Root CA sourced? Vault or Consul? Elsewhere?

Copy link
Member

Choose a reason for hiding this comment

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

This enhancement assumes that the external root CA is stored in a location that is completely external to Consul or Vault, such as protected in an HSM. The specifics of signing keys from this system will vary based on the customer's environment.

website/content/docs/connect/ca/vault.mdx Show resolved Hide resolved
Comment on lines +179 to +180
1. Write the bundle.pem to the `RootPKIPath` with
[`set-signed`](https://www.vaultproject.io/api-docs/secret/pki#set-signed-intermediate).
Copy link

Choose a reason for hiding this comment

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

If a practitioner doesn't understand Vault paths, set-signed will feel opaque and hard to understand. Can you clarify:

  • Is this path created automatically?
  • What exactly does set-signed do?

I see you've linked to the docs, but a quick callout could be useful here.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. Write the bundle.pem to the `RootPKIPath` with
[`set-signed`](https://www.vaultproject.io/api-docs/secret/pki#set-signed-intermediate).
1. Write the `bundle.pem` to the `RootPKIPath` with
[`set-signed`](https://www.vaultproject.io/api-docs/secret/pki#set-signed-intermediate).
```shell-session
vault write connect_root/intermediate/set-signed certificate=@bundle.pem

@kisunji
Copy link
Contributor

kisunji commented Mar 10, 2022

Just FYI @webdog & @trujillo-adam, @dnephin has recently left Hashicorp so this PR may need some additional love to bring across the finish line.

I can get to it in a week or so but lack the background knowledge for this feature set so this might need some team involvement

Co-authored-by: Christian Weber <webdog@users.noreply.github.com>
Co-authored-by: Christian Weber <webdog@users.noreply.github.com>
@trujillo-adam
Copy link
Contributor

I applied as much of the feedback and answered as many questions as I could. Most of @webdog 's question will need to be addressed by @kisunji when he has a chance next week. The remaining questions have to do with Nathan's original additions, which, unfortunately, we can't ask him to clarify.

I updated the table column headers in a way that I hope addresses @webdog 's concerns.

On hold for now.

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link

github-actions bot commented Jul 1, 2022

This pull request has been automatically flagged for inactivity because it has not been acted upon in the last 60 days. It will be closed if no new activity occurs in the next 30 days. Please feel free to re-open to resurrect the change if you feel this has happened by mistake. Thank you for your contributions.

@github-actions github-actions bot added the meta/stale Automatically flagged for inactivity by stalebot label Jul 1, 2022
@github-actions
Copy link

Closing due to inactivity. If you feel this was a mistake or you wish to re-open at any time in the future, please leave a comment and it will be re-surfaced for the maintainers to review.

@github-actions github-actions bot closed this Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta/stale Automatically flagged for inactivity by stalebot pr/no-changelog PR does not need a corresponding .changelog entry type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants