add root_cert_ttl option for consul connect, vault ca providers #11428

acpana · 2021-10-26T19:27:51Z

Overview

This change adds an option to the CommonCA config to allow a root_cert_ttl value which will control the TTL (time to live) for root certs issued by supported CA providers. It's mostly plumbing.

PR Notes/ Callouts

At present the only providers supported are consul connect and vault.
Let's focus on the functionality (going QSF -- quality, security, feature), before delving into all the refactoring opportunities
No corresponding "cli flag" option added as it's not a pattern for our current ca config

Config checklist

I used the config file checklist here. Click below to expand on a "filled in" version of it.

Click to expand!

Legend

n/a -- not applicable
? -- I believe I may have fully completed it

Adding a Simple Config Field for Client Agents

Adding a Simple Config Field for Servers

[?] Do all of the steps in Adding a Simple Config
Field For Client Agents.
Add the new field to Config struct in agent/consul/config.go
[?] Add code to set the values from the RuntimeConfig in the confusingly
named consulConfig method in agent/agent.go
[n/a] If needed, add a test to agent_test.go if there is some non-trivial
behavior in the code you added in the previous step. We tend not to test
simple assignments from one to the other since these are typically caught by
higher-level tests of the actual functionality that matters but some examples
can be found prefixed with TestAgent_consulConfig*
[n/a] If your config should take effect on a reload/HUP
- [n/a] Add necessary code to ReloadConfig in agent/consul/server.go this
  needs to be adequately synchronized with any readers of the state being
  updated.
  - [n/a] Add a new test or a new assertion to TestServer_ReloadConfig

Issues related

connect/ca: Vault Provider: Document and make the root CA TTL configuration #10762

TODO:

one off testing with https://github.com/dhiaayachi/consul-local-cluster

Steps to expand!

make linux
Use the binary with https://github.com/dhiaayachi/consul-local-cluster
Add a RootCertTTL value to the vault-provider.json.template file locally to something like:

{
  "Provider": "vault",
  "Config": {
      "LeafCertTTL": "72h",
      "Address": "http://myvault:8200",
      "Token": {{VAULT_TOKEN}},
      "RootPKIPath": "connect-root",
      "IntermediatePKIPath": "connect-intermediate",
      "RootCertTTL": "8761h"
  },
  "ForceWithoutCrossSigning": false
}

Assert root cert properties are as expected -- consul connect ca get-config -token=<TOKEN>

Signed-off-by: FFMMM FFMMM@users.noreply.github.com

hashicorp-ci · 2021-10-26T19:28:29Z

🤔 This PR has changes in the website/ directory but does not have a type/docs-cherrypick label. If the changes are for the next version, this can be ignored. If they are updates to current docs, attach the label to auto cherrypick to the stable-website branch after merging.

kisunji

Left a bunch of nit-picky comments to consider!

agent/config/testdata/full-config.json

agent/connect/ca/provider_consul_test.go

agent/structs/connect_ca.go

kisunji · 2021-10-27T23:01:39Z

api/connect_ca.go

@@ -38,6 +38,7 @@ type CAConfig struct {
 // CommonCAProviderConfig is the common options available to all CA providers.
 type CommonCAProviderConfig struct {
 	LeafCertTTL      time.Duration
+	RootCertTTL      time.Duration


For my own understanding, why is this under Common and not Consul (which has a field called RootCert?)

Good questions!

IIUC, the ConsulCAProvider is one of the 3 providers we support. RootCertTTL is a common option to all providers. So I thought we should add it in the CommonCAProviderConfig once, rather than 3 times for all providers.

sdk/testutil/server.go

agent/connect/ca/provider_vault_test.go

Signed-off-by: FFMMM <FFMMM@users.noreply.github.com>

Co-authored-by: Chris S. Kim <ckim@hashicorp.com>

kyhavlov

Looking good, left a couple comments but nothing blocking - once the doc change is done this should be good to merge 👍

kyhavlov · 2021-11-01T17:36:39Z

agent/structs/connect_ca.go

@@ -380,6 +382,17 @@ func (c CommonCAProviderConfig) Validate() error {
 		return nil
 	}

+	// if the root cert ttl is not set, set it to the default
+	if c.RootCertTTL == 0 {
+		c.RootCertTTL = 10 * 365 * 24 * time.Hour


I think we can do c.RootCertTTL = time.ParseDuration(DefaultRootCertTTL) here and use the default that's already defined above

This function is Validate, but here we are normalizing (not validating).

I suspect this should this return an error instead? In normal operations we expect RuntimeConfig to always have set a default, so users should only encounter this problem if they explicit set a 0 for TTL (I think).

but here we are normalizing
In normal operations we expect RuntimeConfig to always have set a default,

Hey both, thanks for the engagement here -- I think per Daniel's comment, I'm inclined to take out this chunk of code.

If a callee actually sets the value to less than the intermediate cert ttl than the right error should come out.

I also see no such "normalization"/ defaulting for leaf and intermediate ttls

kyhavlov · 2021-11-01T17:38:43Z

website/content/docs/agent/options.mdx

@@ -1267,6 +1267,11 @@ bind_addr = "{{ GetPrivateInterfaces | include \"network\" \"10.0.0.0/8\" | attr
      for more than twice the _current_ `leaf_cert_ttl`, it will be removed
      from the trusted list.

+    - `root_cert_ttl` ((#todo)) todo. Defaults to 10 years as `87600h`.
+      todo restrictions.


Still needs a description (leftover todo)

By restrictions I guess you mean "is only used when the CA system is first initialized, or when a configuration change causes a rotation or the root certificate" ? I think we should say something about that.

I was punting on the docs a bit to make sure that the stuff here was something we eventually wanted to check in.

Which looks like it is case. So I added the docs now :)

@FFMMM Can you also make sure this field gets documented under https://github.com/hashicorp/consul/blob/main/website/content/partials/http_api_connect_ca_common_options.mdx?

The options in this file are displayed under the provider-specific CA config options. For example, https://www.consul.io/docs/connect/ca/vault#common-ca-config-options.

dnephin

Nice work! I see you found all the places where we plumb this config.

Mostly just small suggestions about testing values and docs.

.changelog/11428.txt

agent/config/runtime_test.go

agent/config/testdata/full-config.hcl

dnephin · 2021-11-01T18:23:22Z

agent/connect/ca/provider_vault.go

+				// the max lease ttl denotes the maximum ttl that secrets are created from the engine
+				// the default lease ttl is the kind of ttl that will *reliably* set the ttl to v.config.RootCertTTL
+				// https://www.vaultproject.io/docs/secrets/pki#configure-a-ca-certificate
+				MaxLeaseTTL:     v.config.RootCertTTL.String(),
+				DefaultLeaseTTL: v.config.RootCertTTL.String(),


dnephin · 2021-11-01T18:26:35Z

agent/structs/connect_ca.go

@@ -380,6 +382,17 @@ func (c CommonCAProviderConfig) Validate() error {
 		return nil
 	}

+	// if the root cert ttl is not set, set it to the default
+	if c.RootCertTTL == 0 {
+		c.RootCertTTL = 10 * 365 * 24 * time.Hour


This function is Validate, but here we are normalizing (not validating).

I suspect this should this return an error instead? In normal operations we expect RuntimeConfig to always have set a default, so users should only encounter this problem if they explicit set a 0 for TTL (I think).

dnephin · 2021-11-01T18:29:28Z

website/content/docs/agent/options.mdx

@@ -1267,6 +1267,11 @@ bind_addr = "{{ GetPrivateInterfaces | include \"network\" \"10.0.0.0/8\" | attr
      for more than twice the _current_ `leaf_cert_ttl`, it will be removed
      from the trusted list.

+    - `root_cert_ttl` ((#todo)) todo. Defaults to 10 years as `87600h`.
+      todo restrictions.


By restrictions I guess you mean "is only used when the CA system is first initialized, or when a configuration change causes a rotation or the root certificate" ? I think we should say something about that.

Signed-off-by: FFMMM <FFMMM@users.noreply.github.com>

Co-authored-by: Daniel Nephin <dnephin@hashicorp.com>

kyhavlov

Looks good to merge, just had one tiny suggestion 👍

website/content/docs/agent/options.mdx

Co-authored-by: Kyle Havlovitz <kylehav@gmail.com>

hc-github-team-consul-core · 2021-11-02T18:02:58Z

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/491685.

acpana requested a review from a team as a code owner October 26, 2021 19:27

acpana force-pushed the ffmmm/f-10762 branch from 4c5747a to 262fc01 Compare October 26, 2021 20:06

vercel bot temporarily deployed to Preview – consul-ui-staging October 26, 2021 20:06 Inactive

vercel bot deployed to Preview – consul October 26, 2021 20:06 View deployment

acpana force-pushed the ffmmm/f-10762 branch from 262fc01 to ab0a211 Compare October 26, 2021 20:09

vercel bot deployed to Preview – consul October 26, 2021 20:09 View deployment

vercel bot temporarily deployed to Preview – consul-ui-staging October 26, 2021 20:09 Inactive

acpana force-pushed the ffmmm/f-10762 branch from ab0a211 to 57d5f39 Compare October 26, 2021 20:40

vercel bot deployed to Preview – consul October 26, 2021 20:40 View deployment

vercel bot temporarily deployed to Preview – consul-ui-staging October 26, 2021 20:40 Inactive

acpana force-pushed the ffmmm/f-10762 branch from 57d5f39 to 1671bc0 Compare October 26, 2021 20:51

vercel bot temporarily deployed to Preview – consul-ui-staging October 26, 2021 20:51 Inactive

vercel bot deployed to Preview – consul October 26, 2021 20:51 View deployment

kisunji reviewed Oct 27, 2021

View reviewed changes

vercel bot temporarily deployed to Preview – consul-ui-staging October 28, 2021 19:10 Inactive

vercel bot temporarily deployed to Preview – consul October 28, 2021 19:10 Inactive

acpana force-pushed the ffmmm/f-10762 branch from f3564f5 to 5861ad9 Compare October 28, 2021 19:22

vercel bot temporarily deployed to Preview – consul October 28, 2021 19:22 Inactive

vercel bot temporarily deployed to Preview – consul-ui-staging October 28, 2021 19:22 Inactive

vercel bot temporarily deployed to Preview – consul October 28, 2021 20:23 Inactive

vercel bot temporarily deployed to Preview – consul-ui-staging October 28, 2021 20:23 Inactive

kisunji reviewed Oct 28, 2021

View reviewed changes

agent/connect/ca/provider_vault_test.go Outdated Show resolved Hide resolved

acpana mentioned this pull request Oct 29, 2021

plumb thru root cert tll to the aws ca provider #11449

Merged

4 tasks

acpana force-pushed the ffmmm/f-10762 branch from a3e203b to 42bf1ce Compare October 29, 2021 00:21

vercel bot temporarily deployed to Preview – consul October 29, 2021 00:21 Inactive

vercel bot temporarily deployed to Preview – consul-ui-staging October 29, 2021 00:21 Inactive

FFMMM and others added 2 commits November 1, 2021 09:50

add root_cert_ttl option for consul connect, vault ca providers

fc94bde

Signed-off-by: FFMMM <FFMMM@users.noreply.github.com>

Apply suggestions from code review

430c298

Co-authored-by: Chris S. Kim <ckim@hashicorp.com>

acpana force-pushed the ffmmm/f-10762 branch from 42bf1ce to 037564f Compare November 1, 2021 16:52

vercel bot temporarily deployed to Preview – consul-ui-staging November 1, 2021 16:53 Inactive

vercel bot temporarily deployed to Preview – consul November 1, 2021 16:53 Inactive

kyhavlov reviewed Nov 1, 2021

View reviewed changes

dnephin reviewed Nov 1, 2021

View reviewed changes

acpana force-pushed the ffmmm/f-10762 branch from 037564f to bd12b92 Compare November 1, 2021 19:28

vercel bot temporarily deployed to Preview – consul-ui-staging November 1, 2021 19:28 Inactive

vercel bot deployed to Preview – consul November 1, 2021 19:28 View deployment

add changelog, pr feedback

9725132

Signed-off-by: FFMMM <FFMMM@users.noreply.github.com>

acpana force-pushed the ffmmm/f-10762 branch from bd12b92 to 9725132 Compare November 1, 2021 19:49

vercel bot temporarily deployed to Preview – consul-ui-staging November 1, 2021 19:49 Inactive

vercel bot deployed to Preview – consul November 1, 2021 19:49 View deployment

vercel bot temporarily deployed to Preview – consul-ui-staging November 1, 2021 20:35 Inactive

vercel bot temporarily deployed to Preview – consul November 1, 2021 20:35 Inactive

Update .changelog/11428.txt, more docs

aa0ffc0

Co-authored-by: Daniel Nephin <dnephin@hashicorp.com>

acpana force-pushed the ffmmm/f-10762 branch from aa54c88 to aa0ffc0 Compare November 1, 2021 20:42

vercel bot deployed to Preview – consul November 1, 2021 20:42 View deployment

vercel bot temporarily deployed to Preview – consul-ui-staging November 1, 2021 20:42 Inactive

kyhavlov approved these changes Nov 2, 2021

View reviewed changes

website/content/docs/agent/options.mdx Outdated Show resolved Hide resolved

Update website/content/docs/agent/options.mdx

a5548f2

Co-authored-by: Kyle Havlovitz <kylehav@gmail.com>

vercel bot deployed to Preview – consul November 2, 2021 17:29 View deployment

vercel bot temporarily deployed to Preview – consul-ui-staging November 2, 2021 17:29 Inactive

acpana merged commit 4ddf973 into main Nov 2, 2021

acpana deleted the ffmmm/f-10762 branch November 2, 2021 18:02

This was referenced Nov 3, 2021

change vault ca docs to mention root cert ttl config #11488

Merged

connect/ca: Vault Provider: Document and make the root CA TTL configuration #10762

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add root_cert_ttl option for consul connect, vault ca providers #11428

add root_cert_ttl option for consul connect, vault ca providers #11428

acpana commented Oct 26, 2021 •

edited

Loading

hashicorp-ci commented Oct 26, 2021

kisunji left a comment

kisunji Oct 27, 2021

acpana Oct 28, 2021

kyhavlov left a comment

kyhavlov Nov 1, 2021

dnephin Nov 1, 2021

acpana Nov 1, 2021

kyhavlov Nov 1, 2021

dnephin Nov 1, 2021

acpana Nov 1, 2021

blake Nov 1, 2021

dnephin left a comment

dnephin Nov 1, 2021

dnephin Nov 1, 2021

dnephin Nov 1, 2021

kyhavlov left a comment

hc-github-team-consul-core commented Nov 2, 2021

add root_cert_ttl option for consul connect, vault ca providers #11428

add root_cert_ttl option for consul connect, vault ca providers #11428

Conversation

acpana commented Oct 26, 2021 • edited Loading

Overview

PR Notes/ Callouts

Config checklist

Legend

Adding a Simple Config Field for Client Agents

Adding a Simple Config Field for Servers

Issues related

TODO:

hashicorp-ci commented Oct 26, 2021

kisunji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyhavlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyhavlov left a comment

Choose a reason for hiding this comment

hc-github-team-consul-core commented Nov 2, 2021

acpana commented Oct 26, 2021 •

edited

Loading