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 <[email protected]>
ory · Apr 11, 2019 · cb2ad55 · cb2ad55
1 parent 20aaa46
commit cb2ad55
Show file tree

Hide file tree

Showing 13 changed files with 101 additions and 20 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)
+	}
+}