Skip to content
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

Questions about the example #144

Closed
TwiN opened this issue Dec 6, 2021 · 2 comments
Closed

Questions about the example #144

TwiN opened this issue Dec 6, 2021 · 2 comments
Labels
question Further information is requested

Comments

@TwiN
Copy link

TwiN commented Dec 6, 2021

Hey there,

My knowledge when it comes to hosting an OIDC provider is fairly limited, and I have a couple of questions I hope you'll be able to answer.

For the sake of verbosity, I plan on integrating the server implementation of this library in an existing application. It will therefore live alongside multiple other handlers. Furthermore, I'm only interested in the app client implementation, though I'm not 100% sure what flow each of the folder represent.

  1. Why is there a /healthz handler registered by the server example? Is it because the server is meant to be run as a standalone application (albeit with some modifications to support endpoints like userinfo, etc.)

  2. What exactly does the t variable represent here?:
    https://github.com/caos/oidc/blob/eb10752e485ced36bd996bcb290c6e617f5ea449/example/internal/mock/storage.go#L118-L122

  3. Why is this function called HandleCallback? Shouldn't the callback technically be on the client side? From what I understand, it would be more appropriate to name it something like HandleLoginAttempt?
    https://github.com/caos/oidc/blob/eb10752e485ced36bd996bcb290c6e617f5ea449/example/server/default/default.go#L68

  4. If the statement above is accurate, then I think the handler should also be in charge of verifying the credentials (username/password) of the user. If so, how does one convey that the verification of the credentials was successful and convey that data? From what I see, it's just redirecting to another path
    https://github.com/caos/oidc/blob/eb10752e485ced36bd996bcb290c6e617f5ea449/example/server/default/default.go#L71

  5. Still in the same function/handler above, assuming we swap the input client for username and add an extra input password in the HandleLogin handler, we would get something like:

func HandleCallback(w http.ResponseWriter, r *http.Request) {
  r.ParseForm()
  username := r.FormValue("username")
  password := r.FormValue("password")
  isUsernameAndPasswordValid := DoSomethingToValidateCredentialsHere(username, password)
  if !isUsernameAndPasswordValid {
    http.Error(w, "invalid username or password", http.StatusUnauthorized)
    return
  }
  // At this point, we've verified that the credentials are valid, but we have to return an id..?
  http.Redirect(w, r, "/authorize/callback?id="+id, http.StatusFound)
}

instead of
https://github.com/caos/oidc/blob/eb10752e485ced36bd996bcb290c6e617f5ea449/example/server/default/default.go#L68-L71
which prompts the question, is the id passed as query parameter in the redirect function expected to be some kind of session id?

I think a diagram -- not just of a typical IODC provider, but of the exact endpoints used in each client example -- would greatly help people in understanding how it works. Normally, I'd offer to do it, alas, like I said, I'm not really knowledgeable when it comes to the implementation of an IODC provider 😅 If I end up getting the gist of it, I'll definitely try to help out with the documentation though.

@fforootd fforootd added the question Further information is requested label Dec 6, 2021
@livio-a
Copy link
Member

livio-a commented Dec 8, 2021

Hi @TwiN

Thanks for these great questions. I'll do my best to answer them and sorry for the late response.

First of all, it certainly needs some more documentation and explanations about the structure and the examples, especially the server one. I like the idea of a small diagram to help people understand. I'll try to give you short overview here and will update the documentation and examples as soon as possible (#116). Especially the server example needs some extra dedication. In the meantime I can already refer you to our implementation: https://github.com/caos/zitadel/tree/main/internal/api/oidc

The structure or most important packages are basically these:

/pkg
    /client     clients using the OP for retrieving, exchanging and verifying tokens       
        /rp     definition and implementation of an OIDC Relying Party (client)
        /rs     definition and implementation of an OAuth Resource Server (API)
    /op         definition and implementation of an OIDC OpenID Provider (server)
    /oidc       definitions shared by clients and server

/example
    /api        example of an api / resource server implementation using token introspection
    /app        web app / RP demonstrating authorization code flow using various authentication methods (code, PKCE, JWT profile)
    /github     example of the extended OAuth2 library, providing an HTTP client with a reuse token source
    /service    demonstration of JWT Profile Authorization Grant
    /server     http server starting OP implementation using the mock storage (internal/mock/storage.go)

So:

  1. Yes I think that was the idea, but looking at it now I'd probably won't add it anymore. If your server has its own heath / readiness endpoint, I'd instead use the Health function of the Storage interface to check for readiness.
    https://github.com/caos/oidc/blob/eb10752e485ced36bd996bcb290c6e617f5ea449/pkg/op/storage.go#L44

  2. This was obviously real bad practice and dark moment in my coding career 🤨
    (and the more I look at your questions I find more in this example)
    I used it so save state:
    https://github.com/caos/oidc/blob/eb10752e485ced36bd996bcb290c6e617f5ea449/example/internal/mock/storage.go#L128-L138
    https://github.com/caos/oidc/blob/eb10752e485ced36bd996bcb290c6e617f5ea449/example/internal/mock/storage.go#L152-L155
    3.-5. The idea is that the library will solve as much as possible of the OpenID specification the OP has to implement. As the actual authentication of the user is not part of the spec and can be done by a lot of ways (username + password, webauthn, ...).
    The library will redirect the user agent to the login UI with the id of the auth request and expects a redirect back after authentication again with the id of the auth request as query param.
    (We therefore host the authorization endpoint and login page in ZITADEL on the same domain and use cookies to share and ensure certain information.)
    It will check if the user was authenticated and create the response (redirect to the callback endpoint of the RP / client.
    https://github.com/caos/oidc/blob/eb10752e485ced36bd996bcb290c6e617f5ea449/pkg/op/auth_request.go#L387-L393

So your suggestion for HandleLoginAttempt (or bad named HandleCallback ) is correct. You'd just need something to store the successful authentication and return that when the library will check if the authReq is Done() (<-- might rename that in the future to something more meaningful, e.g. authenticated)

I hope I could clarify this a bit and give you a basic idea. It definitely showed me that we need to improve the docs and examples.
Don't hesitate to ask again if I've missed a question of if you have others 😃

@TwiN
Copy link
Author

TwiN commented Dec 9, 2021

@livio-a Thank you so much for taking the time to answer me.

You and your fellow contributors have definitely done some amazing work.

I went into this with very little knowledge on the provider side of things, but after going through https://openid.net/specs/openid-connect-core-1_0.html#CodeFlowSteps and https://www.rfc-editor.org/rfc/rfc6749.txt, I have to admit that bootstrapping an OP with so little configuration needed from the end user is quite a feat!

I may come around and contribute at some point since I'll have to implement an IODC provider eventually, alas, my hands are full with other projects short term.

In any case, thanks again!

@TwiN TwiN closed this as completed Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants