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

Add email to JWT claims #17530

Closed
ChevronTango opened this issue May 8, 2023 · 16 comments · Fixed by #17678
Closed

Add email to JWT claims #17530

ChevronTango opened this issue May 8, 2023 · 16 comments · Fixed by #17678
Labels
team: webapp Issue belongs to the WebApp team

Comments

@ChevronTango
Copy link

Is your feature request related to a problem?

I would like to use the tokens generated from gp idp token with gitsign and fulcio but they are missing certain claims for that to work.

{
  "iss": "https://api.gitpod.io/idp",
  "aud": [
    "sigstore"
  ],
  "azp": "sigstore",
  "c_hash": "",
  "exp": 1683410309,
  "iat": 1683406709,
  "auth_time": 1683406709,
  "sub": "https://gitlab.com/ChevronTango/gpg-test/-/tree/main/",
  "name": "ChevronTango"
}

Describe the behaviour you'd like

Please include a additional email and email_verified claims in the JWT token.

https://github.com/sigstore/fulcio/blob/main/docs/oidc.md#email-1

Describe alternatives you've considered

Additional context

Support for fulcio allows me to progress my investigation as part of #666

@axonasif axonasif added the team: webapp Issue belongs to the WebApp team label May 9, 2023
@loujaybee
Copy link
Member

Thanks for raising 🙏 We are actively looking into our OIDC support, and will take a look further into this @ChevronTango ! Hopefully we can move the current token implementation to a more visible beta status soon, and then we can look at modifying and adding additional claims as you suggested!

@ChevronTango
Copy link
Author

Thanks for raising 🙏 We are actively looking into our OIDC support, and will take a look further into this @ChevronTango ! Hopefully we can move the current token implementation to a more visible beta status soon, and then we can look at modifying and adding additional claims as you suggested!

Thanks @loujaybee. This is very exciting and I'm keen to help get it out ASAP. It unlocks a lot of potential for Gitpod, so if there's anything we can do to help do let us know.

@loujaybee
Copy link
Member

loujaybee commented May 19, 2023

Thanks for the quick turnaround, on the pull request @csweichel !

Question: Do we not also need to proxy through the email_verified property of the user info from the IDP? I believe that claim is also required for the Fulcio integration? (see docs), though I can also try when this is released/deployed.

@ChevronTango
Copy link
Author

Thanks for the quick turnaround, on the pull request @csweichel !

Question: Do we not also need to proxy through the email_verified property of the user info from the IDP? I believe that claim is also required for the Fulcio integration? (see docs), though I can also try when this is released/deployed.

email_verified is indeed required and I'm tempted to say this issue isn't complete yet without it, though I am immensly grateful for the very rapid turn around! Thanks a lot guys. Any chance we can add that extra field in before we close this?

@filiptronicek
Copy link
Member

filiptronicek commented May 19, 2023

I think there might be hope still with this deployment 😄! Although I'm not familiar with the OIDC go package we are using, I poked around the documentation and it seems that email_verified might be a field that automatically gets added with the second argument of the setter function.

https://github.com/zitadel/oidc/blob/cdf2af6c2c3b10b3c066ae28ff43e7acd5a6e0b4/pkg/oidc/userinfo.go#L56-L64

type UserInfoSetter interface {
	UserInfo
	SetSubject(sub string)
	UserInfoProfileSetter
	SetEmail(email string, verified bool)
	SetPhone(phone string, verified bool)
	SetAddress(address UserInfoAddress)
	AppendClaims(key string, values interface{})
}

@filiptronicek
Copy link
Member

filiptronicek commented May 19, 2023

@ChevronTango should be good. Have a nice weekend :)! I think this will deploy on Monday, but running from sources it works.

image

@ChevronTango
Copy link
Author

You are a star @filiptronicek, much appreciated... now we just need to get everything else working!

@csweichel
Copy link
Contributor

email_verified

email_verified was a last minute addition to the original PR :)

@ChevronTango
Copy link
Author

I hate to be the downer but...

{
  "iss": "https://api.gitpod.io/idp",
  "aud": [
    "sigstore"
  ],
  "azp": "sigstore",
  "c_hash": "rwIyisNnRNIDTlLm1pbRGQ",
  "exp": 1684860542,
  "iat": 1684856942,
  "auth_time": 1684856942,
  "sub": "https://github.com/ChevronTango/fulcio",
  "name": "ChevronTango",
  "email": "admin@example.com",
  "email_verified": true
}

that's not my email

@ChevronTango
Copy link
Author

this raises a very good point, as we are going to be asking people to trust Gitpod and the email that it says the user is. Where is the email taken from? if a user has added additional integrations/providers, such as private GitLab or Bitbucket instances, do we trust those to provide the email correctly? I feel like only the main GitLab/GitHub IDP's can really be trusted. Does each account in Gitpod have a single verified email that cannot be changed? Where has that email come from? is it always from one of the main IDPs or could it come from a third party? Can you have different emails from different IDP's all linked to the same Gitpod account?

@loujaybee
Copy link
Member

Thanks for the report @ChevronTango, we'll take a look into the email issue. Regarding the questions about IDP source for the emails, we'll factor that into our upcoming documentation: https://github.com/gitpod-io/website/pull/3656. I'll try to remember to drop back here when we have that put together to see if it covers everything you mentioned above. 🙏

@ChevronTango
Copy link
Author

So I recon I know what happened with that email. I added an additional IDP to connect to a test GitLab instance I was playing with and that had a dummy email I added. I was testing a repo in my dummy GitLab instance using a dummy account but still wanted to use Gitpod to test it.

My worry is that this exposes a dangerous attack vector with the new email IDP. If I can create a new integration in Gitpod with any email in that I like and associate it with my account, I can potentially impersonate any user/email that I like.

The only way we can realistically use this is if the provider you choose to use for the primary email matches the provider for the currently open workspace, and also if that is one of the built in integrations and not a user added one. The current approach of just cycling through all the providers in an account isn't robust enough, especially if we want people to trust us to have validated this properly.

@filiptronicek
Copy link
Member

@ChevronTango we were chatting about the attack vector you're mentioning here yesterday. We should never trust self-hosted GitLab instances with giving us e-mail addresses we expose. I think it's ok-enough to trust https://github.com and https://gitlab.com, but only those (maybe in addition to Bitbucket but I'm not familiar with it).

@ChevronTango
Copy link
Author

@ChevronTango we were chatting about the attack vector you're mentioning here yesterday. We should never trust self-hosted GitLab instances with giving us e-mail addresses we expose. I think it's ok-enough to trust https://github.com and https://gitlab.com, but only those (maybe in addition to Bitbucket but I'm not familiar with it).

Agreed. Best approach is to only trust the main IDPs, and it would be even better if we picked the IDP that matched the currently open workspace that was requesting the token. Any manually added integrations should never be trusted to provide accurate information in gitpod tokens.

I think Bitbucket is fine to trust.

I suppose an alternative is for gitpod to validate the emails itself, but that would add a lot of complexity with email integrations that I think is unnecessary for the ultimate end goal.

@ChevronTango
Copy link
Author

@ChevronTango we were chatting about the attack vector you're mentioning here yesterday. We should never trust self-hosted GitLab instances with giving us e-mail addresses we expose. I think it's ok-enough to trust https://github.com and https://gitlab.com, but only those (maybe in addition to Bitbucket but I'm not familiar with it).

As an addendum, the services we trust should be configurable in config rather than hard coded. I imagine some Gitpod Dedicated users may well trust different private repositories like Gitlab for their organisation as a whole. Those aren't user configured though, but we should have the option for an administrator to say which core IDPs they are happy to trust.

@ChevronTango
Copy link
Author

ChevronTango commented Jun 13, 2023

@filiptronicek @loujaybee is there an issue to cover the remediation work highlighted here or should I raise one. I note that sigstore/fulcio#1177 is currently held up on this as I can't reasonably add gitpod as an IDP to a service like fulcio when the emails it could be sending back are potentially anything the user wants. Fulcio is too important a service and they need to be able to trust Gitpod is being honest about the emails it is claiming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants