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

Added function to create a Provider without OIDC discovery #179

Closed
wants to merge 1 commit into from
Closed

Added function to create a Provider without OIDC discovery #179

wants to merge 1 commit into from

Conversation

ingopansa
Copy link

There are situations where an OIDC Provider does not provide support for the OIDC Provider Discovery Protocol.

This PR adds support to the widely used go-oidc lib to allow integration of these "manual" OIDC providers.

Happy to discuss!

@ericchiang
Copy link
Collaborator

Thanks for the PR!

There's already a way to create a verifier without discovery. Does that fit your use case? https://godoc.org/github.com/coreos/go-oidc#NewVerifier

@ingopansa
Copy link
Author

ingopansa commented Sep 25, 2018

Hi Eric,

There's already a way to create a verifier without discovery. Does that fit your use case? https://godoc.org/github.com/coreos/go-oidc#NewVerifier

I looked into this as well, however, what we really actually want is Provider object, so we can make use of all the type functions already being in place.

However - all the members of the provider type are private, so auth url, token url, ... cannot be set, and this seems to be the reason why the NewProvider() function exists in first place. Either we would extend the signature for the NewProvider function in order to provide the values (which might break a lot of things), or we set the member values through setters, or we make the values public (which also breaks stuff).

The option of having a dedicated function to construct a Provider object through providing the required values seems to be most attractive to me.

Again - happy to discuss, if I might miss something huge in here :)

@ericchiang
Copy link
Collaborator

Can you explain why and provide some code which demonstrated why you need a provider? That's what I'm confused about. Providers are usually means to get other structs or information. They're not generally the end thing you're trying to get.

@ingopansa
Copy link
Author

Sure! Really happy to see this discussed and I appreciate your valuable feedback!

One example is the OIDC connector for dexidp. In the Open() function, I do the following, instead of directly calling oidc.NewProvider()

ctx, cancel := context.WithCancel(context.Background())

	provider, err := c.getProvider(ctx)
	if err != nil {
		cancel()
		return nil, fmt.Errorf("failed to get provider: %v", err)
	}

With getProvider():

func (c *Config) getProvider(ctx context.Context) (*oidc.Provider, error) {
	if c.ManualIssuer {
		return oidc.NewProviderWithoutDiscovery(ctx, c.Issuer, c.AuthURL, c.TokenURL, c.UserInfoURL, c.JwksURL)
	} else {
		return oidc.NewProvider(ctx, c.Issuer)
	}
}

We extended the dex config file to account for manually configuration an OIDC connector. While one might argue that it might be worth having a separate type of OIDC connector, I feel like manual discovery is more like a bevahiour detail, rather than a fundamental difference. Approaching this solution, we can reuse pretty much all the existing code for the connector.

(bottom line: we are also working on providing a PR for dexidp)

@ericchiang
Copy link
Collaborator

You can do that without a change to go-oidc :)

Feel free to work off this diff.

diff --git a/connector/oidc/oidc.go b/connector/oidc/oidc.go
index 4468edc..7f6cb53 100644
--- a/connector/oidc/oidc.go
+++ b/connector/oidc/oidc.go
@@ -10,7 +10,7 @@ import (
        "strings"
        "sync"
 
-       "github.com/coreos/go-oidc"
+       oidc "github.com/coreos/go-oidc"
        "github.com/sirupsen/logrus"
        "golang.org/x/oauth2"
 
@@ -36,6 +36,11 @@ type Config struct {
        // Optional list of whitelisted domains when using Google
        // If this field is nonempty, only users from a listed domain will be allowed to log in
        HostedDomains []string `json:"hostedDomains"`
+
+       // Manually provided endpoints.
+       AuthURL  string
+       TokenURL string
+       JWKURL   string
 }
 
 // Domains that don't support basic auth. golang.org/x/oauth2 has an internal
@@ -78,19 +83,39 @@ func registerBrokenAuthHeaderProvider(url string) {
 func (c *Config) Open(id string, logger logrus.FieldLogger) (conn connector.Connector, err error) {
        ctx, cancel := context.WithCancel(context.Background())
 
-       provider, err := oidc.NewProvider(ctx, c.Issuer)
-       if err != nil {
-               cancel()
-               return nil, fmt.Errorf("failed to get provider: %v", err)
+       var (
+               endpoint        oauth2.Endpoint
+               idTokenVerifier *oidc.IDTokenVerifier
+               verifierConfig  = &oidc.Config{ClientID: c.ClientID}
+       )
+
+       if c.TokenURL == "" {
+               provider, err := oidc.NewProvider(ctx, c.Issuer)
+               if err != nil {
+                       cancel()
+                       return nil, fmt.Errorf("failed to get provider: %v", err)
+               }
+               endpoint = provider.Endpoint()
+               idTokenVerifier = provider.Verifier(verifierConfig)
+       } else {
+               endpoint = oauth2.Endpoint{
+                       AuthURL:  c.AuthURL,
+                       TokenURL: c.TokenURL,
+               }
+               idTokenVerifier = oidc.NewVerifier(
+                       c.Issuer,
+                       oidc.NewRemoteKeySet(ctx, c.JWKURL),
+                       verifierConfig,
+               )
        }
 
        if c.BasicAuthUnsupported != nil {
                // Setting "basicAuthUnsupported" always overrides our detection.
                if *c.BasicAuthUnsupported {
-                       registerBrokenAuthHeaderProvider(provider.Endpoint().TokenURL)
+                       registerBrokenAuthHeaderProvider(endpoint.TokenURL)
                }
        } else if knownBrokenAuthHeaderProvider(c.Issuer) {
-               registerBrokenAuthHeaderProvider(provider.Endpoint().TokenURL)
+               registerBrokenAuthHeaderProvider(endpoint.TokenURL)
        }
 
        scopes := []string{oidc.ScopeOpenID}
@@ -100,19 +125,16 @@ func (c *Config) Open(id string, logger logrus.FieldLogger) (conn connector.Conn
                scopes = append(scopes, "profile", "email")
        }
 
-       clientID := c.ClientID
        return &oidcConnector{
                redirectURI: c.RedirectURI,
                oauth2Config: &oauth2.Config{
-                       ClientID:     clientID,
+                       ClientID:     c.ClientID,
                        ClientSecret: c.ClientSecret,
-                       Endpoint:     provider.Endpoint(),
+                       Endpoint:     endpoint,
                        Scopes:       scopes,
                        RedirectURL:  c.RedirectURI,
                },
-               verifier: provider.Verifier(
-                       &oidc.Config{ClientID: clientID},
-               ),
+               verifier:      idTokenVerifier,
                logger:        logger,
                cancel:        cancel,
                hostedDomains: c.HostedDomains,

@ingopansa
Copy link
Author

Great, thanks! Does exactly the job!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants