Merge pull request #860 from ericchiang/oidc-broken-auth-header

connector/oidc: expose oauth2.RegisterBrokenAuthHeaderProvider

Documentation/oidc-connector.md

+# Authentication through an OpenID Connect provider
+
+## Overview
+
+Dex is able to use another OpenID Connect provider as an authentication source. When logging in, dex will redirect to the upstream provider and perform the necessary OAuth2 flows to determine the end users email, username, etc. More details on the OpenID Connect protocol can be found in [_An overview of OpenID Connect_][oidc-doc].
+
+Prominent examples of OpenID Connect providers include Google Accounts, Salesforce, and Azure AD v2 ([not v1][azure-ad-v1]).
+
+## Caveats
+
+Many OpenID Connect providers implement different restrictions on refresh tokens. For example, Google will only issue the first login attempt a refresh token, then not return one after. Because of this, this connector does not refresh the id_token claims when a client of dex redeems a refresh token, which can result in stale user info.
+
+It's generally recommended to avoid using refresh tokens with the `oidc` connector.
+
+Progress on this caveat can be tracked in [issue #863][google-refreshing].
+
+## Configuration
+
+```yaml
+connectors:
+- type: oidc
+  id: google
+  name: Google
+  config:
+    # Canonical URL of the provider, also used for configuration discovery.
+    # This value MUST match the value returned in the provider config discovery.
+    #
+    # See: https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig
+    issuer: https://accounts.google.com
+
+    # Connector config values starting with a "$" will read from the environment.
+    clientID: $GOOGLE_CLIENT_ID
+    clientSecret: $GOOGLE_CLIENT_SECRET
+
+    # Dex's issuer URL + "/callback"
+    redirectURI: http://127.0.0.1:5556/callback
+
+
+    # Some providers require passing client_secret via POST parameters instead
+    # of basic auth, despite the OAuth2 RFC discouraging it. Many of these
+    # cases are caught internally, but some may need to uncommented the
+    # following field.
+    #
+    # basicAuthUnsupported: true
+```
+
+[oidc-doc]: openid-connect.md
+[google-refreshing]: https://github.com/coreos/dex/issues/863
+[azure-ad-v1]: https://github.com/coreos/go-oidc/issues/133

README.md

@@ -38,6 +38,7 @@ More docs for running dex as a Kubernetes authenticator can be found [here](Docu
   * [LDAP](Documentation/ldap-connector.md)
   * [GitHub](Documentation/github-connector.md)
   * [SAML 2.0 (experimental)](Documentation/saml-connector.md)
+  * [OpenID Connect](Documentation/oidc-connector.md) (includes Google, Salesforce, Azure, etc.)
 * Client libraries
   * [Go][go-oidc]
 

connector/oidc/oidc.go

@@ -6,6 +6,9 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"net/url"
+	"strings"
+	"sync"
 
 	"github.com/Sirupsen/logrus"
 	"github.com/coreos/go-oidc"
@@ -21,7 +24,50 @@ type Config struct {
 	ClientSecret string `json:"clientSecret"`
 	RedirectURI  string `json:"redirectURI"`
 
+	// Causes client_secret to be passed as POST parameters instead of basic
+	// auth. This is specifically "NOT RECOMMENDED" by the OAuth2 RFC, but some
+	// providers require it.
+	//
+	// https://tools.ietf.org/html/rfc6749#section-2.3.1
+	BasicAuthUnsupported *bool `json:"basicAuthUnsupported"`
+
 	Scopes []string `json:"scopes"` // defaults to "profile" and "email"
+
+}
+
+// Domains that don't support basic auth. golang.org/x/oauth2 has an internal
+// list, but it only matches specific URLs, not top level domains.
+var brokenAuthHeaderDomains = []string{
+	// See: https://github.com/coreos/dex/issues/859
+	"okta.com",
+	"oktapreview.com",
+}
+
+// Detect auth header provider issues for known providers. This lets users
+// avoid having to explicitly set "basicAuthUnsupported" in their config.
+//
+// Setting the config field always overrides values returned by this function.
+func knownBrokenAuthHeaderProvider(issuerURL string) bool {
+	if u, err := url.Parse(issuerURL); err == nil {
+		for _, host := range brokenAuthHeaderDomains {
+			if u.Host == host || strings.HasSuffix(u.Host, "."+host) {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+// golang.org/x/oauth2 doesn't do internal locking. Need to do it in this
+// package ourselves and hope that other packages aren't calling it at the
+// same time.
+var registerMu = new(sync.Mutex)
+
+func registerBrokenAuthHeaderProvider(url string) {
+	registerMu.Lock()
+	defer registerMu.Unlock()
+
+	oauth2.RegisterBrokenAuthHeaderProvider(url)
 }
 
 // Open returns a connector which can be used to login users through an upstream
@@ -35,6 +81,15 @@ func (c *Config) Open(logger logrus.FieldLogger) (conn connector.Connector, err
 		return nil, fmt.Errorf("failed to get provider: %v", err)
 	}
 
+	if c.BasicAuthUnsupported != nil {
+		// Setting "basicAuthUnsupported" always overrides our detection.
+		if *c.BasicAuthUnsupported {
+			registerBrokenAuthHeaderProvider(provider.Endpoint().TokenURL)
+		}
+	} else if knownBrokenAuthHeaderProvider(c.Issuer) {
+		registerBrokenAuthHeaderProvider(provider.Endpoint().TokenURL)
+	}
+
 	scopes := []string{oidc.ScopeOpenID}
 	if len(c.Scopes) > 0 {
 		scopes = append(scopes, c.Scopes...)

connector/oidc/oidc_test.go

+package oidc
+
+import "testing"
+
+func TestKnownBrokenAuthHeaderProvider(t *testing.T) {
+	tests := []struct {
+		issuerURL string
+		expect    bool
+	}{
+		{"https://dev.oktapreview.com", true},
+		{"https://dev.okta.com", true},
+		{"https://okta.com", true},
+		{"https://dev.oktaaccounts.com", false},
+		{"https://accounts.google.com", false},
+	}
+
+	for _, tc := range tests {
+		got := knownBrokenAuthHeaderProvider(tc.issuerURL)
+		if got != tc.expect {
+			t.Errorf("knownBrokenAuthHeaderProvider(%q), want=%t, got=%t", tc.issuerURL, tc.expect, got)
+		}
+	}
+}