Add support to login using identity from an identity-aware proxy #715
No reviewers
Labels
No Label
dependencies
duplicate
help wanted
invalid
kind/bug
kind/feature
needs reproduction
question
security
wontfix
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/vikunja#715
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "branchmispredictor/api:feature/identity-aware-proxy"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description
This PR adds the api component to allow users to sign in using an identity provided by an Identity-Aware Proxy (IAP). One such example is Pomerium.
The IAP (in this case Pomerium) passes a signed JWT in an HTTP header to any backend service running behind it. This JWT contains a claim for
email
,iss
,sub
, and other typical items. To validate the JWT, we get a public key from the proxy's jwks url.This PR has grown a good bit. There is an identityawareproxy middleware on all authenticated endpoints and on a special IAP token endpoint. It validates the identity from the proxy and attaches it to the echo context. All internal auth will use this identity if it exists.
The identityawareproxy token endpoint, and the passing of tokens to the frontend of a new auth type is to:
The token endpoint can be called at any time to get a new token based on the IAP identity.
TODO: modify the frontend to call the endpoint often enough
Other Changes
This PR moved the claims map into a struct. This removes the potential for miscopying or mistyping a string key when accessing a claim from the claims map. This also removes a potential issue where claim values could be truncated (e.g. in
int64(claims["sharedByID"].(float64))
, if the sharedById was above the maximum integer a float64 could hold, that value would be truncated).Claims were also disambiguated to prevent type confusion. Now there is no chance that a linkshare JWT can me mistaken for a user JWT (previously both simply used an "id" claim, now they are "user_id" vs "link_share_id"). Not every caller was checking the JWT "type" claim to see if the token was for a user or a link share to begin with, and this removes the need to do so altogether.
Checklist
mage do-the-swag
)config.yml.saml
and runningmage generate-docs
)@ -229,6 +233,8 @@ func InitDefaultConfig() {
// Auth
AuthLocalEnabled.setDefault(true)
AuthOpenIDEnabled.setDefault(false)
AuthOpenIDProviders.setDefault(make([]interface{}, 0))
This was required to stop getting a 500 on the /info endpoint.
Huh, that sounds a lot like a bug. I'll look into that and maybe backport it - the openid integration is pretty new and still a bit rough around the edges.
Fixed in
d2d610e0f5
@ -0,0 +115,4 @@
return auth.NewUserAuthTokenResponse(u, c)
}
func getOrCreateUser(cl *claims) (u *user.User, err error) {
This is almost an exact copy from the method in
openid.go
. I'll try to refactor into some common helper, but I'm not too familiar with go and how to get them to work with both different claim structs. Open to suggestions here.Maybe just have the parameters of the claim like
Issuer
,Subject
,Username
and so on as function parameters? Or a separatecreateUserOpts
struct or something like that.Add support to login using identity from an identity-aware proxyto WIP: Add support to login using identity from an identity-aware proxy@ -0,0 +70,4 @@
// Parse the jwt from the identity-aware proxy using the correct key
tken, err := jwt.ParseWithClaims(token, &claims{}, func(unvalidatedToken *jwt.Token) (interface{}, error) {
// Only support either ECDSA or RSA signing methods
Please document this.
@ -0,0 +73,4 @@
// Only support either ECDSA or RSA signing methods
if _, ok := unvalidatedToken.Method.(*jwt.SigningMethodECDSA); !ok {
if _, ok := unvalidatedToken.Method.(*jwt.SigningMethodRSA); !ok {
return nil, fmt.Errorf("Unexpected signing method: %v", unvalidatedToken.Header["alg"])
Please make errors like this into custom error types which you'd then return.
@ -0,0 +83,4 @@
}
keys := keyset.LookupKeyID(keyID)
if len(keys) != 1 {
What if there's more than one Key? Is that even possible? In that case, should we still just use the first one or return a different error?
@ -0,0 +187,4 @@
}
// Validates the claims in the jwt
func (c claims) Valid() error {
Please use a pointer as the parameter, like so:
c *claims
.@ -0,0 +188,4 @@
// Validates the claims in the jwt
func (c claims) Valid() error {
// Validate that expiresAt and issuedAt are set and valid (with up to 1 minute of skew)
Would it make sense to make that skew configurable?
As far as I understand it, IAPs handle all the authentication related stuff and then proxy all subsequent request to the actual application (in this case Vikunja). This would mean that you'd need to add a middleware to check if an IAP was configured and if that's the case validate it and make sure the user exists and so on, essentially bypassing Vikunja's authentication methods. How are you planning to authenticate users through the seperate endpoint you created?
This would also mean you won't need any frontend changes as everything would be handled through the header.
Yes, the IAP will be proxying all requests to the application with the JWT header set.
Initially I was envisioning a login-with-IAP-identity button that essentially works as another login button. This would support enabling multiple auth solutions if need be, for instance local auth + proxy auth.
Checking this with a middleware may be a better approach, I can try that out if you think that's reasonable. In that case, I think there are a couple of standing questions:
7c99c9b0d0
toba3805443c
I think so, please try that :slight_smiling_face:
As far as I understand it, you would not get to Vikunja through the IAP if you're not logged in, so a button would be obsolete.
I think you should check if a user exists for the
(iss, sub)
tuple and log that user in, if no user was found create one and log them in. That's essentially whatgetOrCreateUser
does.How is logout handled in general with an IAP?
79ca2c4e2f
toce96e2b077
WIP: Add support to login using identity from an identity-aware proxyto Add support to login using identity from an identity-aware proxyI've rebased the PR and hopefully fixed any remaining CI issues. Sorry for the delay, work was quite busy leading up to the holiday season. I've also updated the frontend PR to match these changes.
Hey @konrad, just wanted to check in on this PR. If it's not in an acceptable state I can work on it further. Or if it's not something you're considering accepting at this point (perhaps an auth overhaul is in the works for OAuth or something), that's fine too. I'd just like to know the general status before I start fixing merge conflicts again.
@branchmispredictor I'll give it another review, I just haven't found the time yet.
What about logging the user out?
Nothing sure why the pr was closed, that wasn't intentional.
I think that came from renaming the main branch.
Thanks for refactoring! (And sorry for my very late review...)
I think this looks quite good so far, just some comments.
Just to clarify if I understood it correctly: Does using the iap middleware mean only either local auth or iap auth can be used? If that's the case, there should be a check when initializing the docs with an error if both are configured.
@ -43,0 +78,4 @@
// In these cases, AuthClaims may contain hints to the user identity,
// but an outside source is the final source-of-truth for auth (e.g. Identity-Aware Proxy auth)
type AuthProvider interface {
GetWebAuth(echo.Context, *AuthClaims) (web.Auth, error)
I think this should return a
*user.User
since the external auth is still represented as a user in Vikunja. It's not a "new type of authentication" like link share is.@ -0,0 +39,4 @@
}
// ErrorCodeIAPTokenMissing holds the unique world-error code of this error
const ErrorCodeIAPTokenMissing = 12001
This should be documented in
docs/content/doc/usage/errors.md
@ -0,0 +81,4 @@
// @Produce json
// @Success 200 {object} auth.Token
// @Failure 500 {object} models.Message "Internal error"
// @Router /auth/identityawareproxy/token [get]
Is there a special reason this is a separate endpoint instead of reusing the existing token one? It seems like this will require a frontend change?
There's a problem with bootstrapping auth here. The way IAPs work is by setting an http header with claims to the downstream service (vikunja-api), however javascript or front-end code does not have access to http headers.
So on initial load, there is no cookie / vikunja-jwt set so the frontend does not see us as logged in. Similarly, the existing
/user/token
endpoint is also unavailable because it is behind the jwt middleware and requires a jwt cookie set. So this endpoint exists outside of the jwt middleware so it can be accessed from before a user is "logged in"So this is the endpoint to "translate" the http header from the IAP to a Vikunja token? Wouldn't it be possible to just extend the existing middleware to look for that http header? All that'd be left then would be to extend the frontend so it somehow checks if the user is logged in (maybe by calling
/api/v1/user
and see if there's a 200 response coming back and then setting the frontend state to "logged in"?)What I can do, is abtract this away a bit and make this an/auth/externalprovider/loggedin
endpoint, so any future external auth source might also reuse the same endpoint.Was writing that before I saw the reply. Yes, this basically translates the http header from the IAP to a vikunja token. The only problem is that the jwt middleware will error out for /api/v1/user/* if the frontend does not provide an already valid jwt as a cookie
How would that look in the frontend, would it call that endpoint every time on reload?
Yes, or at least if not already logged in / if current jwt token is expired. Essentially it's similar to an open-id auth from the frontend's point of view except it 1) is hitting vikunja-backend directly instead of first hitting an external service and 2) it is automatically calling the endpoint instead of waiting for the openid button to be hit.
We can actually make num 2 optional, if for whatever reason we prefer being able to have both IAP login and local/openid logins too.
Then I'd prefer modifying the existing jwt middleware to accept a jwt token or an IAP header if no token is provided (and move all the logic to create a new user etc there as well). That way, we won't need the new endpoint and can modify the frontend only a bit to make a call to
/user
every time it is loaded, regardless if it has a jwt token in localstorage or not. I think that would make the IAP a lot more transparent to the frontend.Okay, I'll give it a shot.
Following up just to make sure I capture the implicit decisions here, so you want for the backend:
In the frontend:
Almost, I wouldn't generate a jwt token from the backend at all but instead use the IAP header to authenticate the user.
That would look like this in the middleware:
The frontend would then always call
user
to figure out if the user is authenticated or not. Currently it only does that if a jwt token is present in local storage.@ -0,0 +143,4 @@
skew := time.Minute
if !c.VerifyExpiresAt(now.Add(-skew).Unix(), true) {
delta := now.Sub(time.Unix(c.ExpiresAt, 0))
return fmt.Errorf("token is expired by %v", delta)
All errors should have a proper type in
errors.go
. Maybe you could add a new error typeErrorIAPClaimsInvalid
and then return that from here for all errors? That would avoid creating 5 different error types for basically the same error.@ -0,0 +16,4 @@
package identityawareproxy
// TODO test GetWebAuth()
🙂
@ -0,0 +34,4 @@
var TimeFunc = time.Now
// Caches the public keys of the identity-aware proxy used to validate the auth data it sends
type iapCache struct {
Please use the
keyvalue
package for this.I will re-review the frontend PR once this one is ready to merge.
Since you unified the external auth providers, could you add some docs about how to add a new auth provider in the future?
I didn't quite pull in openid, since it does something a bit different (there's no continuous checking against the external auth provider), but I can spruce up the docs a bit.
It's a bit awkward, but it is possible for someone to log out and log in as another user in an IAP. If so, the backend will immediately see the new identity in the IAPClaims. The frontend will not, and will have a JWT cookie pointing to the old identity. There's an if statement to catch this specific case of mismatched identities and raise an error. The next time the frontend gets a new token from /identityawareproxy/token, it will overwrite the old and outdated JWT cookie and then will be fully logged in as a new user.
But wouldn't this only take effect once the page is reloaded? And at that point the frontend will reload everything (lists in the menu etc) so it would change the content and update the state?
f75e8fa764
to8840525b4a
Okay, I've updated the PR to make the IAP middleware more transparent as requested. Now, all the frontend needs to do is call
/user/token
on refresh and it will get an IAP derived token if there is one.TODO:
8840525b4a
toe6af0ac4ee
e6af0ac4ee
to0c87a9d221
0c87a9d221
to0b82e1ddf3
0b82e1ddf3
to7e92a273eb
7e92a273eb
to191261a5cd
191261a5cd
tof7642b7982
f7642b7982
to911dffd8b3
Oh and by the way, you don't need to force push all the time - I'll squash merge anyway at the end 🙂
I'll close this due to lack of activity - please ping if you want to pick it up again.
Pull request closed