zitadel / oidc

Easy to use OpenID Connect client and server library written for Go and certified by the OpenID Foundation
https://zitadel.com
Apache License 2.0
1.41k stars 148 forks source link

AuthRequestByID Missing field #162

Closed xslasd closed 1 year ago

xslasd commented 2 years ago

Is your feature request related to a problem? Please describe. After the user logs in, call back /authorize/callback?id=。 At this time, the ID is authrequestid. I can't know the logged in user ID. Therefore, the relationship between the logged in user ID and authrequestid needs to be implemented by yourself. This may be a problem

Describe the solution you'd like In / authorize / callback? Id = add a parameter to the interface, such as / authorize / callback? Id = & uid =, where uid is the currently logged in user ID. At the same time, the interface authrequestbyid (context. Context, string) (authrequest, error) adds the parameter authrequestbyid (context. Context, ID, uid, string) (authrequest, error)

fforootd commented 2 years ago

Hi @xslasd can you explain why do you need the userid at the callback endpoint?

If we open the call to return the userid we open up risks of account impersonation.

IMO it is better to use the information returned within the id_tokens sub field after exchanging the code.

@livio-a will create improved examples soon, maybe that helps to ease this problem as well 😁

xslasd commented 2 years ago

Every request will be 302 to the loginurl. After logging in, it will be 302 to / authorize / callback? id= 。 How to bind the login user ID and request ID at this time?

livio-a commented 2 years ago

Hey @xslasd

I've just pushed the improved example implementation of the OpenID Provider: https://github.com/caos/oidc/pull/165

Although it's not completely, you should get an impression on how to implement it. It's mainly the storage interface and some login UI. The scope of the login UI depends on your needs and use case. The example shows a really basic password check without any 2FA.

A successful check then sets the userID in the Auth Request: https://github.com/caos/oidc/blob/d91fe7aacf067c6e6d61a8a0ac37302cc4e813fd/example/server/internal/storage.go#L58-L79

So on callback the userID will be already be set. If the login UI also provides some sort of 2FA/MFA checks, then you could do this in the same way as for the password check and set some state/boolean in the auth request and implement the ACR / AMR methods of Auth Request Interface : https://github.com/caos/oidc/blob/d91fe7aacf067c6e6d61a8a0ac37302cc4e813fd/example/server/internal/oidc.go#L46-L56

I hope this brings some clarity.

BTW another release yesterday added a function to build the url for redirecting back from the login UI. This can also be seen in the example: https://github.com/caos/oidc/blob/d91fe7aacf067c6e6d61a8a0ac37302cc4e813fd/example/server/op.go#L60

fforootd commented 2 years ago

How to bind the login user ID and request ID at this time?

If you want to handle a state while redirecting you can rely on the state param.

So normally it works like this:

  1. The client redirect to the IdP and provides a state (among other things)
  2. The user authenticates against the IdP
  3. The IdP returns the auth code and the state (among other things)
  4. The client uses the auth code to get the id_token and access_token
  5. The client reads the userid from the sub claim in the id_token

Hope this helps.

xslasd commented 2 years ago

Hey @livio-a I'm glad to hear from you! I understand your use process. It should be that I didn't express it clearly. My usage scenario is to separate IDP and identity authentication services, which leads to the problem. I want to use it like ory / Hydra, but I don't want to use ory / Hydra. It's a little heavy.

xslasd commented 2 years ago

Therefore, during my use, I encountered some obstacles in the callback "/ authorize / callback? Id =" and endsession. Sign in

  1. The client redirects to IDP for authentication.
  2. IDP redirects to the authentication service, issues the request ID, and logs in the user information.
  3. The authentication service provides login information and request ID for binding. (my backend code implementation)
  4. Call / authorize / callback? Id = create code and redirect to client. Log out
  5. Redirect the client to endsession interface.
  6. IDP calls terminatesession to delete the token.
  7. Because there is no way to redirect to the authentication service, delete the login status. I can only give up post logout redirect URI, post logout redirect Set the URI to the logout of the authentication service.
fforootd commented 2 years ago

My usage scenario is to separate IDP and identity authentication services, which leads to the problem. I want to use it like ory / Hydra, but I don't want to use ory / Hydra. It's a little heavy.

Ok, so you want to separate the authentication protocol from the users authentication right? So you only want this library to handle the OpenID Connect part but the actual authentication should be happening somewhere else.

Do I understand this correctly?

xslasd commented 2 years ago

yes. At the same time, I found that verifyidtoken is not implemented, and the naming in the storage interface method is not standardized.

xslasd commented 2 years ago

I modified your code and it works.

func VerifyIDToken(ctx context.Context, token string, v IDTokenVerifier) (oidc.IDTokenClaims, error) {
    claims := oidc.EmptyIDTokenClaims()
    decrypted, err :=jose.ParseSigned(token)
    if err != nil {
        return nil, err
    }
    payload,err:=v.KeySet().VerifySignature(ctx,decrypted)
    if err!=nil{
        return nil, err
    }
    err = json.Unmarshal(payload, claims)
    if err!=nil{
        return nil, err
    }
fforootd commented 2 years ago

yes. At the same time, I found that verifyidtoken is not implemented, and the naming in the storage interface method is not standardized.

hm @livio-a this looks odd, whats your take on this?

I regard to the separation of authN and OIDC. Thats already done by this lib by relying on an internal state. So you only need to implement your login as stated in @livio-a example above

livio-a commented 2 years ago

yes. At the same time, I found that verifyidtoken is not implemented, and the naming in the storage interface method is not standardized.

hm @livio-a this looks odd, whats your take on this?

the code above seems to only check the signature of the id_token. There are not checks for the issuer, audience, expiration and so on. I do not recommend using this, especially not in production! The provided VerifyIDToken function of the rp package is implemented according to the ID Token Validation of the OIDC Core 1.0 spec (https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation). It has also passed the OIDC RP Certification.

hifabienne commented 1 year ago

We close this, because it looks like its done. Please feel free to reopen if something is missing.