Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

connector/oidc: expose oauth2.RegisterBrokenAuthHeaderProvider #860

Merged
merged 2 commits into from
Mar 20, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions Documentation/oidc-connector.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# 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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down
55 changes: 55 additions & 0 deletions connector/oidc/oidc.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ import (
"errors"
"fmt"
"net/http"
"net/url"
"strings"
"sync"

"github.com/Sirupsen/logrus"
"github.com/coreos/go-oidc"
Expand All @@ -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 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO implementing a known list of broken providers isn't a good idea. This list will grow and grow over time just like it has in oauth2 violating the open close principle.

I think it's better to rely on clear documentation in this case

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conversely, not all IDPs have nice error messages like Okta so things could fail and operators will have no idea why... So I can see both sides of it

Copy link
Contributor Author

@ericchiang ericchiang Mar 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can detect cases where someone will always have to set the basicAuthUnsupported flag then I think we should just avoid the overhead and do it automatically. These providers being weird shouldn't cost our users ease of use, and maintaining a list isn't that bad.

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
Expand All @@ -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...)
Expand Down
23 changes: 23 additions & 0 deletions connector/oidc/oidc_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
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)
}
}
}