agent: convert listener config to TLS types #12522

mikemorris · 2022-03-04T16:29:02Z

Replaces #11647 after the listener TLS refactoring work in #12504.

I think I've pushed the use of types versus as close to the surface I can -agent/config/builder.go appears to be a boundary where generic string values capturing user config input that could be invalid can be parsed into types, and pbconfig.TLS appears to be another boundary for autoconfig to avoid breaking backwards compatibility.

Reviewer Notes:

ParseTLSVersion and ParseCiphers both implement some logic specific to agent config (the fallback logic to support tls10 etc values and the comma-delimited cipher string) so I don't think these would belong in types/tls.go
- Per suggestion from @boxofrad, these have been subsumed into agent config builder parsing functions directly.
It appears CipherString is only used for generating pbconfig.TLS autoconfig, should it be renamed for clarity?

TODO:

Update website docs per comment in agent: use TLS types for config #11647 (review)
Update tls_cipher_suites search link in website docs
Add validation to error or warn when configuring cipher suites with TLS min verison 1.3
Close out remaining FIXMEs and TODOs
Add changelog entries

hashicorp-ci · 2022-03-04T16:29:43Z

🤔 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.

mikemorris · 2022-03-04T23:17:50Z

agent/config/runtime_test.go

@@ -6330,8 +6330,8 @@ func TestLoad_FullConfig(t *testing.T) {
 				CAPath:         "nu4PlHzn",
 				CertFile:       "1yrhPlMk",
 				KeyFile:        "1bHapOkL",
-				TLSMinVersion:  "mK14iOpz",
-				CipherSuites:   []uint16{tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256},
+				TLSMinVersion:  types.TLSv1_3,


This should fail, TLS 1.3 cipher suites are not configurable.

Config combinations like this are being handled as expected now (config builder error when specifying cipher suites alongside or with an inherited TLS min version 1.3, cipher suites omitted from config when they would be inherited by a listener config only specifying a TLS min version of 1.3).

tlsutil/config.go

mikemorris · 2022-03-05T00:41:47Z

The remaining test failures actually look good! Invalid config that was previously being passed through is now being rejected, will update test expectations next, but this looks almost ready!

hashicorp-cla · 2022-03-12T16:39:12Z

All committers have signed the CLA.

boxofrad

This is awesome! Thanks so much for taking the time to re-work your changes on top of #12504, @mikemorris 👏🏻 🎉

I've left a few comments for your consideration. Let me know if you'd like to talk any of them through synchronously.

agent/config/builder.go

agent/config/runtime_test.go

agent/consul/auto_config_endpoint.go

tlsutil/config.go

boxofrad · 2022-03-14T11:52:06Z

tlsutil/config.go

+		// FIXME: move cipherSuiteLookup to be called externally, maybe
+		// in agent/config/runtime parsing before the tlsutil.Config struct is
+		// created?


Yep agree, this should be done up-front rather than when we're trying to use the cipher suites.

Looking at this again, this actually feels like an appropriate place to handle the conversion from types.TLSCipherSuite to crypto/tls values for creating the tls.Config object.

tlsutil/config.go

tlsutil: add test for parsing deprecated agent TLS version strings tlsutil: return TLSVersionInvalid with error tlsutil: start moving tlsutil cipher suite lookups over to types/tls tlsutil: rename tlsLookup to ParseTLSVersion, add cipherSuiteLookup agent: attempt to use types in runtime config agent: implement b.tlsVersion validation in config builder agent: fix tlsVersion nil check in builder tlsutil: update to renamed ParseTLSVersion and goTLSVersions tlsutil: fixup TestConfigurator_CommonTLSConfigTLSMinVersion tlsutil: disable invalid config parsing tests tlsutil: update tests auto_config: lookup old config strings from base.TLSMinVersion auto_config: update endpoint tests to use TLS types agent: update runtime_test to use TLS types agent: update TestRuntimeCinfig_Sanitize.golden agent: update config runtime tests to expect TLS types

…ig_test

This reverts commit 3858592.

mikemorris · 2022-03-22T16:51:32Z

@boxofrad I can't quite figure out why these failing TestAutoConfig_Integration tests seem to be skipping the deprecated config parsing. Does applyDeprecatedConfig as called at

consul/agent/config/config.go

Line 81 in a7e5ee0

c, warns := applyDeprecatedConfig(&target)

need to be added somewhere to the auto-config code path?

consul/agent/auto-config/auto_config.go

Line 107 in a7e5ee0

func (ac *AutoConfig) ReadConfig() (*config.RuntimeConfig, error) {

It does appear to be appropriately setting tls12 in pbconfig.TLS for backwards compatibility, but instead of the agent just logging a warning and updating the config to TLS.Defaults.TLSMinVersion: TLSv1_2 internally at

consul/agent/config/deprecated.go

Lines 226 to 230 in f1a2a87

    
           if deprecatedVersion, ok := types.DeprecatedConsulAgentTLSVersions[*v]; ok { 
        
           	// Log warning about deprecated config values 
        
           	warns = append(warns, fmt.Sprintf("The value for 'tls_min_version' field is deprecated, please specify one of %v", types.TLSVersions())) 
        
           	deprecatedVersionString := deprecatedVersion.String() 
        
           	defaults.TLSMinVersion = &deprecatedVersionString

it just hits the validation error and fails to start the agent.

--- FAIL: TestAutoConfig_Integration (1.76s)
    agent_test.go:5065: testagent.go:107: b3a9ac58-755b-c31f-f007-24cd304f7085 test-client Error starting agent: failed to fully resolve configuration: 1 error occurred:
                * tls.defaults.tls_min_version: invalid TLS version: no matching TLS version found for tls12, please specify one of [TLS_AUTO, TLSv1_0, TLSv1_1, TLSv1_2, TLSv1_3]

agent/consul/auto_config_endpoint_test.go

boxofrad

Great job 👏🏻 I've left one last nit, but please feel free to take or leave it.

boxofrad · 2022-03-24T10:24:16Z

agent/config/builder.go

+func validateTLSVersionCipherSuitesCompat(tlsMinVersion types.TLSVersion) error {
+	if tlsMinVersion == types.TLSv1_3 {
+		return fmt.Errorf("TLS 1.3 cipher suites are not configurable")
+	}
+	return nil
+}


Thanks for adding this validation, it's a great UX improvement 🙌🏻

agent/config/deprecated.go

Co-authored-by: Dan Upton <[email protected]>

hc-github-team-consul-core · 2022-03-24T19:33:02Z

🍒 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/613317.

mikemorris requested a review from boxofrad March 4, 2022 16:29

mikemorris changed the base branch from main to boxofrad/per-listener-tls-configuration March 4, 2022 16:29

vercel bot temporarily deployed to Preview – consul-ui-staging March 4, 2022 22:41 Inactive

vercel bot temporarily deployed to Preview – consul March 4, 2022 22:41 Inactive

vercel bot temporarily deployed to Preview – consul-ui-staging March 4, 2022 23:12 Inactive

vercel bot temporarily deployed to Preview – consul March 4, 2022 23:12 Inactive

mikemorris commented Mar 4, 2022

View reviewed changes

tlsutil/config.go Outdated Show resolved Hide resolved

vercel bot temporarily deployed to Preview – consul-ui-staging March 4, 2022 23:26 Inactive

vercel bot temporarily deployed to Preview – consul March 4, 2022 23:26 Inactive

mikemorris commented Mar 4, 2022

View reviewed changes

tlsutil/config.go Show resolved Hide resolved

vercel bot temporarily deployed to Preview – consul-ui-staging March 5, 2022 00:00 Inactive

vercel bot temporarily deployed to Preview – consul March 5, 2022 00:00 Inactive

boxofrad reviewed Mar 14, 2022

View reviewed changes

Base automatically changed from boxofrad/per-listener-tls-configuration to main March 18, 2022 10:47

mikemorris added 6 commits March 18, 2022 15:28

website: update Consul agent tls_min_version values

27c9463

agent: fixup TLS parsing and compilation errors

74f383e

test: fixup lint issues in agent/config_runtime_test and tlsutil/conf…

b80241a

…ig_test

tlsutil: add CHACHA20_POLY1305 cipher suites to goTLSCipherSuites

b9357d1

test: revert autoconfig tls min version fixtures to old format

200957c

vercel bot temporarily deployed to Preview – consul-ui-staging March 21, 2022 21:07 Inactive

website: moar linebreaks to fix tls_min_version formatting

3858592

vercel bot deployed to Preview – consul March 21, 2022 21:18 View deployment

vercel bot temporarily deployed to Preview – consul-ui-staging March 21, 2022 21:18 Inactive

Revert "website: moar linebreaks to fix tls_min_version formatting"

f1a2a87

This reverts commit 3858592.

vercel bot temporarily deployed to Preview – consul-ui-staging March 21, 2022 21:22 Inactive

vercel bot deployed to Preview – consul March 21, 2022 21:22 View deployment

mikemorris marked this pull request as ready for review March 21, 2022 21:25

mikemorris requested a review from a team as a code owner March 21, 2022 21:25

mikemorris requested a review from boxofrad March 21, 2022 21:25

mkeeler reviewed Mar 22, 2022

View reviewed changes

agent/consul/auto_config_endpoint_test.go Show resolved Hide resolved

mikemorris added 2 commits March 23, 2022 15:58

autoconfig: translate old values for TLSMinVersion

f44a8d7

agent: rename var for translated value of deprecated TLS version value

7e9840c

vercel bot temporarily deployed to Preview – consul March 23, 2022 20:00 Inactive

vercel bot temporarily deployed to Preview – consul-ui-staging March 23, 2022 20:00 Inactive

mikemorris added the pr/no-metrics-test label Mar 23, 2022

boxofrad approved these changes Mar 24, 2022

View reviewed changes

Update agent/config/deprecated.go

e6e422a

Co-authored-by: Dan Upton <[email protected]>

vercel bot temporarily deployed to Preview – consul-ui-staging March 24, 2022 11:58 Inactive

vercel bot temporarily deployed to Preview – consul March 24, 2022 11:58 Inactive

agent: fix lint issue

892a38f

vercel bot temporarily deployed to Preview – consul-ui-staging March 24, 2022 15:43 Inactive

vercel bot temporarily deployed to Preview – consul March 24, 2022 15:43 Inactive

agent: fixup deprecated config test assertions for updated warning

5a83a68

vercel bot temporarily deployed to Preview – consul-ui-staging March 24, 2022 15:54 Inactive

vercel bot temporarily deployed to Preview – consul March 24, 2022 15:54 Inactive

mikemorris merged commit f8a2ae2 into main Mar 24, 2022

mikemorris deleted the agent/listener-tls-config-types branch March 24, 2022 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

agent: convert listener config to TLS types #12522

agent: convert listener config to TLS types #12522

mikemorris commented Mar 4, 2022 •

edited

Loading

hashicorp-ci commented Mar 4, 2022

mikemorris Mar 4, 2022

mikemorris Mar 21, 2022

mikemorris commented Mar 5, 2022

hashicorp-cla commented Mar 12, 2022 •

edited

Loading

boxofrad left a comment

boxofrad Mar 14, 2022

mikemorris Mar 18, 2022

mikemorris commented Mar 22, 2022 •

edited

Loading

boxofrad left a comment

boxofrad Mar 24, 2022

hc-github-team-consul-core commented Mar 24, 2022

agent: convert listener config to TLS types #12522

agent: convert listener config to TLS types #12522

Conversation

mikemorris commented Mar 4, 2022 • edited Loading

hashicorp-ci commented Mar 4, 2022

mikemorris Mar 4, 2022

Choose a reason for hiding this comment

mikemorris Mar 21, 2022

Choose a reason for hiding this comment

mikemorris commented Mar 5, 2022

hashicorp-cla commented Mar 12, 2022 • edited Loading

boxofrad left a comment

Choose a reason for hiding this comment

boxofrad Mar 14, 2022

Choose a reason for hiding this comment

mikemorris Mar 18, 2022

Choose a reason for hiding this comment

mikemorris commented Mar 22, 2022 • edited Loading

boxofrad left a comment

Choose a reason for hiding this comment

boxofrad Mar 24, 2022

Choose a reason for hiding this comment

hc-github-team-consul-core commented Mar 24, 2022

mikemorris commented Mar 4, 2022 •

edited

Loading

hashicorp-cla commented Mar 12, 2022 •

edited

Loading

mikemorris commented Mar 22, 2022 •

edited

Loading