`jwk.SetGlobalFetcher` requires object implementing interface with unexported methods about jwx HOT 19 CLOSED

I think I understand, but can you show me your code? I want to make sure I'm on the same wavelength as you.

from jwx.

Main when setting up everything and file where I

func main() {
	ctx := utilities.OnShutDown()
	jwk.SetGlobalFetcher(credential.NewOIDCFetcher(ctx))

	service := &credentialManagementService{ jwkCache: jwk.NewCache(ctx) }
	// ...
}

// ... different file 

func (x *credentialManagementService) fetchKeys(ctx context.Context, issuer string) (jwk.Set, error) {
	return x.jwkCache.Get(ctx, issuer)
}

And finally the OIDCFetcher, which is just copy-paste your code with renaming, TODOs and commenting out options.
See comments with ISSUE where I describe different approaches and different issues.

package credential

import (
	"context"
	"fmt"
	"net/http"
	"sync"

	"github.com/lestrrat-go/httprc"
)

type fetchRequest struct {
	mu sync.RWMutex

	// client contains the HTTP Client that can be used to make a
	// request. By setting a custom *http.Client, you can for example
	// provide a custom http.Transport
	//
	// If not specified, http.DefaultClient will be used.
	client httprc.HTTPClient

	wl httprc.Whitelist

	// u contains the URL to be fetched
	url string

	// reply is a field that is only used by the internals of the fetcher
	// it is used to return the result of fetching
	reply chan *fetchResult
}

type fetchResult struct {
	mu  sync.RWMutex
	res *http.Response
	err error
}

func (fr *fetchResult) reply(ctx context.Context, reply chan *fetchResult) error {
	select {
	case <-ctx.Done():
		return ctx.Err()
	case reply <- fr:
	}

	close(reply)
	return nil
}

type fetcher struct {
	requests chan *fetchRequest
}

// ISSUE: if I use this interface, then I get
// cannot use credential.NewOIDCFetcher(ctx) (value of type credential.Fetcher) as httprc.Fetcher value in argument to jwk.SetGlobalFetcher: credential.Fetcher does not implement httprc.Fetcher (missing method fetch)
// 
// type Fetcher interface {
// 	Fetch(context.Context, string, ...httprc.FetchOption) (*http.Response, error)
// 	fetch(context.Context, *fetchRequest) (*http.Response, error)
// }

func NewOIDCFetcher(ctx context.Context, options ...httprc.FetcherOption) httprc.Fetcher {
	var nworkers int
	var wl httprc.Whitelist
	// for _, option := range options {
	// 	//nolint:forcetypeassert
	// 	switch option.Ident() {
	// 	case identFetcherWorkerCount{}:
	// 		nworkers = option.Value().(int)
	// 	case identWhitelist{}:
	// 		wl = option.Value().(Whitelist)
	// 	}
	// }

	if nworkers < 1 {
		nworkers = 3
	}

	incoming := make(chan *fetchRequest)
	for i := 0; i < nworkers; i++ {
		go runFetchWorker(ctx, incoming, wl)
	}

	// ISSUE: My onw implementation of fetcher doesn't fullfil httprc.Fetcher
	// 
	// cannot use &fetcher{…} (value of type *fetcher) as httprc.Fetcher value in return statement: *fetcher does not implement httprc.Fetcher (missing method fetch)
	//	have Fetch("context".Context, string, ...httprc.FetchOption) (*"net/http".Response, error)
	//	want fetch("context".Context, *httprc.fetchRequest) (*"net/http".Response, error)
	return &fetcher{
		requests: incoming,
	}
}

func (f *fetcher) Fetch(ctx context.Context, u string, options ...httprc.FetchOption) (*http.Response, error) {
	var client httprc.HTTPClient
	var wl httprc.Whitelist
	// for _, option := range options {
	// 	//nolint:forcetypeassert
	// 	switch option.Ident() {
	// 	case identHTTPClient{}:
	// 		client = option.Value().(httprc.HTTPClient)
	// 	case identWhitelist{}:
	// 		wl = option.Value().(httprc.Whitelist)
	// 	}
	// }

	req := fetchRequest{
		client: client,
		url:    u,
		wl:     wl,
	}

	return f.fetch(ctx, &req)
}

// fetch (unexported) is the main fetching implemntation.
// it allows the caller to reuse the same *fetchRequest object
func (f *fetcher) fetch(ctx context.Context, req *fetchRequest) (*http.Response, error) {
	reply := make(chan *fetchResult, 1)
	req.mu.Lock()
	req.reply = reply
	req.mu.Unlock()

	// Send a request to the backend
	select {
	case <-ctx.Done():
		return nil, ctx.Err()
	case f.requests <- req:
	}

	// wait until we get a reply
	select {
	case <-ctx.Done():
		return nil, ctx.Err()
	case fr := <-reply:
		fr.mu.RLock()
		res := fr.res
		err := fr.err
		fr.mu.RUnlock()
		return res, err
	}
}

func runFetchWorker(ctx context.Context, incoming chan *fetchRequest, wl httprc.Whitelist) {
LOOP:
	for {
		select {
		case <-ctx.Done():
			break LOOP
		case req := <-incoming:
			req.mu.RLock()
			reply := req.reply
			client := req.client
			if client == nil {
				client = http.DefaultClient
			}
			url := req.url
			reqwl := req.wl
			req.mu.RUnlock()

			var wls []httprc.Whitelist
			for _, v := range []httprc.Whitelist{wl, reqwl} {
				if v != nil {
					wls = append(wls, v)
				}
			}

			if len(wls) > 0 {
				for _, wl := range wls {
					if !wl.IsAllowed(url) {
						r := &fetchResult{
							err: fmt.Errorf(`fetching url %q rejected by whitelist`, url),
						}
						if err := r.reply(ctx, reply); err != nil {
							break LOOP
						}
						continue LOOP
					}
				}
			}

			// TODO: handle OIDC configuration endpoint here

			// The body is handled by the consumer of the fetcher
			//nolint:bodyclose
			res, err := client.Get(url)
			r := &fetchResult{
				res: res,
				err: err,
			}
			if err := r.reply(ctx, reply); err != nil {
				break LOOP
			}
		}
	}
}

from jwx.

Thank you.

So two things:

First, I'm not sure what kind of usage would require jwk.Cache to do the processing of issuer -> jwks_uri. I mean, jwk.Cache was written for the express purpose to be used for JWE and JWS operations, and I'm not aware of a situation where this processing is required. So I'm just wondering what you are using this for...

If you are using jwk.Cache for something outside of the scope of JWE/JWS, I don't recommend you using it (especially because of the next item below). If you are using it within the JWE/JWS process, I would like to know what/when exactly this processing is required.

Second, assuming that there's some legitimate reason to do the issuer -> jwks_uri thing within JWE/JWS, I see that there exist a design problem. It's both an oversight and a by-design type of problem.

• httprc.Fetcher is designed to be tightly coupled with its asynchronous queue which is running in the background, and requires more than just the Fetch() API to work.
• jwk.SetGlobalFetcher did not take into account using anything other than httprc.Fetcher, because it was designed only as an escape hatch to allow proper destruction of the worker goroutines.

If I were to fix this, I sense an API change is necessary to correctly fix it -- and thus it may take more time than you may like.

from jwx.

We need to introspect 3rd party JWT. We offer our customers to specify, if they want Online verification (different story) or Offline verification. For offline, here are the steps I need to perform

1. jwt.Parse(token, jwt.WithRequiredClaim(jwt.IssuerKey), jwt.WithVerify(false))
2. Get iss claim from received token. Let's say it is Google token which is in the request. iss will be https://accounts.google.com
3. Based on OIDC specification I know, that there must be URL iss+"/.well-known/openid-configuration", meaning https://accounts.google.com/.well-known/openid-configuration
4. Returned JSON contains top-level key "jwks_uri" which points to https://www.googleapis.com/oauth2/v3/certs
5. That URL I can pass to jwk.Fetch()

This works well and then I can do jws.Verify(token, jws.WithKeySet(keySet)) where keySet I received in step 5.

I would like to enclose step 2-5 with the cache, so I don't reach target IDP every time to fetch keys.
So if I take the copy-pasted code above, there would be just this modification.

			// TODO: handle OIDC configuration endpoint here
+			res, err := client.Get(url + "/.well-known/openid-configuration")
+			if err != nil {
+				return nil, err
+			}
+			type openidConfiguration struct {
+				JWKsUri string `json:"jwks_uri"`
+			}
+			
+			oidcConf := openidConfiguration{}
+			err = json.NewDecoder(resp.Body).Decode(&oidcConf)
+			if err != nil {
+				return nil, err
+			}
+
			// The body is handled by the consumer of the fetcher
			//nolint:bodyclose
+			res, err := client.Get(oidcConf.JWKsUri)
-			res, err := client.Get(url)
			r := &fetchResult{
				res: res,
				err: err,
			}
			if err := r.reply(ctx, reply); err != nil {
				break LOOP
			}

from jwx.

Hmmmm. So if you're skipping verification...

# pseudocode

# somewhere, in a global or higher layer
cache := jwk.NewCache(...)

token, _ := jwt.ParseInsecure(payload)
iss := token.Issuer()

Then you can do the OIDC well known endpoint thingie OUTSIDE of the jwk.Cache..., no?

# pseudocode
// do the well known endpoint thingie
jwksURI := ...


if !cache.IsRegistered(jwksURI) {
   cache.Register(jwksURI, ...)
   cache.Refresh(jwksURI, ...)
}

set, _ := cache.Get(jwksURI)

jws.Verify(token, jws.WithKeySet(set))

My point is that if you need to get into the guts of jwk.Cache, that means the issuer -> cert URL thing would have to be an integral part of the verification process that can't be skipped/done in a different order, but this case doesn't seem like it, no? Or am I missing something?

from jwx.

I'm not sure I fully understand the last paragraph.
You are suggesting ParseInsecure() to get iss. Do OIDC thingie which will give me jwks_uri and then use that in Cache.

But I don't want to cache only JWKs. But I want to avoid going to .well-know/openid-configuration too.
So it means that I need to implement another cache for mapping iss to jwks_uri.

With my own fetcher and If everything is cached, I get iss (https://accounts.google.com/) and call cache.Get(iss) only. And I would get the keys.

Meaning, based on issuer I can get keys (based on OIDC specification). If cache is empty, it would require to do 2 HTTP requests. But if it is cached, no HTTP request would happen.

from jwx.

Oh, I get it. you want the jwks_uri -> cert path included in the cache. Sorry, that didn't register to me.

Well, then I guess I'm back to this

If I were to fix this, I sense an API change is necessary to correctly fix it -- and thus it may take more time than you may like.

TBH I don't know ATM if this is a quick fix or something that I need to dive deeper and possibly punt until v3, sorry.

from jwx.

quick memo:

• I could use jwk.Fetcher instead of httprc.Fetcher, but the proble lies in the signatures for each Fetch() (i.e the option types)
  • Also, jwk.Fetcher vs httprc.Fetcher is confusing. This needs to be cleaned up
• If the API/signature for SetGlobalFetcher changes, I need to weigh in the backwards compatibility. Did I mark this experimental? (Update: no, I didn't. Bummer)

from jwx.

Oh, I get it. you want the jwks_uri -> cert path included in the cache.

The cache doesn't need to know that directly, but in general yes. In other words, the key of cache (url) isn't directly the URL where keys are stored. But intermediate HTTP request is required to get the final url, where keys are located.

TBH I don't know ATM if this is a quick fix or something that I need to dive deeper and possibly punt until v3, sorry.

Got it, no problem. Just for me it means, I need to implement another way. I can use jwk.Fetch but probably not jwk.Cache.

from jwx.

Okay, I'm about to go to bed, here's my diagnosis as of now:

• SetGlobalFetcher needs to take in jwk.Fetcher
• jwk.Fetcher needs to be changed to something that can act as an adapter to httprc.Fetcher
• Current uses of jwk.Fetcher needs to be tweaked so that it can leverage httprc.Fetcher (via jwk.Fetcher)
• ...Thus, this requires an API change, which would definitely be something that needs to go to v3.

The good news is that I'd been working on v3 for a while already, and I've written most of what I needed other than this feature.
The bad news is that this needs to go on v3, and there's no way I'd be able to finish writing it up quickly.

I know this doesn't change your situation, but JFYI.
Sorry for the inconvenience due to my oversight on the design.

from jwx.

@arxeiss Just an update.

I created a partial fix in https://github.com/lestrrat-go/jwx/tree/gh-1047. It changes the type that SetGlobalFetcher accepts to jwk.Fetcher, which 1) doesn't have hidden methods to implement, and 2) decouples this module with its dependencies more. But I also think the problem lies in the design of httprc as well, so I'm not sure if I would merge this as is (without creating a v2 for httprc) yet.

from jwx.

Hello, thank you for your tries and ongoing support. Actually I was happy with that for a while. But then I spot another issue.

I was able to write own Fetcher, so performing jwk.Fetch("https://accounts.google.com") actually worked (fetched well-know endpoint and then the jwks_uri path). But then I tried to implement it with jwk.Cache and that has another issue. Becuase Cache ignores jwk.SetGlobalFetcher() and creates own instance of Fetcher.

My bad I didn't saw that before. I was so focused on the Fetcher, that I didn't dig deeper.

from jwx.

@arxeiss yes, that was my next issue. I think when jwk.Cache is created, I should look at the global fetcher, and that's the next API incompatibility -- now in httprc -- that I'm going to think about

from jwx.

@arxeiss Okay, I did more digging, and here's my final verdict:

SetGlobalFetcher must be fixed, and it will be. it will accept jwk.Fetcher instead, as done in that branch above.

jwk.Cache -- or, more like httprc.Cache is NOT meant to be customizable. It gives the false sense that it could be because of the silly artifact in the design that exposes httprc.Fetcher, but this is very wrong. It should never have been exposed to begin with, because the implementation is way too tightly coupled with the backend queue.

What I could do in the future version (v3) is to make jwk.Cache an interface. This way you could do whatever you want as long as the interface is the same.

from jwx.

I also need to customize a fetcher so I can dump the response body when hitting parsing errors. I have 2 questions when adopting this branch:

1. Why the jwk.Fetcher has different Fetch signature other with httprc.Fetcher?

type jwk.Fetcher interface {
	Fetch(context.Context, string, ...FetchOption) (Set, error) // Return jwk.Set
}

type httprc.Fetcher interface {
	Fetch(context.Context, string, ...FetchOption) (*http.Response, error)  // Return http.Response
	fetch(context.Context, *fetchRequest) (*http.Response, error)
}

If I want to implement my own fetcher, I need to parse the response body inside the fetcher? Why not let both Fetch function return http.Response?

2. Builtin options like FetchOption is not usable in external fetcher implementation, as the Ident() returns internal structure which cannot be used externally, if I am implementing the fetcher should I ignore the options now? Are you planning to make the options visible and customizable externally? Thanks.

from jwx.

@koote

1. Because they were developed at different times for completely different purposes. jwk.SetGlobalFetcher was a stop gap API that in hindsight should not have been introduced, at least in this format (see #928). The details of their implementations are not relevant for discussion for v2, because their APIs won't change to keep backwards compatibility. They are still game for v3.

2. These are options were not meant to be used by external users when they were introduced. While I'm open to a better API, I am not interested in making them closure based, if that's what you are asking. I think it'd be better if I provided public functions that return their respective Identity.

func IdentBlah() interface {
   return identBlah{}
}

from jwx.

@koote I was just thinking about how to expand the options, but I think the easiest way to compare them outside of the jwx package is just to:

var identOptionA = WithJwxOptionA().Ident()
var identOptionB = WithJwxOptionB().Ident()
func myFun(options ... JwxOption) {
   for _, option := range options { 
       switch option.Ident() {
       case identOptionA:
       csae identOptionB:
   }
}

from jwx.

reference: #1065. uses a different httprc version, still work in progress

from jwx.

For the time being, I consider #1071 to be complete. in v3 there will be no global fetcher, and thus this issue will not be a problem. fetching behavior should be customized using HTTPClient interface, which just abstracts away (http.Client).Do

I'm going to close this when #1071 is merged to v3, but feel free to reopen or open a new issue if this still is problematic.

from jwx.