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

feature request : ability to overide discovery endpoint #439

Closed
mcarbonneaux opened this issue Nov 7, 2024 · 7 comments
Closed

feature request : ability to overide discovery endpoint #439

mcarbonneaux opened this issue Nov 7, 2024 · 7 comments

Comments

@mcarbonneaux
Copy link

mcarbonneaux commented Nov 7, 2024

actualy the oidc client support only full discovery, but some time when you have dual exposition of your idp (internet and private), and if the client application are executed on private network you need to overide userinfo to go through private network to access to the idp.

in this case we need the ability to overide all the provider endpoint url.
like to add setter to all the discovered url after Provider initialisation.

like that:

       provider, err := oidc.NewProvider(ctx, issuerUrl)
       if err != nil {
		log.Fatal(err)
       }
       provider.setTokenURL("https://idp/mytokenurl")
       provider.setAuthURL("https://idp/myauthurl")
       provider.setUserInfoURL("https://idp/myuserinfourl")
       provider.setDeviceAuthURL("https://idp/mydeviceauthurl")
       provider.setJwksURL("https://idp/myjwksurl")
@ericchiang
Copy link
Collaborator

I think ProviderConfig is what you're looking for? https://pkg.go.dev/github.com/coreos/go-oidc/v3/oidc#ProviderConfig.NewProvider

@mcarbonneaux
Copy link
Author

mcarbonneaux commented Nov 8, 2024

i've seen that, but no this oblige to overide all endpoint (and also the Algorithms).
the idea is to let the discovery to do, them only change some endpoint.

func (p *ProviderConfig) NewProvider(ctx context.Context) *Provider {
	return &Provider{
		issuer:        p.IssuerURL,
		authURL:       p.AuthURL,
		tokenURL:      p.TokenURL,
		deviceAuthURL: p.DeviceAuthURL,
		userInfoURL:   p.UserInfoURL,
		jwksURL:       p.JWKSURL,
		algorithms:    p.Algorithms,
		client:        getClient(ctx),
	}
}

all the attributes must be set without that is not working (there are not defaulted to the discovery)...

in the discovery version

func NewProvider(ctx context.Context, issuer string) (*Provider, error) {
you have all the necessery to do the discovery, we need only the setter to overide value retreived....

@zepatrik
Copy link

I think it would be useful to have a (p *Provider) Config() *ProviderConfig function that can then be used with (p *ProviderConfig) NewProvider(ctx context.Context). This way one can do auto-discovery and still modify some options later, or even use it for some kind of caching.

@ericchiang
Copy link
Collaborator

The discovery URL has spec defined semantics for deriving it from the issuer

OpenID Providers supporting Discovery MUST make a JSON document available at the path formed by concatenating the string /.well-known/openid-configuration to the Issuer. The syntax and semantics of .well-known are defined in RFC 5785 [RFC5785] and apply to the Issuer value when it contains no path component. openid-configuration MUST point to a JSON document compliant with this specification and MUST be returned using the application/json content type.

https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig

If the discovery endpoint is at a different URL or returns incorrect value, please use ProviderConfig instead to specify these values.

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

We've had a lot of issues with providers doing non-spec compliant things with discovery (#344), and that API is the solution. I believe that covers this use case. If your provider is doing something odd, please parse its discovery doc and use those values to fill in ProviderConfig.

I really don't want to add more API surface for off spec providers.

@mcarbonneaux
Copy link
Author

mcarbonneaux commented Dec 24, 2024

The discovery URL has spec defined semantics for deriving it from the issuer

is generally not possible to do, because the discovery url generally expose all there endpoing the same fqdn (the idp fqdn).

if you use public discovery url for application hosted in private network (but publicly exposed with public load balancer) that not have internet access your application cannot abel to call userinfo endpoint from the private network.

If the discovery endpoint is at a different URL or returns incorrect value, please use ProviderConfig instead to specify these values.

but ProviderConfig oblige to overid all provider information, generaly you need to overide endpoint that are call between oidc application SP with the idp like UserInfoURL, TokenURL, JWKSURL when the application and the idp are hosted in private network, tho avoid to go to front internet exposition. Generaly you don't need to overide Algorithms (you need to use the who the idp know)...

ericchiang added a commit to ericchiang/go-oidc that referenced this issue Jan 4, 2025
This PR adds JSON tags to allow parsing a ProviderConfig directly from
the OpenID Connect JSON metadata document. Since this is the preferred
workaround for providers that don't support discovery in a
spec-compliant way, such as returning the wrong issuer, or requiring a
URL parameter, make this path easier and add an example to the godoc.

Updates coreos#445
Updates coreos#444
Updates coreos#439
Updates coreos#442
Updates coreos#344
Fixes coreos#290
ericchiang added a commit to ericchiang/go-oidc that referenced this issue Jan 4, 2025
This PR adds JSON tags to allow parsing a ProviderConfig directly from
the OpenID Connect JSON metadata document. Since this is the preferred
workaround for providers that don't support discovery in a
spec-compliant way, such as returning the wrong issuer, or requiring a
URL parameter, make this path easier and add an example to the godoc.

Updates coreos#445
Updates coreos#444
Updates coreos#439
Updates coreos#442
Updates coreos#344
Fixes coreos#290
ericchiang added a commit to ericchiang/go-oidc that referenced this issue Jan 4, 2025
This PR adds JSON tags to allow parsing a ProviderConfig directly from
the OpenID Connect JSON metadata document. Since this is the preferred
workaround for providers that don't support discovery in a
spec-compliant way, such as returning the wrong issuer, or requiring a
URL parameter, make this path easier and add an example to the godoc.

Updates coreos#445
Updates coreos#444
Updates coreos#439
Updates coreos#442
Updates coreos#344
Fixes coreos#290
ericchiang added a commit that referenced this issue Jan 4, 2025
This PR adds JSON tags to allow parsing a ProviderConfig directly from
the OpenID Connect JSON metadata document. Since this is the preferred
workaround for providers that don't support discovery in a
spec-compliant way, such as returning the wrong issuer, or requiring a
URL parameter, make this path easier and add an example to the godoc.

Updates #445
Updates #444
Updates #439
Updates #442
Updates #344
Fixes #290
@mcarbonneaux
Copy link
Author

mcarbonneaux commented Jan 20, 2025

Your merge #446 looks functional!

But why add the json tags to ProviderConfig instead of making providerJSON public or using ProviderConfig instead of providerJSON?

Or add a NewProviderConfig function that would use the existing NewProvider code to do the discovery and have NewProvider use this function?!

This way we could use it to do the discovery and possibly modify the endpoints returned in the ProviderConfig structure before using it to create the Provider with NewProvider.

func NewProviderConfig(ctx context.Context, issuer string) (*ProviderConfig, error) {
   // the old NewProvider code

   // but return ProviderConfig rather than Provider
   return &ProviderConfig {
     IssuerURL: issuerURL,
     AuthURL: p.AuthURL,
     TokenURL: p.TokenURL,
     DeviceAuthURL: p.DeviceAuthURL,
     UserInfoURL: p.UserInfoURL,
     JWKSURL: p.JWKSURL,
     Algorithms: algs,
     // with the possibility of adding rawClaims in ProviderConfig
     // rawClaims: body,
     client: getClient(ctx),
   }, nil
}

@ericchiang
Copy link
Collaborator

Hey @mcarbonneaux,

Off spec providers have many different ways they supply users these values. You're describing one way for one provider, but I don't think this package needs to make a convenient API for every bespoke implementation.

If there's anything you can't do with the current API, please do open an issue and we can work through that.

If you find the current API clunky, please open a bug against your provider to correctly implement the discovery spec. I'm not trying to make this too inconvenient for you, but server implementations need to bear some responsibility here. This can't be all on the clients.

Eric

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

3 participants