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

Support provider without discovery #319

Closed
adombeck opened this issue Sep 22, 2021 · 12 comments
Closed

Support provider without discovery #319

adombeck opened this issue Sep 22, 2021 · 12 comments

Comments

@adombeck
Copy link

adombeck commented Sep 22, 2021

Is there a way to initialize a provider which doesn't implement the OpenID Connect Discovery spec, i.e. doesn't have a .well-known/openid-configuration JSON file? I would like to be able to specify the configuration myself in that case, for example:

provider := &oidc.Provider{
  Issuer: "https://gitlab.com/",
  AuthURL: "https://gitlab.com/oauth/authorize",
  TokenURL: "https://gitlab.com/oauth/token",
  UserInfoURL: "https://gitlab.com/oauth/userinfo",
  Algorithms: []string{"RS256"},
  RemoteKeySet: oidc.NewRemoteKeySet(ctx, "https://gitlab.com/oauth/discovery/keys")
}

The Provider fields are not exported, so I can't do it like that.

@ericchiang
Copy link
Collaborator

There's not a way. To verify ID tokens, the expectation is to create an IDTokenVerifier with NewVerifier

https://pkg.go.dev/github.com/coreos/go-oidc/v3/oidc#NewVerifier

Does that work for your use case?

@adombeck
Copy link
Author

To verify ID tokens, the expectation is to create an IDTokenVerifier with NewVerifier

https://pkg.go.dev/github.com/coreos/go-oidc/v3/oidc#NewVerifier

Does that work for your use case?

Only if there is also a way to get UserInfo without creating a Provider - those are the two things we're currently using the provider for, verifying the ID token and fetching UserInfo.

I solved that on a fork by exporting the fields of the Provider struct, so that I can set them myself. Do you think that's the right way to solve it? And could this be merged here, so that I don't have to maintain my fork?

@ericchiang
Copy link
Collaborator

There was some discussion of exposing UserInfo parsing

#248 (comment)

That way in your callback you could do:

oauth2Token, err := oauth2Config.Exchange(ctx, r.URL.Query().Get("code"))
if err != nil {
    // ...
}
resp, err := oauth2Config.Client(ctx, oauth2Token).Get(userInfoURL)
if err != nil {
    // ...
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
    // ...
}
var u oidc.UserInfo
if err := json.NewDecoder(resp.Body).Decode(&u); err != nil {
    // ...
}

Would that work?

@adombeck
Copy link
Author

Yes, I guess it would work if I re-implement the UserInfo fetching and parsing, but I would prefer to use your implementation.

@ericchiang
Copy link
Collaborator

The suggestion in #319 (comment) is to export the parsing, so you'd be in charge of the fetching, but not the parsing.

Hmm, maybe it's be clearer to provide a NewProvider function

type ProviderConfig struct {
   	IssuerURL   string
	AuthURL     string
	TokenURL    string
	UserInfoURL string
	Algorithms  []string
}

// NewProvider builds a provider without using discovery. Specific methods may not work
// depending on what URLs are supplied.
func NewProvider(c ProviderConfig) (*Provider, error) {}

The issue is that certain Provider may not work if you don't provide these fields. E.g. https://pkg.go.dev/github.com/coreos/go-oidc/v3/oidc#Provider.Verifier

So it may not make sense to actually have a Provider that returns gibberish.

@adombeck
Copy link
Author

That would work for my use case (except that the code snippet misses RemoteKeySet). But I agree that it doesn't make it clear which URLs have to be provided for which method to work. IMO using a config struct with a NewProvider function like that makes it feel more like the config fields are optional. If you export the fields of Provider instead and let the user create the provider via &Provider{...}, it feels more like the user has to know what they're doing.

@Om1kami
Copy link

Om1kami commented Sep 30, 2021

HI!
@adombeck you can use reflect to access unexported fields and set your own values to provider struct

@adombeck
Copy link
Author

Yes, that's true, I can hack my way around the unexported fields using reflect.

@andig
Copy link

andig commented Mar 22, 2022

Joining the party. Is there a compelling reason to not implement #319 (comment)? For client applications it's a pain to do the OIDC discovery roundtrips all the time.

@ericchiang
Copy link
Collaborator

Fixed with #333

@andig
Copy link

andig commented Mar 25, 2022

@ericchiang any idea how I could actually require this version?

❯ go get -u github.com/coreos/go-oidc@2d47dd95152744f41009de5797efe6cc07832a41
go: github.com/coreos/go-oidc@2d47dd95152744f41009de5797efe6cc07832a41: github.com/coreos/[email protected]: 
invalid version: go.mod has post-v0 module path "github.com/coreos/go-oidc/v3" at revision 2d47dd951527

❯ go get -u github.com/coreos/go-oidc@v3
go: github.com/coreos/go-oidc@v3: no matching versions for query "v3"

@andig
Copy link

andig commented Mar 26, 2022

Sorry for the noise. This solves it:

go get -u github.com/coreos/go-oidc/v3@2d47dd95152744f41009de5797efe6cc07832a41

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

No branches or pull requests

4 participants