dexidp / dex

OpenID Connect (OIDC) identity and OAuth 2.0 provider with pluggable connectors
https://dexidp.io
Apache License 2.0
9.48k stars 1.7k forks source link

Verification claims in payload #378

Closed waterdudu closed 8 years ago

waterdudu commented 8 years ago

I have a question on client claims verification. Claims are generated by the following function by passing UserID as sub and ClientID as aud.

func (s *Session) Claims(issuerURL string) jose.Claims {
    claims := oidc.NewClaims(issuerURL, s.UserID, s.ClientID, s.CreatedAt, s.ExpiresAt)
    if s.Nonce != "" {
        claims["nonce"] = s.Nonce
    }
    return claims
}

According to oidc specification, aud must match the client_id in the verification process. But in the signature of NewClaims function(seeing above), ClientID is passed as the aud in NewClaims function, which is not an array. In verification.go, L120-L124, client claims verification will fail if aud is not equal to sub.

if aud, ok, err := claims.StringClaim("aud"); err == nil && ok {
        if aud != sub {
            return "", fmt.Errorf("invalid claims, 'aud' claim and 'sub' claim do not match, aud=%s, sub=%s", aud, sub)
        }
    }

I passed a string slice to the third parameter of Session.Claims function and the client claims verification passed.

auds := []string{s.UserID, s.ClientID}
// claims := oidc.NewClaims(issuerURL, s.UserID, s.ClientID, s.CreatedAt, s.ExpiresAt)
claims := oidc.NewClaims(issuerURL, s.UserID, auds, s.CreatedAt, s.ExpiresAt)

Did I verify client claims in the right way? If not, could you show me more details on how to do it the right way?

Thanks.

ericchiang commented 8 years ago

The audience is allowed to be an array of strings

REQUIRED. Audience(s) that this ID Token is intended for. It MUST contain the OAuth 2.0 client_id of the Relying Party as an audience value. It MAY also contain identifiers for other audiences. In the general case, the aud value is an array of case sensitive strings. In the common special case when there is one audience, the aud value MAY be a single case sensitive string.

https://openid.net/specs/openid-connect-core-1_0.html#IDToken

However, since the aud claim determines what client_id can use the token, if an OpenID Connect provider is minting you ID Tokens for a different client, that's a critical bug on the server side.

Does that answer your question?

waterdudu commented 8 years ago

Thanks for you explain, but my question is the process of verification. Why you check the equality of aud and sub?

if aud, ok, err := claims.StringClaim("aud"); err == nil && ok {
        if aud != sub {
            return "", fmt.Errorf("invalid claims, 'aud' claim and 'sub' claim do not match, aud=%s, sub=%s", aud, sub)
        }
    }

I can't find any clue in oidc specification talking about aud != sub check. Thanks.

rsoletob commented 8 years ago

Hi @waterdudu, this check is used to verify the client when token is issued using client credentials authorization flow, where sub and aud would be the same.

ericchiang commented 8 years ago

Ah. Yes, VerifyClaims is used to verify the ID Token returned with the oauth2 token, VerifyClientClaims is used to verify the client authorization sent to the provider during the token exchange.

See the spec here: https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication

VerifyClientClaims seems like an odd thing for us to expose in go-oidc.

waterdudu commented 8 years ago

@ericchiang @rsoletob Thanks, that's very helpful!