From da48cf64b3ae4ea18ed45b3604499d9e9b859f60 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Tue, 31 Jan 2023 12:40:38 +0100 Subject: [PATCH] Set OpenID Connect Expiry This commit adds a default OpenID Connect expiry to 180d to align with Tailscale SaaS (previously infinite or based on token expiry). In addition, it adds an option use the expiry time from the Token sent by the OpenID provider. This will typically cause really short expiry and you should only turn on this option if you know what you are desiring. This fixes #1176. Co-authored-by: Even Holthe Signed-off-by: Kristoffer Dalby --- ...estOIDCExpireNodesBasedOnTokenExpiry.yaml} | 4 +-- CHANGELOG.md | 3 ++ cmd/gh-action-integration-generator/main.go | 2 +- config-example.yaml | 27 ++++++++++++------ config.go | 28 ++++++++++++++++++- integration/auth_oidc_test.go | 16 +++++++---- .../etc/alt-config.dump.gold.yaml | 2 ++ .../etc/alt-env-config.dump.gold.yaml | 2 ++ integration_test/etc/config.dump.gold.yaml | 2 ++ oidc.go | 24 +++++++++++++--- 10 files changed, 88 insertions(+), 22 deletions(-) rename .github/workflows/{test-integration-v2-TestOIDCExpireNodes.yaml => test-integration-v2-TestOIDCExpireNodesBasedOnTokenExpiry.yaml} (92%) diff --git a/.github/workflows/test-integration-v2-TestOIDCExpireNodes.yaml b/.github/workflows/test-integration-v2-TestOIDCExpireNodesBasedOnTokenExpiry.yaml similarity index 92% rename from .github/workflows/test-integration-v2-TestOIDCExpireNodes.yaml rename to .github/workflows/test-integration-v2-TestOIDCExpireNodesBasedOnTokenExpiry.yaml index db2ea62b..287d129b 100644 --- a/.github/workflows/test-integration-v2-TestOIDCExpireNodes.yaml +++ b/.github/workflows/test-integration-v2-TestOIDCExpireNodesBasedOnTokenExpiry.yaml @@ -2,7 +2,7 @@ # DO NOT EDIT, generated with cmd/gh-action-integration-generator/main.go # To regenerate, run "go generate" in cmd/gh-action-integration-generator/ -name: Integration Test v2 - TestOIDCExpireNodes +name: Integration Test v2 - TestOIDCExpireNodesBasedOnTokenExpiry on: [pull_request] @@ -49,7 +49,7 @@ jobs: -failfast \ -timeout 120m \ -parallel 1 \ - -run "^TestOIDCExpireNodes$" + -run "^TestOIDCExpireNodesBasedOnTokenExpiry$" - uses: actions/upload-artifact@v3 if: always() && steps.changed-files.outputs.any_changed == 'true' diff --git a/CHANGELOG.md b/CHANGELOG.md index 90971fd3..593c76e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ - Fix wrong behaviour in exit nodes [#1159](https://github.com/juanfont/headscale/pull/1159) - Align behaviour of `dns_config.restricted_nameservers` to tailscale [#1162](https://github.com/juanfont/headscale/pull/1162) +- Make OpenID Connect authenticated client expiry time configurable [#1191](https://github.com/juanfont/headscale/pull/1191) + - defaults to 180 days like Tailscale SaaS + - adds option to use the expiry time from the OpenID token for the node (see config-example.yaml) ## 0.19.0 (2023-01-29) diff --git a/cmd/gh-action-integration-generator/main.go b/cmd/gh-action-integration-generator/main.go index af60a7fa..6231ec35 100644 --- a/cmd/gh-action-integration-generator/main.go +++ b/cmd/gh-action-integration-generator/main.go @@ -90,7 +90,7 @@ func main() { "TestHeadscale", "TestUserCommand", "TestOIDCAuthenticationPingAll", - "TestOIDCExpireNodes", + "TestOIDCExpireNodesBasedOnTokenExpiry", "TestPingAllByHostname", "TestPingAllByIP", "TestPreAuthKeyCommand", diff --git a/config-example.yaml b/config-example.yaml index 7b4398d0..249a0442 100644 --- a/config-example.yaml +++ b/config-example.yaml @@ -282,27 +282,38 @@ unix_socket_permission: "0770" # client_secret_path: "${CREDENTIALS_DIRECTORY}/oidc_client_secret" # # client_secret and client_secret_path are mutually exclusive. # -# Customize the scopes used in the OIDC flow, defaults to "openid", "profile" and "email" and add custom query -# parameters to the Authorize Endpoint request. Scopes default to "openid", "profile" and "email". +# # The amount of time from a node is authenticated with OpenID until it +# # expires and needs to reauthenticate. +# # Setting the value to "0" will mean no expiry. +# expiry: 180d +# +# # Use the expiry from the token received from OpenID when the user logged +# # in, this will typically lead to frequent need to reauthenticate and should +# # only been enabled if you know what you are doing. +# # Note: enabling this will cause `oidc.expiry` to be ignored. +# use_expiry_from_token: false +# +# # Customize the scopes used in the OIDC flow, defaults to "openid", "profile" and "email" and add custom query +# # parameters to the Authorize Endpoint request. Scopes default to "openid", "profile" and "email". # # scope: ["openid", "profile", "email", "custom"] # extra_params: # domain_hint: example.com # -# List allowed principal domains and/or users. If an authenticated user's domain is not in this list, the -# authentication request will be rejected. +# # List allowed principal domains and/or users. If an authenticated user's domain is not in this list, the +# # authentication request will be rejected. # # allowed_domains: # - example.com -# Groups from keycloak have a leading '/' +# # Note: Groups from keycloak have a leading '/' # allowed_groups: # - /headscale # allowed_users: # - alice@example.com # -# If `strip_email_domain` is set to `true`, the domain part of the username email address will be removed. -# This will transform `first-name.last-name@example.com` to the user `first-name.last-name` -# If `strip_email_domain` is set to `false` the domain part will NOT be removed resulting to the following +# # If `strip_email_domain` is set to `true`, the domain part of the username email address will be removed. +# # This will transform `first-name.last-name@example.com` to the user `first-name.last-name` +# # If `strip_email_domain` is set to `false` the domain part will NOT be removed resulting to the following # user: `first-name.last-name.example.com` # # strip_email_domain: true diff --git a/config.go b/config.go index fed9b032..c0dd1c98 100644 --- a/config.go +++ b/config.go @@ -11,6 +11,7 @@ import ( "time" "github.com/coreos/go-oidc/v3/oidc" + "github.com/prometheus/common/model" "github.com/rs/zerolog" "github.com/rs/zerolog/log" "github.com/spf13/viper" @@ -25,9 +26,14 @@ const ( JSONLogFormat = "json" TextLogFormat = "text" + + defaultOIDCExpiryTime = 180 * 24 * time.Hour // 180 Days + maxDuration time.Duration = 1<<63 - 1 ) -var errOidcMutuallyExclusive = errors.New("oidc_client_secret and oidc_client_secret_path are mutually exclusive") +var errOidcMutuallyExclusive = errors.New( + "oidc_client_secret and oidc_client_secret_path are mutually exclusive", +) // Config contains the initial Headscale configuration. type Config struct { @@ -101,6 +107,8 @@ type OIDCConfig struct { AllowedUsers []string AllowedGroups []string StripEmaildomain bool + Expiry time.Duration + UseExpiryFromToken bool } type DERPConfig struct { @@ -180,6 +188,8 @@ func LoadConfig(path string, isFile bool) error { viper.SetDefault("oidc.scope", []string{oidc.ScopeOpenID, "profile", "email"}) viper.SetDefault("oidc.strip_email_domain", true) viper.SetDefault("oidc.only_start_if_oidc_is_available", true) + viper.SetDefault("oidc.expiry", "180d") + viper.SetDefault("oidc.use_expiry_from_token", false) viper.SetDefault("logtail.enabled", false) viper.SetDefault("randomize_client_port", false) @@ -601,6 +611,22 @@ func GetHeadscaleConfig() (*Config, error) { AllowedUsers: viper.GetStringSlice("oidc.allowed_users"), AllowedGroups: viper.GetStringSlice("oidc.allowed_groups"), StripEmaildomain: viper.GetBool("oidc.strip_email_domain"), + Expiry: func() time.Duration { + // if set to 0, we assume no expiry + if value := viper.GetString("oidc.expiry"); value == "0" { + return maxDuration + } else { + expiry, err := model.ParseDuration(value) + if err != nil { + log.Warn().Msg("failed to parse oidc.expiry, defaulting back to 180 days") + + return defaultOIDCExpiryTime + } + + return time.Duration(expiry) + } + }(), + UseExpiryFromToken: viper.GetBool("oidc.use_expiry_from_token"), }, LogTail: logConfig, diff --git a/integration/auth_oidc_test.go b/integration/auth_oidc_test.go index e9803457..f3ac040d 100644 --- a/integration/auth_oidc_test.go +++ b/integration/auth_oidc_test.go @@ -100,7 +100,7 @@ func TestOIDCAuthenticationPingAll(t *testing.T) { } } -func TestOIDCExpireNodes(t *testing.T) { +func TestOIDCExpireNodesBasedOnTokenExpiry(t *testing.T) { IntegrationSkip(t) t.Parallel() @@ -125,10 +125,11 @@ func TestOIDCExpireNodes(t *testing.T) { } oidcMap := map[string]string{ - "HEADSCALE_OIDC_ISSUER": oidcConfig.Issuer, - "HEADSCALE_OIDC_CLIENT_ID": oidcConfig.ClientID, - "HEADSCALE_OIDC_CLIENT_SECRET": oidcConfig.ClientSecret, - "HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN": fmt.Sprintf("%t", oidcConfig.StripEmaildomain), + "HEADSCALE_OIDC_ISSUER": oidcConfig.Issuer, + "HEADSCALE_OIDC_CLIENT_ID": oidcConfig.ClientID, + "HEADSCALE_OIDC_CLIENT_SECRET": oidcConfig.ClientSecret, + "HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN": fmt.Sprintf("%t", oidcConfig.StripEmaildomain), + "HEADSCALE_OIDC_USE_EXPIRY_FROM_TOKEN": "1", } err = scenario.CreateHeadscaleEnv( @@ -278,7 +279,10 @@ func (s *AuthOIDCScenario) runMockOIDC(accessTTL time.Duration) (*headscale.OIDC log.Printf("headscale mock oidc is ready for tests at %s", hostEndpoint) return &headscale.OIDCConfig{ - Issuer: fmt.Sprintf("http://%s/oidc", net.JoinHostPort(s.mockOIDC.GetIPInNetwork(s.network), strconv.Itoa(port))), + Issuer: fmt.Sprintf( + "http://%s/oidc", + net.JoinHostPort(s.mockOIDC.GetIPInNetwork(s.network), strconv.Itoa(port)), + ), ClientID: "superclient", ClientSecret: "supersecret", StripEmaildomain: true, diff --git a/integration_test/etc/alt-config.dump.gold.yaml b/integration_test/etc/alt-config.dump.gold.yaml index 6a51d904..c0665b30 100644 --- a/integration_test/etc/alt-config.dump.gold.yaml +++ b/integration_test/etc/alt-config.dump.gold.yaml @@ -37,12 +37,14 @@ logtail: enabled: false metrics_listen_addr: 127.0.0.1:19090 oidc: + expiry: 180d only_start_if_oidc_is_available: true scope: - openid - profile - email strip_email_domain: true + use_expiry_from_token: false private_key_path: private.key noise: private_key_path: noise_private.key diff --git a/integration_test/etc/alt-env-config.dump.gold.yaml b/integration_test/etc/alt-env-config.dump.gold.yaml index 908b6326..0be5d9ed 100644 --- a/integration_test/etc/alt-env-config.dump.gold.yaml +++ b/integration_test/etc/alt-env-config.dump.gold.yaml @@ -36,12 +36,14 @@ logtail: enabled: false metrics_listen_addr: 127.0.0.1:19090 oidc: + expiry: 180d only_start_if_oidc_is_available: true scope: - openid - profile - email strip_email_domain: true + use_expiry_from_token: false private_key_path: private.key noise: private_key_path: noise_private.key diff --git a/integration_test/etc/config.dump.gold.yaml b/integration_test/etc/config.dump.gold.yaml index 2e9675f7..e6a822a5 100644 --- a/integration_test/etc/config.dump.gold.yaml +++ b/integration_test/etc/config.dump.gold.yaml @@ -37,12 +37,14 @@ logtail: enabled: false metrics_listen_addr: 127.0.0.1:9090 oidc: + expiry: 180d only_start_if_oidc_is_available: true scope: - openid - profile - email strip_email_domain: true + use_expiry_from_token: false private_key_path: private.key noise: private_key_path: noise_private.key diff --git a/oidc.go b/oidc.go index 4909ba99..5aa3ba62 100644 --- a/oidc.go +++ b/oidc.go @@ -27,8 +27,10 @@ const ( errOIDCAllowedDomains = Error("authenticated principal does not match any allowed domain") errOIDCAllowedGroups = Error("authenticated principal is not in any allowed group") errOIDCAllowedUsers = Error("authenticated principal does not match any allowed user") - errOIDCInvalidMachineState = Error("requested machine state key expired before authorisation completed") - errOIDCNodeKeyMissing = Error("could not get node key from cache") + errOIDCInvalidMachineState = Error( + "requested machine state key expired before authorisation completed", + ) + errOIDCNodeKeyMissing = Error("could not get node key from cache") ) type IDTokenClaims struct { @@ -68,6 +70,14 @@ func (h *Headscale) initOIDC() error { return nil } +func (h *Headscale) determineTokenExpiration(idTokenExpiration time.Time) time.Time { + if h.cfg.OIDC.UseExpiryFromToken { + return idTokenExpiration + } + + return time.Now().Add(h.cfg.OIDC.Expiry) +} + // RegisterOIDC redirects to the OIDC provider for authentication // Puts NodeKey in cache so the callback can retrieve it using the oidc state param // Listens in /oidc/register/:nKey. @@ -193,6 +203,7 @@ func (h *Headscale) OIDCCallback( if err != nil { return } + idTokenExpiry := h.determineTokenExpiration(idToken.Expiry) // TODO: we can use userinfo at some point to grab additional information about the user (groups membership, etc) // userInfo, err := oidcProvider.UserInfo(context.Background(), oauth2.StaticTokenSource(oauth2Token)) @@ -218,7 +229,12 @@ func (h *Headscale) OIDCCallback( return } - nodeKey, machineExists, err := h.validateMachineForOIDCCallback(writer, state, claims, idToken.Expiry) + nodeKey, machineExists, err := h.validateMachineForOIDCCallback( + writer, + state, + claims, + idTokenExpiry, + ) if err != nil || machineExists { return } @@ -236,7 +252,7 @@ func (h *Headscale) OIDCCallback( return } - if err := h.registerMachineForOIDCCallback(writer, user, nodeKey, idToken.Expiry); err != nil { + if err := h.registerMachineForOIDCCallback(writer, user, nodeKey, idTokenExpiry); err != nil { return }