From 89b761aa362adbf7a042797282c3a9500d2f618f Mon Sep 17 00:00:00 2001 From: Sergi Castro Date: Wed, 21 Feb 2024 18:11:16 +0100 Subject: [PATCH] Validate callback and logout paths are unique (#33) --- internal/config.go | 32 +++++++++++++++++++ internal/config_test.go | 8 +++-- internal/testdata/duplicate-oidc.json | 4 +-- .../testdata/invalid-callback-logout.json | 29 +++++++++++++++++ internal/testdata/invalid-callback.json | 26 +++++++++++++++ internal/testdata/invalid-logout.json | 29 +++++++++++++++++ internal/testdata/invalid-oidc-override.json | 2 +- internal/testdata/invalid-oidc-uris.json | 2 +- internal/testdata/invalid-redis.json | 2 +- internal/testdata/multiple-oidc.json | 4 +-- internal/testdata/oidc-dynamic.json | 2 +- internal/testdata/oidc-override.json | 6 +++- internal/testdata/oidc.json | 6 +++- 13 files changed, 140 insertions(+), 12 deletions(-) create mode 100644 internal/testdata/invalid-callback-logout.json create mode 100644 internal/testdata/invalid-callback.json create mode 100644 internal/testdata/invalid-logout.json diff --git a/internal/config.go b/internal/config.go index 1e7e78d..6bec876 100644 --- a/internal/config.go +++ b/internal/config.go @@ -42,6 +42,8 @@ var ( ErrInvalidURL = errors.New("invalid URL") ErrRequiredURL = errors.New("required URL") ErrHealthPortInUse = errors.New("health port is already in use by listen port") + ErrMustNotBeRootPath = errors.New("must not be root path") + ErrMustBeDifferentPath = errors.New("must be different path") ) // LocalConfigFile is a run.Config that loads the configuration file. @@ -152,6 +154,12 @@ func mergeAndValidateOIDCConfigs(cfg *configv1.Config) error { // Set the defaults applyOIDCDefaults(f.GetOidc()) + + // validate the callback and the logout path are different + callbackURI, _ := url.Parse(f.GetOidc().GetCallbackUri()) + if f.GetOidc().GetLogout() != nil && callbackURI.Path == f.GetOidc().GetLogout().GetPath() { + errs = append(errs, fmt.Errorf("%w: callback and logout paths must be different in chain %q", ErrMustBeDifferentPath, fc.Name)) + } } } // Clear the default config as it has already been merged. This way there is only one @@ -233,6 +241,15 @@ func validateOIDCConfigURLs(c *oidcv1.OIDCConfig) error { return fmt.Errorf("%w: invalid Redis session store URL: %w", ErrInvalidURL, err) } } + + if hasRootPath(c.GetCallbackUri()) { + return fmt.Errorf("%w: invalid callback URL", ErrMustNotBeRootPath) + } + if c.GetLogout() != nil { + if isRootPath(c.GetLogout().GetPath()) { + return fmt.Errorf("%w: invalid logout path", ErrMustNotBeRootPath) + } + } return nil } @@ -243,3 +260,18 @@ func validateURL(u string) error { _, err := url.Parse(u) return err } + +// hasRootPath returns true if the path of the given URL is "/" or empty. +// prerequisite: u is a valid URL. +func hasRootPath(uri string) bool { + if uri == "" { + return false + } + parsed, _ := url.Parse(uri) + return isRootPath(parsed.Path) +} + +// isRootPath returns true if the path is "/" or empty. +func isRootPath(path string) bool { + return path == "/" || path == "" +} diff --git a/internal/config_test.go b/internal/config_test.go index c745a81..524914a 100644 --- a/internal/config_test.go +++ b/internal/config_test.go @@ -62,6 +62,9 @@ func TestValidateConfig(t *testing.T) { {"invalid-redis", "testdata/invalid-redis.json", errCheck{is: ErrInvalidURL}}, {"invalid-oidc-uris", "testdata/invalid-oidc-uris.json", errCheck{is: ErrRequiredURL}}, {"invalid-health-port", "testdata/invalid-health-port.json", errCheck{is: ErrHealthPortInUse}}, + {"invalid-callback-uri", "testdata/invalid-callback.json", errCheck{is: ErrMustNotBeRootPath}}, + {"invalid-logout-path", "testdata/invalid-logout.json", errCheck{is: ErrMustNotBeRootPath}}, + {"invalid-callback-and-logout-path", "testdata/invalid-callback-logout.json", errCheck{is: ErrMustBeDifferentPath}}, {"oidc-dynamic", "testdata/oidc-dynamic.json", errCheck{is: nil}}, {"valid", "testdata/mock.json", errCheck{is: nil}}, } @@ -76,7 +79,7 @@ func TestValidateConfig(t *testing.T) { func TestValidateURLs(t *testing.T) { const ( - validURL = "http://fake" + validURL = "http://fake/path" invalidURL = "ht tp://invalid" validRedisURL = "redis://localhost:6379/0" ) @@ -202,7 +205,7 @@ func TestLoadOIDC(t *testing.T) { Oidc: &oidcv1.OIDCConfig{ AuthorizationUri: "http://fake", TokenUri: "http://fake", - CallbackUri: "http://fake", + CallbackUri: "http://fake/callback", JwksConfig: &oidcv1.OIDCConfig_Jwks{Jwks: "fake-jwks"}, ClientId: "fake-client-id", ClientSecret: "fake-client-secret", @@ -211,6 +214,7 @@ func TestLoadOIDC(t *testing.T) { ProxyUri: "http://fake", RedisSessionStoreConfig: &oidcv1.RedisConfig{ServerUri: "redis://localhost:6379/0"}, Scopes: []string{scopeOIDC}, + Logout: &oidcv1.LogoutConfig{Path: "/logout", RedirectUri: "http://fake"}, }, }, }, diff --git a/internal/testdata/duplicate-oidc.json b/internal/testdata/duplicate-oidc.json index 47b6b3a..2e134c5 100644 --- a/internal/testdata/duplicate-oidc.json +++ b/internal/testdata/duplicate-oidc.json @@ -5,7 +5,7 @@ "default_oidc_config": { "authorization_uri": "http://fake", "token_uri": "http://fake", - "callback_uri": "http://fake", + "callback_uri": "http://fake/callback", "proxy_uri": "http://fake", "jwks": "fake-jwks", "client_id": "fake-client-id", @@ -23,7 +23,7 @@ "oidc": { "authorization_uri": "http://fake", "token_uri": "http://fake", - "callback_uri": "http://fake", + "callback_uri": "http://fake/callback", "proxy_uri": "http://fake", "jwks": "fake-jwks", "client_id": "fake-client-id", diff --git a/internal/testdata/invalid-callback-logout.json b/internal/testdata/invalid-callback-logout.json new file mode 100644 index 0000000..b103c94 --- /dev/null +++ b/internal/testdata/invalid-callback-logout.json @@ -0,0 +1,29 @@ +{ + "listen_address": "0.0.0.0", + "listen_port": 8080, + "log_level": "debug", + "chains": [ + { + "name": "mock", + "filters": [ + { + "oidc": { + "callback_uri": "http://fake/same", + "authorization_uri": "http://fake", + "token_uri": "http://fake", + "client_id": "fake", + "client_secret": "fake", + "jwks": "fake", + "id_token": { + "header": "authorization", + "preamble": "Bearer" + }, + "logout": { + "path": "/same" + } + } + } + ] + } + ] +} diff --git a/internal/testdata/invalid-callback.json b/internal/testdata/invalid-callback.json new file mode 100644 index 0000000..fd3bc14 --- /dev/null +++ b/internal/testdata/invalid-callback.json @@ -0,0 +1,26 @@ +{ + "listen_address": "0.0.0.0", + "listen_port": 8080, + "log_level": "debug", + "chains": [ + { + "name": "mock", + "filters": [ + { + "oidc": { + "callback_uri": "http://fake", + "authorization_uri": "http://fake", + "token_uri": "http://fake", + "client_id": "fake", + "client_secret": "fake", + "jwks": "fake", + "id_token": { + "header": "authorization", + "preamble": "Bearer" + } + } + } + ] + } + ] +} diff --git a/internal/testdata/invalid-logout.json b/internal/testdata/invalid-logout.json new file mode 100644 index 0000000..97faa5f --- /dev/null +++ b/internal/testdata/invalid-logout.json @@ -0,0 +1,29 @@ +{ + "listen_address": "0.0.0.0", + "listen_port": 8080, + "log_level": "debug", + "chains": [ + { + "name": "mock", + "filters": [ + { + "oidc": { + "callback_uri": "http://fake/callback", + "authorization_uri": "http://fake", + "token_uri": "http://fake", + "client_id": "fake", + "client_secret": "fake", + "jwks": "fake", + "id_token": { + "header": "authorization", + "preamble": "Bearer" + }, + "logout": { + "path": "/" + } + } + } + ] + } + ] +} diff --git a/internal/testdata/invalid-oidc-override.json b/internal/testdata/invalid-oidc-override.json index afa39ee..336fe50 100644 --- a/internal/testdata/invalid-oidc-override.json +++ b/internal/testdata/invalid-oidc-override.json @@ -10,7 +10,7 @@ "oidc_override": { "authorization_uri": "http://fake", "token_uri": "http://fake", - "callback_uri": "http://fake", + "callback_uri": "http://fake/callback", "proxy_uri": "http://fake", "jwks": "fake-jwks", "client_id": "fake-client-id", diff --git a/internal/testdata/invalid-oidc-uris.json b/internal/testdata/invalid-oidc-uris.json index 6a67b82..d688bcc 100644 --- a/internal/testdata/invalid-oidc-uris.json +++ b/internal/testdata/invalid-oidc-uris.json @@ -8,7 +8,7 @@ "filters": [ { "oidc": { - "callback_uri": "http://fake", + "callback_uri": "http://fake/callback", "proxy_uri": "http://fake", "client_id": "fake-client-id", "client_secret": "fake-client-secret", diff --git a/internal/testdata/invalid-redis.json b/internal/testdata/invalid-redis.json index 14ce9bd..1c5d9fc 100644 --- a/internal/testdata/invalid-redis.json +++ b/internal/testdata/invalid-redis.json @@ -10,7 +10,7 @@ "oidc": { "authorization_uri": "http://fake", "token_uri": "http://fake", - "callback_uri": "http://fake", + "callback_uri": "http://fake/callback", "proxy_uri": "http://fake", "jwks": "fake-jwks", "client_id": "fake-client-id", diff --git a/internal/testdata/multiple-oidc.json b/internal/testdata/multiple-oidc.json index 292d326..dafe29b 100644 --- a/internal/testdata/multiple-oidc.json +++ b/internal/testdata/multiple-oidc.json @@ -10,7 +10,7 @@ "oidc": { "authorization_uri": "http://fake", "token_uri": "http://fake", - "callback_uri": "http://fake", + "callback_uri": "http://fake/callback", "proxy_uri": "http://fake", "jwks": "fake-jwks", "client_id": "fake-client-id", @@ -25,7 +25,7 @@ "oidc": { "authorization_uri": "http://fake", "token_uri": "http://fake", - "callback_uri": "http://fake", + "callback_uri": "http://fake/callback", "proxy_uri": "http://fake", "jwks": "fake-jwks", "client_id": "fake-client-id", diff --git a/internal/testdata/oidc-dynamic.json b/internal/testdata/oidc-dynamic.json index 26599b6..22d0d29 100644 --- a/internal/testdata/oidc-dynamic.json +++ b/internal/testdata/oidc-dynamic.json @@ -9,7 +9,7 @@ { "oidc": { "configuration_uri": "http://fake", - "callback_uri": "http://fake", + "callback_uri": "http://fake/callback", "proxy_uri": "http://fake", "client_id": "fake-client-id", "client_secret": "fake-client-secret", diff --git a/internal/testdata/oidc-override.json b/internal/testdata/oidc-override.json index 82f060d..cbf45b9 100644 --- a/internal/testdata/oidc-override.json +++ b/internal/testdata/oidc-override.json @@ -5,7 +5,7 @@ "default_oidc_config": { "authorization_uri": "http://default", "token_uri": "http://default", - "callback_uri": "http://fake", + "callback_uri": "http://fake/callback", "proxy_uri": "http://fake" }, "chains": [ @@ -25,6 +25,10 @@ }, "redis_session_store_config": { "server_uri": "tcp://localhost:6379/0" + }, + "logout": { + "path": "/logout", + "redirect_uri": "http://fake" } } } diff --git a/internal/testdata/oidc.json b/internal/testdata/oidc.json index 29918b8..6673f30 100644 --- a/internal/testdata/oidc.json +++ b/internal/testdata/oidc.json @@ -10,7 +10,7 @@ "oidc": { "authorization_uri": "http://fake", "token_uri": "http://fake", - "callback_uri": "http://fake", + "callback_uri": "http://fake/callback", "proxy_uri": "http://fake", "jwks": "fake-jwks", "client_id": "fake-client-id", @@ -21,6 +21,10 @@ }, "redis_session_store_config": { "server_uri": "redis://localhost:6379/0" + }, + "logout": { + "path": "/logout", + "redirect_uri": "http://fake" } } }