coreos / go-oidc

A Go OpenID Connect client.
Apache License 2.0
1.92k stars 393 forks source link

Distributed claims utilities #171

Open ericchiang opened 6 years ago

ericchiang commented 6 years ago

http://openid.net/specs/openid-connect-core-1_0.html#AggregatedDistributedClaims

ref https://github.com/kubernetes/kubernetes/pull/63213

Consider adding some sort of distributed claim resolver. Maybe another field on the IDToken?

type IDToken struct { }

// Stays the same, doesn't do any resolution
func (t *IDToken) Claims(into interface{}) error {}

// If the named claim is a distributed claim, this will be resolved and unmarshalled into "v"
func (t *IDToken) Claim(name string, v interface{}) error {}

Claim expansion could be done when calling IDToken.Claim or could be built into IDTokenVerifier.Verify

cc @filmil for any thoughts here

ericchiang commented 6 years ago

cc @sean-q-sun

filmil commented 6 years ago

Hi folks. I was away from keyboard for a few days, hence the late response.

This certainly seems like an useful addition to have out of the box.

A few questions come to mind:

Hope this helps, even if belated, as you've already started working on it.

Cheers, F

ericchiang commented 6 years ago

Hi folks. I was away from keyboard for a few days, hence the late response.

Hope you enjoyed your time off!

What is the roll-out strategy for the distributed claims? Should they become available to the users out of the box? (I mean, what is the incentive to start using it if it requires a code change?)

The proposed change is to keep all the existing behavior the same, but introduce a new method Claim to the IDToken struct:

// Claim unmarshals the value of the named claim into v.
//
//    var email string
//    if err := idToken.Claim("email", &email); err != nil {
//        // handle error
//    }
//
// If the named claims is a distributed claim, Claim queries the remote endpoint to
// resolve the claim value.
//
// See: https://openid.net/specs/openid-connect-core-1_0.html#DistributedExample
func (i *IDToken) Claim(name string, v interface{}) error

We'd also add something to the verifier config:

type Config struct {
    // Existing fields...

    // GetVerifier, if not nil, is called when attempting to verify JWTs returned by a
    // distributed claim. If GetVerifier returns an error, the JWT is rejected.
    //
    // If nil, the verifier will be initialized using HTTP discovery and passed the same
    // context and config as the existing verifier.
    GetVerifier func(issuerURL string) (*IDTokenVerifier, error)
}

Would we also want to support aggregated claims on the library level? If yes, is there a benefit to keeping the structure of the claim somehow visible (e.g. if there are conflicting claims, do we expose all of them and how)

I'd like to hold off on aggregated claims until we have a use case. The existing Claims method will also continue to unmarshal the unmodified claims payload, so users still have the flexibility to do custom stuff.

I think that the OIDC spec is somewhat unclear about the structure of the distributed claim response. E.g. it seems that while the response to the claim must be a JWT, it says nothing about whether it may be wrapped into something.

Yeah... I guess we'll start with the strictest verification then loosen our requirements as someone asks for it.

universam1 commented 2 years ago

Yeah... I guess we'll start with the strictest verification then loosen our requirements as someone asks for it.

Hi - we are facing the distributed claim with Azure AD -> https://github.com/zalando/skipper/issues/1955

The map distributedClaims would be very handy for further Graph calls, however it is not exported. Any other access available? Thanks!