Add basic OIDC client #15267
Add basic OIDC client #15267
Conversation
github-actions
bot
added
team: SID
team: devx
team: webapp
There was a problem hiding this comment.
NIce. Left a couple of initial comments.
For all of this, unit tests are a strict must. Let's make sure we have good coverage here.
Additionally, the PR is quite large. It would benefit you, and other reviewers to make this more incremental (yes, there are seperate commits but ideally we roll out this changes incrementally, validating them incrementally in a rolled out env). You could split it into the following:
- Config for the server seperately
- Each handler separately (with unit tests)
There will be a couple of improvements we'll want to add to make it easier for us to work with HTTP. For example, we'll want a helper HandlerWrapper which takes (Request, Response) but returns an (error) such that we can make handling sub-errors a bit easier, and bit more coherent.
We should think very hard about having cross-component dependencies on the db-level. Either we should be authartive about creating the Config on the IAM component (and having the DB implementation there), or IAM should do a ListClientConfig against Public API. I believe IAM should be authoritative and should ultimately do the create/delete etc. Public API would then only delegate to this component. |
|
For the tests, I recommend looking at |
AlexTugarev
force-pushed
the
at/oidc-client
branch
from
a613e00 to
eaf6121
Compare
AlexTugarev
force-pushed
the
at/oidc-client
branch
2 times, most recently
from
3106423 to
a4a1402
Compare
AlexTugarev
force-pushed
the
at/oidc-client
branch
from
a4a1402 to
2108147
Compare
|
Ah, it auto-landed because the hold got removed.. |
| func OAuth2Middleware(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { | ||
| log.Trace("at oauth2 middleware") | ||
| ctx := r.Context() | ||
| config, ok := ctx.Value(keyOIDCClientConfig{}).(OIDCClientConfig) | ||
| if !ok { | ||
| http.Error(rw, "config not found", http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| // http-only cookie written during flow start request | ||
| stateCookie, err := r.Cookie(stateCookieName) | ||
| if err != nil { | ||
| http.Error(rw, "state cookie not found", http.StatusBadRequest) | ||
| return | ||
| } | ||
| // the starte param passed back from IdP | ||
| stateParam := r.URL.Query().Get("state") | ||
| if stateParam == "" { | ||
| http.Error(rw, "state param not found", http.StatusBadRequest) | ||
| return | ||
| } | ||
| // on mismatch, obviously there is a client side error | ||
| if stateParam != stateCookie.Value { | ||
| http.Error(rw, "state did not match", http.StatusBadRequest) | ||
| return | ||
| } | ||
|
|
||
| code := r.URL.Query().Get("code") | ||
| if code == "" { | ||
| http.Error(rw, "code param not found", http.StatusBadRequest) | ||
| return | ||
| } | ||
|
|
||
| oauth2Token, err := config.OAuth2Config.Exchange(ctx, code) | ||
| if err != nil { | ||
| http.Error(rw, "failed to exchange token: "+err.Error(), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| ctx = context.WithValue(ctx, keyOAuth2Result{}, OAuth2Result{ | ||
| OAuth2Token: oauth2Token, | ||
| }) | ||
| next.ServeHTTP(rw, r.WithContext(ctx)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
I'd strongly recommend not to over-use middlewares which extract concrete data. Here's why:
- Golang's context is not the same as the
express.Requestcontext. The context in go is used for downstream calls irrespective of whether it's an http handler or not. Using the context to pass request specific arguments results in carry extra baggage. - The middleware forces you write/and parse the data out of the context. Because context is not type safe, you incur extra overhead in extracting the data, but also risk cases where the context does not parse correctly.
- It forces you to handle the existence of the data twice, once on the middleware here, and another when you pull the value from the context
- Middlewares tend to make sense, once you have general purpose logic you want to re-use across handlers. Here, we only have a single handler. This causes a couple of problems:
- Testing the handler becomes harder, you always have to test it together with the middleware
- Readability becomes harder, the reader now has to step a layer above and figure out what's populating the context
- It makes handling errors from context values much harder. The context of why the value is not present is missing. Yes, the middleware should return an error but we need to be defensive and also guard this in the handler.
There was a problem hiding this comment.
middlewares which extract concrete data
The point in using middleware here is to be made clear: this is to separate protocol implementations. First there is the OAuth2 layer, then the OIDC AuthN validation. It seems to be an acceptable cost if you consider that the OAuth2 layer can be reused for Git Auth later with a different handler.
The point on configuration and lax contract, indeed that's key-value onion might be fragile and requires complete coverage and precondition checks. I'll make improvements to get the coverage needed.
Golang's context is not the same as the express.Request context.
Do you have examples, or docs for that? I read different and saw different examples, see https://github.com/go-chi/chi and comparable, they advertising it as such. Could you name concrete problems with that?
I see the usage of middleware is justified here, especially for the separation of two protocols, where one is an extension to the other. The outcome of the first part can/must be checked and verified. To test the first part separately, one would need to slice it anyways. Same applies for the testability of the second part.
There was a problem hiding this comment.
That said, checking for a solid connection should be a must here, thus it's worth revisiting tests. 👍🏻
| var claims map[string]interface{} | ||
| err = result.IDToken.Claims(&claims) | ||
| if err != nil { | ||
| http.Error(rw, err.Error(), http.StatusInternalServerError) |
There was a problem hiding this comment.
This should be a user error, no? If the user supplies the wrong claim, we shouldn't return a Server error.
There was a problem hiding this comment.
Claims is what Gitpod (as the OIDC client) can obtain from the end user.
Not being able to parse the id_token can have these causes:
- unparseable gibberish received from IdP (e.g. their server/lb is misconfigured)
- otherwise parsing failures => status code 500 it's Gitpod's fault
Also, while the error modes are still relevant, following PRs will remove this section.
| issuerParam := r.URL.Query().Get("issuer") | ||
| if issuerParam == "" { | ||
| return nil, errors.New("issuer param not specified") | ||
| } | ||
| issuer, err := url.QueryUnescape(issuerParam) | ||
| if err != nil { | ||
| return nil, errors.New("bad issuer param") | ||
| } | ||
| log.WithField("issuer", issuer).Trace("at GetClientConfigFromRequest") | ||
|
|
||
| for _, value := range service.configsById { | ||
| if value.Issuer == issuer { | ||
| return value, nil | ||
| } | ||
| } | ||
| return nil, errors.New("failed to find OIDC config for request") |
There was a problem hiding this comment.
This needs to return more specific errors, as these need to be used to directly respond. If the caller does not specify the right arguments, it's a user error
There was a problem hiding this comment.
That's true. I'll add an issue for that.
We need to identify the relevant error modes, to get a good signal (without noise) to be able to render something meaningful:
- no params => 400
- issuer (or maybe client id) + unmatched => 404
- state (including client id) + unmatched => 404
- client config matched + unauthorized => 401
(that list is not complete)
This PR adds a base for OIDC clients.
What it does:
/iam/oidc/startand/iam/oidc/callbackhandlers for OIDC auth flows.id_tokenwhen the OIDC auth was successful (req: Gitpod's Google Workspace account)What's next:
[public-api] implement CreateOIDCProviderConfig #15150[oidc] Setup db #15305)stateto be a validated JWTserverto initiate a sessionRelated Issue(s)
Part of #14955 (OIDC client implementation)
Relates to SSO Epic #7761
How to test
There is another test deployment with a flying configuration which can be used to check this.
Going to https://at-oidc-proxy.preview.gitpod-dev.com/iam/oidc/start will initiate the OIDC flow, which will only allow to proceed if you have a Gitpod account on Google. The result of the flow is less exciting should be an output of the id_token of yours.
Release Notes
Documentation
Werft options:
If enabled this will build
install/previewValid options are
all,workspace,webapp,ide,jetbrains,vscode,ssh