-
-
Notifications
You must be signed in to change notification settings - Fork 162
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
jwk.SetGlobalFetcher
requires object implementing interface with unexported methods
#1047
Comments
I think I understand, but can you show me your code? I want to make sure I'm on the same wavelength as you. |
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 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
}
}
}
} |
Thank you. So two things: First, I'm not sure what kind of usage would require If you are using 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.
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. |
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
This works well and then I can do I would like to enclose step 2-5 with the cache, so I don't reach target IDP every time to fetch keys. // 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
} |
Hmmmm. So if you're skipping verification...
Then you can do the OIDC well known endpoint thingie OUTSIDE of the jwk.Cache..., no?
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? |
I'm not sure I fully understand the last paragraph. But I don't want to cache only JWKs. But I want to avoid going to With my own fetcher and If everything is cached, I get 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. |
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
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. |
quick memo:
|
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.
Got it, no problem. Just for me it means, I need to implement another way. I can use |
Okay, I'm about to go to bed, here's my diagnosis as of now:
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. I know this doesn't change your situation, but JFYI. |
@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. |
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 My bad I didn't saw that before. I was so focused on the Fetcher, that I didn't dig deeper. |
@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 |
@arxeiss Okay, I did more digging, and here's my final verdict:
What I could do in the future version (v3) is to make |
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:
If I want to implement my own fetcher, I need to parse the response body inside the fetcher? Why not let both
|
func IdentBlah() interface {
return identBlah{}
} |
@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:
}
} |
reference: #1065. uses a different httprc version, still work in progress |
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 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. |
Describe the bug
I would like to use
jwk.SetGlobalFetcher()
and pass my own fetcher. The way I did that was to copy-paste https://github.com/lestrrat-go/httprc/blob/main/fetcher.go file and started to modify it. But I was facing multiple issues:option.Ident()
ofFetcherOption
are unexported structs likeidentFetcherWorkerCount
oridentWhitelist
so I wasn't able to easily fetch the value. However I can calloption.Value
which is exported and with switch type to get what I want.jwk.SetGlobalFetcher(fetcher)
it failed withTo Reproduce / Expected behavior
myOwnFetcher := yourpkg.NewFetcher(context.Background())
jwk.SetGlobalFetcher(myOwnFetcher)
Additional context
The reason for that is I want to use
jwk.Cache
, but the key wouldn't be URL of certs (like https://www.googleapis.com/oauth2/v3/certs) but URL of issuer (like https://accounts.google.com/).So what I need to do in my own fetcher is
jwks_uri
key.jwks_uri
and continue in original code.So if later I will have only issuer, I would be still able to use
cache.Get(issuerUrl)
and getjwk.Set
The text was updated successfully, but these errors were encountered: