Configurable mtls #297

ImpostorKeanu · 2022-01-29T19:26:21Z

read the [CONTRIBUTING guidelines](README.md#user-content-contributing
raised a GitHub issue or discussed it on the projects chat beforehand
added unit tests
[] added integration tests
updated documentation if needed
[] updated CHANGELOG.md

The Problem

Currently, mTLS is set to require any certificate from clients when TLS is enabled. Verification of the certificate is not performed by Headscale, and as far as I can tell, there isn't a way to configure the Tailscale client to accept TLS certificate for authentication.

Implemented Solution

This merge makes mTLS configurable by adding a configuration parameter to config.yaml. It's disabled via the disabled value by default and can be configured to require any certificate (relaxed) or require and verify (enforced) a client certificate.

Related Issues

client need certificate? #254 calls this out as a question but not really a bug.

kradalby

Could you add a test to verify that the values work as intended and a failure case to verify failure?

And add the entry to config-example.yaml with some docs (and linking to the tls doc)

kradalby · 2022-01-29T20:10:35Z

cmd/headscale/cli/utils.go

@@ -40,6 +40,7 @@ func LoadConfig(path string) error {

 	viper.SetDefault("tls_letsencrypt_cache_dir", "/var/www/.cache")
 	viper.SetDefault("tls_letsencrypt_challenge_type", "HTTP-01")
+	viper.SetDefault("tls_client_auth_mode", "disabled")


This default is different than the current setting, how come?

Please correct me if I'm wrong, but I think Tailscale doesn't support client certificates right now.

The current default, RequireAnyClientCert, prevents connections over TLS unless any cert is provided by the client.

If I'm not overlooking something, then this means that Tailscale can't interact with Headscale when it's configured to use TLS.

If I'm not overlooking something, then this means that Tailscale can't interact with Headscale when it's configured to use TLS.

Hmm, I am not sure what this means? I am running my headscale with TLS?

How curious! All of my browsers throw a bad ssl error unless a client certificate is supplied, then it works as expected.

I'm using the config parameters to point at cert values for letsencrypt files I already have on hand.

I believe issue #254 reflects the same outcome.

Do we have a misconfiguration somewhere or something?

Hmm, happy to make it configurable so users with this problem can at least test them, however, there are many successful users (me included) and I think think there is a reason to change the default for everyone.

My current setup has tls set in nginx, but a week ago, I used the letsencrypt built in to headscale.

So yep, let's get it in, but, we should not change the default.

Agreed! Sorry for the confusion there.

Should have things buttoned up tonight/early tomorrow.

Alrighty, the latest commit preserves the original default.

app.go

kradalby · 2022-01-29T20:13:29Z

docs/tls.md

+the following values to the `tls_client_auth_mode` setting in the configuration
+file.
+
+| Value | Behavior |


This file needs to be ran through prettier

It has now been ran through prettier. My bad on the oversight.

kradalby · 2022-01-29T20:15:42Z

app.go

@@ -644,12 +645,34 @@ func (h *Headscale) getTLSSettings() (*tls.Config, error) {
 		if !strings.HasPrefix(h.cfg.ServerURL, "https://") {
 			log.Warn().Msg("Listening with TLS but ServerURL does not start with https://")
 		}
+
+		var clientAuthMode tls.ClientAuthType
+		if h.cfg.TLSClientAuthMode == "disabled" {


This should be a switch statement, and the alternatives should be Constants.

Good call. I'm new to Go, so thank you for this critique. Much less typing in a switch statement.

In the latest commit, I moved this logic to a new function to make testing a bit simpler and implemented a switch statement.

…to configurable-mtls

kradalby

Looking like its get there, I think this might be a way to simplify the flow a bit and remove some redundant checks.

kradalby · 2022-02-02T17:40:21Z

app.go

-	TLSKeyPath  string
+	TLSCertPath       string
+	TLSKeyPath        string
+	TLSClientAuthMode string


I would argue that you can just use the tls.ClientAuthType type here.

kradalby · 2022-02-02T17:41:03Z

app.go

+		clientAuthMode, err := h.GetClientAuthMode()
+		if err != nil {
+			return nil, err
+		}


Then remove this ( or cut it for later )

kradalby · 2022-02-02T17:41:31Z

app.go

 		tlsConfig := &tls.Config{
-			ClientAuth:   tls.RequireAnyClientCert,
+			ClientAuth:   clientAuthMode,


Then do this:

Suggested change

ClientAuth: clientAuthMode,

ClientAuth: h.cfg.TLSClientAuthMode,

kradalby · 2022-02-02T17:43:21Z

cmd/headscale/cli/utils.go

+	clientAuthMode := viper.GetString("tls_client_auth_mode")
+	if clientAuthMode != "disabled" && clientAuthMode != "relaxed" && clientAuthMode != "enforced" {
+		errorText += "Invalid tls_client_auth_mode supplied. Accepted values: disabled, relaxed, enforced."
+	}


Then instead here call the h.GetClientAuthMode function and check the error instead of having that if.

kradalby · 2022-02-02T17:44:19Z

cmd/headscale/cli/utils.go

-		TLSKeyPath:  absPath(viper.GetString("tls_key_path")),
+		TLSCertPath:       absPath(viper.GetString("tls_cert_path")),
+		TLSKeyPath:        absPath(viper.GetString("tls_key_path")),
+		TLSClientAuthMode: viper.GetString("tls_client_auth_mode"),


Then just set it here if there was no error in 4.:

Suggested change

TLSClientAuthMode: viper.GetString("tls_client_auth_mode"),

TLSClientAuthMode: clientAuthMode,

kradalby · 2022-02-02T17:45:04Z

config-example.yaml

+# - disabled: client authentication disabled
+# - relaxed: client certificate is required but not verified
+# - enforced: client certificate is required and verified
+tls_client_auth_mode: disabled


This should be relaxed to reflect the default.

Suggested change

tls_client_auth_mode: disabled

tls_client_auth_mode: relaxed

stefan-caraiman · 2022-02-13T15:57:58Z

docs/tls.md

+### Configuring Mutual TLS Authentication (mTLS)
+
+mTLS is a method by which an HTTPS server authenticates clients, e.g. Tailscale,
+using TLS certificates. The capability can be configured by by applying one of


Duplicate wording? by by applying one of

kradalby · 2022-02-20T14:36:59Z

@Arch4ngel Looks good, last few things is just make the linters happy, and add a line to the changelog under TBD.

Good job :)

ImpostorKeanu · 2022-02-21T15:21:12Z

@Arch4ngel Looks good, last few things is just make the linters happy, and add a line to the changelog under TBD.

Good job :)

Thanks for your patience on this! Work consumed me for a few weeks.

kradalby · 2022-02-21T16:54:17Z

Need another run of prettier and then I'll approve, good work, sorry for all the nits ;)

ImpostorKeanu added 3 commits January 29, 2022 12:59

Making client authentication mode configurable

9e619fc

refining

5935b13

linting/formatting

c98a559

kradalby requested changes Jan 29, 2022

View reviewed changes

kradalby and others added 10 commits January 29, 2022 20:15

Merge branch 'main' into configurable-mtls

0609c97

adding default for tls_client_auth_mode

d44b2a7

making alternatives constants

310e7b1

refining and adding tests

0c3fd16

Merge branch 'configurable-mtls' of github.com:arch4ngel/headscale in…

52db80a

…to configurable-mtls

updating default

9de5c7f

Merge branch 'main' into configurable-mtls

168b1bd

linting again

da5250e

Merge branch 'configurable-mtls' of github.com:arch4ngel/headscale in…

af25aa7

…to configurable-mtls

Merge branch 'main' into configurable-mtls

1b2fff4

kradalby requested changes Feb 2, 2022

View reviewed changes

stefan-caraiman reviewed Feb 13, 2022

View reviewed changes

ImpostorKeanu requested a review from juanfont as a code owner February 20, 2022 14:05

ImpostorKeanu and others added 2 commits February 20, 2022 09:06

refactoring

385dd9c

Merge branch 'main' into configurable-mtls

7bf2a91

ImpostorKeanu added 2 commits February 21, 2022 10:09

Linting and updating tests

daa75da

updating changelog and docs

b5a59d4

kradalby added 4 commits February 21, 2022 21:44

Merge branch 'main' into configurable-mtls

f562ad5

Merge branch 'main' into configurable-mtls

823cc49

Merge branch 'main' into configurable-mtls

afb67b6

Sort lint

8c33907

kradalby approved these changes Feb 24, 2022

View reviewed changes

kradalby merged commit 5596a0a into juanfont:main Feb 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable mtls #297

Configurable mtls #297

ImpostorKeanu commented Jan 29, 2022 •

edited

Loading

kradalby left a comment

kradalby Jan 29, 2022

ImpostorKeanu Jan 30, 2022

kradalby Jan 30, 2022

ImpostorKeanu Jan 30, 2022

kradalby Jan 30, 2022

ImpostorKeanu Jan 30, 2022

ImpostorKeanu Feb 2, 2022

kradalby Jan 29, 2022

ImpostorKeanu Feb 2, 2022

kradalby Jan 29, 2022

ImpostorKeanu Jan 30, 2022

ImpostorKeanu Feb 2, 2022

kradalby left a comment

kradalby Feb 2, 2022

kradalby Feb 2, 2022

kradalby Feb 2, 2022

kradalby Feb 2, 2022

kradalby Feb 2, 2022

kradalby Feb 2, 2022

stefan-caraiman Feb 13, 2022

kradalby commented Feb 20, 2022

ImpostorKeanu commented Feb 21, 2022

kradalby commented Feb 21, 2022

	ClientAuth: clientAuthMode,
	ClientAuth: h.cfg.TLSClientAuthMode,

	TLSClientAuthMode: viper.GetString("tls_client_auth_mode"),
	TLSClientAuthMode: clientAuthMode,

Configurable mtls #297

Configurable mtls #297

Conversation

ImpostorKeanu commented Jan 29, 2022 • edited Loading

The Problem

Implemented Solution

Related Issues

kradalby 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

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

kradalby 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

kradalby commented Feb 20, 2022

ImpostorKeanu commented Feb 21, 2022

kradalby commented Feb 21, 2022

ImpostorKeanu commented Jan 29, 2022 •

edited

Loading