From cb2ad555ce12f44af90f61ef73e7e2904af70a2c Mon Sep 17 00:00:00 2001 From: hackerman <3372410+aeneasr@users.noreply.github.com> Date: Thu, 11 Apr 2019 20:39:57 +0200 Subject: [PATCH] oauth2: Allow whitelisting insecure redirect URLs (#1354) This patch enables developers to whitelist insecure redirect URLs while using flag `--dangerous-force-http`. Closes #1021 Signed-off-by: aeneasr --- cmd/cli/handler_migrate.go | 4 +-- cmd/serve.go | 4 +-- cmd/server/handler.go | 3 ++ driver/configuration/provider.go | 5 +++ driver/configuration/provider_viper.go | 23 +++++++++---- driver/configuration/provider_viper_test.go | 2 +- driver/driver_default.go | 4 +-- driver/registry_base.go | 1 + go.mod | 2 +- go.sum | 5 ++- internal/driver.go | 4 +-- x/redirect_uri.go | 27 +++++++++++++++ x/redirect_uri_test.go | 37 +++++++++++++++++++++ 13 files changed, 101 insertions(+), 20 deletions(-) create mode 100644 x/redirect_uri.go create mode 100644 x/redirect_uri_test.go diff --git a/cmd/cli/handler_migrate.go b/cmd/cli/handler_migrate.go index bf2081a0aa9..d9870082890 100644 --- a/cmd/cli/handler_migrate.go +++ b/cmd/cli/handler_migrate.go @@ -45,7 +45,7 @@ func (h *MigrateHandler) MigrateSQL(cmd *cobra.Command, args []string) { var d driver.Driver if flagx.MustGetBool(cmd, "read-from-env") { - d = driver.NewDefaultDriver(logrusx.New(), false, "", "", "") + d = driver.NewDefaultDriver(logrusx.New(), false, nil, "", "", "") if len(d.Configuration().DSN()) == 0 { fmt.Println(cmd.UsageString()) fmt.Println("") @@ -60,7 +60,7 @@ func (h *MigrateHandler) MigrateSQL(cmd *cobra.Command, args []string) { return } viper.Set(configuration.ViperKeyDSN, args[0]) - d = driver.NewDefaultDriver(logrusx.New(), false, "", "", "") + d = driver.NewDefaultDriver(logrusx.New(), false, nil, "", "", "") } reg, ok := d.Registry().(*driver.RegistrySQL) diff --git a/cmd/serve.go b/cmd/serve.go index e088a26470b..faabce34135 100644 --- a/cmd/serve.go +++ b/cmd/serve.go @@ -69,8 +69,8 @@ func init() { // Cobra supports local flags which will only run when this command // is called directly, e.g.: - serveCmd.PersistentFlags().Bool("dangerous-force-http", false, "Disable HTTP/2 over TLS (HTTPS) and serve HTTP instead. Never use this in production.") - //serveCmd.PersistentFlags().Bool("dangerous-auto-logon", false, "Stores the root credentials in ~/.hydra.yml. Do not use in production.") + serveCmd.PersistentFlags().Bool("dangerous-force-http", false, "DO NOT USE THIS IN PRODUCTION - Disables HTTP/2 over TLS (HTTPS) and serves HTTP instead") + serveCmd.PersistentFlags().StringSlice("dangerous-allow-insecure-redirect-urls", []string{}, "DO NOT USE THIS IN PRODUCTION - Disable HTTPS enforcement for the provided redirect URLs") disableTelemetryEnv := viperx.GetBool(logrusx.New(), "sqa.opt_out", "DISABLE_TELEMETRY") serveCmd.PersistentFlags().Bool("disable-telemetry", disableTelemetryEnv, "Disable anonymized telemetry reports - for more information please visit https://www.ory.sh/docs/ecosystem/sqa") diff --git a/cmd/server/handler.go b/cmd/server/handler.go index 946e4ce086e..d470f82a713 100644 --- a/cmd/server/handler.go +++ b/cmd/server/handler.go @@ -79,6 +79,7 @@ func RunServeAdmin(version, build, date string) func(cmd *cobra.Command, args [] d := driver.NewDefaultDriver( logrusx.New(), flagx.MustGetBool(cmd, "dangerous-force-http"), + flagx.MustGetStringSlice(cmd, "dangerous-allow-insecure-redirect-urls"), version, build, date, ).CallRegistry() @@ -104,6 +105,7 @@ func RunServePublic(version, build, date string) func(cmd *cobra.Command, args [ d := driver.NewDefaultDriver( logrusx.New(), flagx.MustGetBool(cmd, "dangerous-force-http"), + flagx.MustGetStringSlice(cmd, "dangerous-allow-insecure-redirect-urls"), version, build, date, ).CallRegistry() @@ -129,6 +131,7 @@ func RunServeAll(version, build, date string) func(cmd *cobra.Command, args []st d := driver.NewDefaultDriver( logrusx.New(), flagx.MustGetBool(cmd, "dangerous-force-http"), + flagx.MustGetStringSlice(cmd, "dangerous-allow-insecure-redirect-urls"), version, build, date, ).CallRegistry() diff --git a/driver/configuration/provider.go b/driver/configuration/provider.go index 273c9b4e5f4..f618c1265a0 100644 --- a/driver/configuration/provider.go +++ b/driver/configuration/provider.go @@ -16,6 +16,7 @@ type Provider interface { //HashSignature() bool IsUsingJWTAsAccessTokens() bool WellKnownKeys(include ...string) []string + InsecureRedirects() []string CORSEnabled(iface string) bool CORSOptions(iface string) cors.Options @@ -65,5 +66,9 @@ func MustValidate(l logrus.FieldLogger, p Provider) { if p.IssuerURL().Scheme != "https" { l.Fatalf(`Scheme from configuration key "%s" must be "https" unless --dangerous-force-http is passed but got scheme in value "%s" is "%s". To find out more, use "hydra help serve".`, ViperKeyIssuerURL, p.IssuerURL().String(), p.IssuerURL().Scheme) } + + if len(p.InsecureRedirects()) > 0 { + l.Fatal(`Flag --dangerous-allow-insecure-redirect-urls can only be used in combination with flag --dangerous-force-http`) + } } } diff --git a/driver/configuration/provider_viper.go b/driver/configuration/provider_viper.go index 0bedad47ef3..731359583da 100644 --- a/driver/configuration/provider_viper.go +++ b/driver/configuration/provider_viper.go @@ -22,10 +22,11 @@ import ( ) type ViperProvider struct { - l logrus.FieldLogger - ss [][]byte - generatedSecret []byte - forcedHTTP bool + l logrus.FieldLogger + ss [][]byte + generatedSecret []byte + forcedHTTP bool + insecureRedirects []string } const ( @@ -67,10 +68,14 @@ func init() { viper.AutomaticEnv() } -func NewViperProvider(l logrus.FieldLogger, forcedHTTP bool) Provider { +func NewViperProvider(l logrus.FieldLogger, forcedHTTP bool, insecureRedirects []string) Provider { + if insecureRedirects == nil { + insecureRedirects = []string{} + } return &ViperProvider{ - l: l, - forcedHTTP: forcedHTTP, + l: l, + forcedHTTP: forcedHTTP, + insecureRedirects: insecureRedirects, } } @@ -81,6 +86,10 @@ func (v *ViperProvider) getAddress(address string, port int) string { return fmt.Sprintf("%s:%d", address, port) } +func (v *ViperProvider) InsecureRedirects() []string { + return v.insecureRedirects +} + func (v *ViperProvider) WellKnownKeys(include ...string) []string { if v.AccessTokenStrategy() == "jwt" { include = append(include, x.OpenIDConnectKeyName) diff --git a/driver/configuration/provider_viper_test.go b/driver/configuration/provider_viper_test.go index dbb63de0bdd..7e3bbfbf20f 100644 --- a/driver/configuration/provider_viper_test.go +++ b/driver/configuration/provider_viper_test.go @@ -20,7 +20,7 @@ func setEnv(key, value string) func(t *testing.T) { } func TestSubjectTypesSupported(t *testing.T) { - p := NewViperProvider(logrus.New(), false) + p := NewViperProvider(logrus.New(), false, nil) viper.Set(ViperKeySubjectIdentifierAlgorithmSalt, "00000000") for k, tc := range []struct { d string diff --git a/driver/driver_default.go b/driver/driver_default.go index 829226a3de9..6dce3cbd040 100644 --- a/driver/driver_default.go +++ b/driver/driver_default.go @@ -11,8 +11,8 @@ type DefaultDriver struct { r Registry } -func NewDefaultDriver(l logrus.FieldLogger, forcedHTTP bool, version, build, date string) Driver { - c := configuration.NewViperProvider(l, forcedHTTP) +func NewDefaultDriver(l logrus.FieldLogger, forcedHTTP bool, insecureRedirects []string, version, build, date string) Driver { + c := configuration.NewViperProvider(l, forcedHTTP, insecureRedirects) configuration.MustValidate(l, c) r, err := NewRegistry(c) diff --git a/driver/registry_base.go b/driver/registry_base.go index 3ade0e78838..6e4056d62ac 100644 --- a/driver/registry_base.go +++ b/driver/registry_base.go @@ -233,6 +233,7 @@ func (m *RegistryBase) oAuth2Config() *compose.Config { EnforcePKCE: false, EnablePKCEPlainChallengeMethod: false, TokenURL: urlx.AppendPaths(m.c.PublicURL(), oauth2.TokenPath).String(), + RedirectSecureChecker: x.IsRedirectURISecure(m.c), } } diff --git a/go.mod b/go.mod index 7a3e6cdc220..d1b0d58b3c1 100644 --- a/go.mod +++ b/go.mod @@ -43,7 +43,7 @@ require ( github.com/oleiade/reflections v1.0.0 github.com/olekukonko/tablewriter v0.0.1 github.com/opentracing/opentracing-go v1.1.0 - github.com/ory/fosite v0.29.1 + github.com/ory/fosite v0.29.2 github.com/ory/go-acc v0.0.0-20181118080137-ddc355013f90 github.com/ory/go-convenience v0.1.0 github.com/ory/graceful v0.1.1 diff --git a/go.sum b/go.sum index 38e45e3f804..f42205275f0 100644 --- a/go.sum +++ b/go.sum @@ -81,7 +81,6 @@ github.com/gliderlabs/ssh v0.1.1/go.mod h1:U7qILu1NlMHj9FlMhZLlkCdDnU1DBEAqr0aev github.com/globalsign/mgo v0.0.0-20180905125535-1ca0a4f7cbcb/go.mod h1:xkRDCp4j0OGD1HRkm4kmhM+pmpv3AKq5SU7GMg4oO/Q= github.com/globalsign/mgo v0.0.0-20181015135952-eeefdecb41b8 h1:DujepqpGd1hyOd7aW59XpK7Qymp8iy83xq74fLr21is= github.com/globalsign/mgo v0.0.0-20181015135952-eeefdecb41b8/go.mod h1:xkRDCp4j0OGD1HRkm4kmhM+pmpv3AKq5SU7GMg4oO/Q= -github.com/go-bindata/go-bindata v3.1.1+incompatible h1:tR4f0e4VTO7LK6B2YWyAoVEzG9ByG1wrXB4TL9+jiYg= github.com/go-errors/errors v1.0.1 h1:LUHzmkK3GUKUrL/1gfBUxAHzcev3apQlezX/+O7ma6w= github.com/go-errors/errors v1.0.1/go.mod h1:f4zRHt4oKfwPJE5k8C9vpYG+aDHdBFUsgrm6/TyX73Q= github.com/go-kit/kit v0.8.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as= @@ -501,8 +500,8 @@ github.com/openzipkin/zipkin-go v0.1.6/go.mod h1:QgAqvLzwWbR/WpD4A3cGpPtJrZXNIiJ github.com/ory/dockertest v3.3.4+incompatible h1:VrpM6Gqg7CrPm3bL4Wm1skO+zFWLbh7/Xb5kGEbJRh8= github.com/ory/dockertest v3.3.4+incompatible/go.mod h1:1vX4m9wsvi00u5bseYwXaSnhNrne+V0E6LAcBILJdPs= github.com/ory/fosite v0.29.0/go.mod h1:0atSZmXO7CAcs6NPMI/Qtot8tmZYj04Nddoold4S2h0= -github.com/ory/fosite v0.29.1 h1:RC6pp3xt5gCILGA+9QKVowB2mqoVLlbKlbpv2/J77Q4= -github.com/ory/fosite v0.29.1/go.mod h1:0atSZmXO7CAcs6NPMI/Qtot8tmZYj04Nddoold4S2h0= +github.com/ory/fosite v0.29.2 h1:SV6tI1JBl64EPaHZ8ZTsfWGB6z4IlID+0xKoAVK+Fzo= +github.com/ory/fosite v0.29.2/go.mod h1:0atSZmXO7CAcs6NPMI/Qtot8tmZYj04Nddoold4S2h0= github.com/ory/go-acc v0.0.0-20181118080137-ddc355013f90 h1:Bpk3eqc3rbJT2mE+uS9ETzmi2cEL4RuIKz2iUeteh04= github.com/ory/go-acc v0.0.0-20181118080137-ddc355013f90/go.mod h1:sxnvPCxChFuSmTJGj8FdMupeq1BezCiEpDjTUXQ4hf4= github.com/ory/go-convenience v0.1.0 h1:zouLKfF2GoSGnJwGq+PE/nJAE6dj2Zj5QlTgmMTsTS8= diff --git a/internal/driver.go b/internal/driver.go index e76341e2311..0545cf15251 100644 --- a/internal/driver.go +++ b/internal/driver.go @@ -54,12 +54,12 @@ func resetConfig() { func NewConfigurationWithDefaults() *configuration.ViperProvider { resetConfig() - return configuration.NewViperProvider(logrusx.New(), true).(*configuration.ViperProvider) + return configuration.NewViperProvider(logrusx.New(), true, nil).(*configuration.ViperProvider) } func NewConfigurationWithDefaultsAndHTTPS() *configuration.ViperProvider { resetConfig() - return configuration.NewViperProvider(logrusx.New(), false).(*configuration.ViperProvider) + return configuration.NewViperProvider(logrusx.New(), false, nil).(*configuration.ViperProvider) } func NewRegistry(c *configuration.ViperProvider) *driver.RegistryMemory { diff --git a/x/redirect_uri.go b/x/redirect_uri.go new file mode 100644 index 00000000000..6443637ff9b --- /dev/null +++ b/x/redirect_uri.go @@ -0,0 +1,27 @@ +package x + +import ( + "net/url" + + "github.com/ory/fosite" +) + +type redirectConfiguration interface { + InsecureRedirects() []string +} + +func IsRedirectURISecure(rc redirectConfiguration) func(redirectURI *url.URL) bool { + return func(redirectURI *url.URL) bool { + if fosite.IsRedirectURISecure(redirectURI) { + return true + } + + for _, allowed := range rc.InsecureRedirects() { + if redirectURI.String() == allowed { + return true + } + } + + return false + } +} diff --git a/x/redirect_uri_test.go b/x/redirect_uri_test.go new file mode 100644 index 00000000000..77437870e16 --- /dev/null +++ b/x/redirect_uri_test.go @@ -0,0 +1,37 @@ +package x + +import ( + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type mockrc struct{} + +func (m *mockrc) InsecureRedirects() []string { + return []string{ + "http://foo.com/bar", + "http://baz.com/bar", + } +} + +func TestIsRedirectURISecure(t *testing.T) { + for d, c := range []struct { + u string + err bool + }{ + {u: "http://google.com", err: true}, + {u: "https://google.com", err: false}, + {u: "http://localhost", err: false}, + {u: "http://test.localhost", err: false}, + {u: "wta://auth", err: false}, + {u: "http://foo.com/bar", err: false}, + {u: "http://baz.com/bar", err: false}, + } { + uu, err := url.Parse(c.u) + require.NoError(t, err) + assert.Equal(t, !c.err, IsRedirectURISecure(new(mockrc))(uu), "case %d", d) + } +}