Client TLS: Add option to require a specific DNS name in Subject Alternate Name #126

chodges15 · 2023-01-10T18:37:36Z

This is a follow up to the work done in #97 which addresses the requested change by @roidelapluie and @SuperQ to go from CN to SAN verification. Closes #96.

Command used to update the certs can be found in the commit message.

chodges15 · 2023-01-19T15:07:38Z

@roidelapluie and @SuperQ this is ready for review when you get a chance, I'd like to get this into Loki for tighter security

chodges15 · 2023-01-31T18:38:57Z

@SuperQ - can you kick off CI for this PR again? Linter error that was causing previous error should be resolved.

web/tls_config.go

chodges15 · 2023-02-10T15:42:37Z

@SuperQ - can I get another round of review for this?

SuperQ · 2023-02-11T11:28:22Z

Looks good, can you update the README docs?

chodges15 · 2023-03-01T04:29:32Z

@SuperQ - docs updated, ready for another review

SuperQ

Minor nit, otherwise LGTM.

VERSION

CHANGELOG.md

SuperQ

LGTM

roidelapluie · 2023-03-08T08:01:51Z

please remove the changelog

chodges15 · 2023-03-08T20:43:28Z

@roidelapluie - changelog removed.

roidelapluie · 2023-03-09T22:36:39Z

I think we should force the regex to be anchored, as it is common in Prometheus.

chodges15 · 2023-03-23T15:35:59Z

web/tls_config.go

+
+// NewRegexp creates a new anchored Regexp and returns an error if the
+// passed-in regular expression does not compile.
+func NewRegexp(s string) (Regexp, error) {


This struct borrowed from Prometheus
If needed, this can be refactored by moving into github.com/prometheus/common, though it would be nice if that could be done in a follow up PR.

roidelapluie · 2023-03-23T22:35:49Z

docs/web-configuration.md

+  # Verify that the client certificate has a Subject Alternate Name (SAN) which includes the following
+  # regex pattern, else terminate connection. SAN match can be one or multiple of the following:
+  # DNS, IP, e-mail, or URI address from https://pkg.go.dev/crypto/x509#Certificate.
+  [ client_allowed_san_regex: <string> | default ""]


This is documented twice

Signed-off-by: saroshali-dbx <[email protected]> Signed-off-by: chodges15 <[email protected]>

@WGH-

The changed certs were updated based on the command @WGH- in prometheus#61, just with an added SAN DNS. They were generated with this command: openssl req -x509 -newkey ec:<(openssl ecparam -name secp384r1) -keyout client2_selfsigned.key -out client2_selfsigned.pem -nodes -subj '/CN=test3' -days 36500 -addext "subjectAltName = DNS:test3" -addext "extendedKeyUsage = clientAuth" Signed-off-by: chodges15 <[email protected]>

Signed-off-by: chodges15 <[email protected]>

chodges15 · 2023-03-31T13:58:18Z

@roidelapluie - documentation fixed, not sure how I missed that.

roidelapluie · 2023-04-04T21:22:51Z

web/tls_config.go

@@ -163,6 +234,11 @@ func ConfigToTLSConfig(c *TLSConfig) (*tls.Config, error) {
 		cfg.ClientCAs = clientCAPool
 	}

+	if c.ClientAllowedSanRegex.Regexp != nil {
+		// verify that the client cert contains the allowed domain name
+		cfg.VerifyPeerCertificate = c.VerifyPeerCertificate


Why do we need that if?

I guess I'm not fully tracking the question, without the if the behavior would be to always require the client to verify SAN; unless you are thinking we could default the regex to be fully permissive with something like .*?

web/tls_config.go

roidelapluie · 2023-04-04T21:27:09Z

Shouldn't we use go upstream tls.VerifyOptions ?

roidelapluie

Sorry, but I think we should use upstream go VerifyOptions

web/tls_config.go

chodges15 · 2023-04-05T01:32:07Z

The upstream x509.VerifyHostname does not check all SAN fields, and for fields that it does check it does not support regex, only a wildcard. That being said, if we only care about DNS and IP addresses here (which would handle my use case) I'm fine going with the standard upstream verification.

roidelapluie · 2023-04-05T20:18:14Z

Okay. You are correct about SAN's. I think we can move ahead, maybe we should explicltly call out in the documentation that dots must be escaped due to the nature of regexes, but I am also happy about removing support for regexes and working with an explicit list of sans instead.

Signed-off-by: chodges15 <[email protected]>

chodges15 · 2023-04-06T02:32:12Z

An explicit list feels better here from a simplicity point of view - and more secure to avoid user regex mistakes.

SuperQ · 2023-04-06T08:11:25Z

Sorry about the regex distraction. It seemed like a good idea at the time. :)

roidelapluie · 2023-04-06T11:39:33Z

One CI lint issue.

Signed-off-by: chodges15 <[email protected]>

roidelapluie · 2023-04-07T13:01:40Z

thanks!

SuperQ · 2023-04-07T13:03:17Z

Nice!

chodges15 · 2023-04-07T14:01:12Z

Thanks for the good feedback @roidelapluie and @SuperQ.

…rnate Name (prometheus#126) Signed-off-by: saroshali-dbx <[email protected]> Signed-off-by: chodges15 <[email protected]> Co-authored-by: saroshali-dbx <[email protected]> Co-authored-by: Chris Hodges <[email protected]> Co-authored-by: chodges15 <[email protected]>

* [FEATURE] Client TLS: Add option to require a specific Subject Alternate Names #126 * [FEATURE] Add a POST form to the landing page #144 * [FEATURE] Add generic customization to landing page #146 * [ENHANCEMENT] Add a Content-Type header to the landing page #142 * [BUGFIX] Fix Nil pointer references for WebSystemdSocket #127 Signed-off-by: SuperQ <[email protected]>

chodges15 force-pushed the verify-cert-san branch from 6149788 to 08e6333 Compare January 30, 2023 15:05

SuperQ reviewed Jan 31, 2023

View reviewed changes

web/tls_config.go Outdated Show resolved Hide resolved

SuperQ reviewed Feb 1, 2023

View reviewed changes

web/tls_config.go Outdated Show resolved Hide resolved

chodges15 force-pushed the verify-cert-san branch from eba3081 to f737664 Compare February 1, 2023 14:38

chodges15 force-pushed the verify-cert-san branch 2 times, most recently from 2e40f19 to f58eb40 Compare March 1, 2023 04:26

SuperQ requested changes Mar 1, 2023

View reviewed changes

VERSION Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

chodges15 requested a review from SuperQ March 1, 2023 04:36

SuperQ approved these changes Mar 2, 2023

View reviewed changes

SuperQ requested a review from roidelapluie March 2, 2023 10:51

chodges15 commented Mar 23, 2023

View reviewed changes

roidelapluie reviewed Mar 23, 2023

View reviewed changes

saroshali-dbx and others added 10 commits March 27, 2023 11:05

add common-name verification

b3ef535

Signed-off-by: saroshali-dbx <[email protected]> Signed-off-by: chodges15 <[email protected]>

fix tests

7c8fecd

Signed-off-by: saroshali-dbx <[email protected]> Signed-off-by: chodges15 <[email protected]>

missing import

a9384c4

Signed-off-by: saroshali-dbx <[email protected]> Signed-off-by: chodges15 <[email protected]>

Fix tests

1515dca

Signed-off-by: saroshali-dbx <[email protected]> Signed-off-by: chodges15 <[email protected]>

fix linter error

f8a6b39

Signed-off-by: chodges15 <[email protected]>

change config to regex

5b49cfe

Signed-off-by: chodges15 <[email protected]>

check all SAN types against regex

fd4ac69

Signed-off-by: chodges15 <[email protected]>

Add documentation

0ed1d0e

Signed-off-by: chodges15 <[email protected]>

version the right way

7b089c7

Signed-off-by: chodges15 <[email protected]>

chodges15 added 5 commits March 27, 2023 11:05

Add documentation

5f08414

Signed-off-by: chodges15 <[email protected]>

remove changelog

208cb53

Signed-off-by: chodges15 <[email protected]>

leave VERSION alone as well

3d91e48

Signed-off-by: chodges15 <[email protected]>

anchor configured regex

b0e6f85

Signed-off-by: chodges15 <[email protected]>

update docs and remove duplicate entry

b205f7a

Signed-off-by: chodges15 <[email protected]>

chodges15 force-pushed the verify-cert-san branch from 2985402 to b205f7a Compare March 27, 2023 16:05

roidelapluie reviewed Apr 4, 2023

View reviewed changes

web/tls_config.go Show resolved Hide resolved

roidelapluie reviewed Apr 4, 2023

View reviewed changes

web/tls_config.go Show resolved Hide resolved

roidelapluie reviewed Apr 4, 2023

View reviewed changes

web/tls_config.go Show resolved Hide resolved

regex -> list for SANs

394edbb

Signed-off-by: chodges15 <[email protected]>

fix linter error

06d5a6a

Signed-off-by: chodges15 <[email protected]>

roidelapluie approved these changes Apr 7, 2023

View reviewed changes

roidelapluie merged commit 47811e7 into prometheus:master Apr 7, 2023

chodges15 deleted the verify-cert-san branch April 7, 2023 14:01

SuperQ mentioned this pull request May 1, 2023

Release 0.10.0 #150

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client TLS: Add option to require a specific DNS name in Subject Alternate Name #126

Client TLS: Add option to require a specific DNS name in Subject Alternate Name #126

chodges15 commented Jan 10, 2023

chodges15 commented Jan 19, 2023

chodges15 commented Jan 31, 2023

chodges15 commented Feb 10, 2023

SuperQ commented Feb 11, 2023

chodges15 commented Mar 1, 2023

SuperQ left a comment

SuperQ left a comment

roidelapluie commented Mar 8, 2023

chodges15 commented Mar 8, 2023

roidelapluie commented Mar 9, 2023

chodges15 Mar 23, 2023

roidelapluie Mar 23, 2023

chodges15 commented Mar 31, 2023

roidelapluie Apr 4, 2023

chodges15 Apr 5, 2023

roidelapluie commented Apr 4, 2023

roidelapluie left a comment

chodges15 commented Apr 5, 2023

roidelapluie commented Apr 5, 2023

chodges15 commented Apr 6, 2023

SuperQ commented Apr 6, 2023

roidelapluie commented Apr 6, 2023

roidelapluie commented Apr 7, 2023

SuperQ commented Apr 7, 2023

chodges15 commented Apr 7, 2023

Client TLS: Add option to require a specific DNS name in Subject Alternate Name #126

Client TLS: Add option to require a specific DNS name in Subject Alternate Name #126

Conversation

chodges15 commented Jan 10, 2023

chodges15 commented Jan 19, 2023

chodges15 commented Jan 31, 2023

chodges15 commented Feb 10, 2023

SuperQ commented Feb 11, 2023

chodges15 commented Mar 1, 2023

SuperQ left a comment

Choose a reason for hiding this comment

SuperQ left a comment

Choose a reason for hiding this comment

roidelapluie commented Mar 8, 2023

chodges15 commented Mar 8, 2023

roidelapluie commented Mar 9, 2023

chodges15 Mar 23, 2023

Choose a reason for hiding this comment

roidelapluie Mar 23, 2023

Choose a reason for hiding this comment

chodges15 commented Mar 31, 2023

roidelapluie Apr 4, 2023

Choose a reason for hiding this comment

chodges15 Apr 5, 2023

Choose a reason for hiding this comment

roidelapluie commented Apr 4, 2023

roidelapluie left a comment

Choose a reason for hiding this comment

chodges15 commented Apr 5, 2023

roidelapluie commented Apr 5, 2023

chodges15 commented Apr 6, 2023

SuperQ commented Apr 6, 2023

roidelapluie commented Apr 6, 2023

roidelapluie commented Apr 7, 2023

SuperQ commented Apr 7, 2023

chodges15 commented Apr 7, 2023