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

feat(proposal): Vela OpenID Connect Provider #976

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

dtanner
Copy link

@dtanner dtanner commented May 10, 2024

No description provided.

@dtanner dtanner requested a review from a team as a code owner May 10, 2024 14:31
Comment on lines +85 to +87
### ID_TOKEN_REQUEST_TOKEN

Token is signed by _VELA_SERVER_PRIVATE_KEY_ using HS256 algorithm. It has the following claims:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think converting all our JWTs to the RS256 method is probably the way to go but figured that transition was a tad out of scope for this proposal.

But in terms of this specific new token (VELA_ID_TOKEN_REQUEST_TOKEN), should we implement a server-side parser of the RSA keys and have it signed by the same private key as the ID_TOKEN? Or add another token type to the HMAC set with plans to migrate everything at a later time?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

converting all our JWTs to the RS256 method is probably the way to go but figured that transition was a tad out of scope for this proposal.

i agree with all of that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i picture rsa key parsing to be handled on the client but if you want to explain this a bit more maybe im not understanding the benefits fully

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think its fine to have the request tokens and the id tokens signed by the same key but i havent looked into it much to know if theres a "standard" for that. im sure if you ask a security pro theyll probably suggest we dont reuse keys 😅

}

// Database Func
func (e *engine) RotateKeys(_ context.Context) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about auto rotating keys after N days

- The build is the expected build (build_id)
- The token is not expired
- The token has indeed been signed by the Vela server
- The associated build is actively running (leaked request tokens are not vulnerable post build).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this run the risk that an external service using the token might not be done with it yet? maybe we give it a little bit of wiggle room?

im picturing a scenario where Vela hands off the workload to a different service and that service might take some time to provision resources or something.

or... are we expecting id_tokens to always be generated within the build context and then the resulting token can be handed off to the external service?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ID token request token is bound by that rule yes because the request for an ID token should always occur during a running build. If there is a pass off that occurs, the new workload should be using the ID token not the ID request token

@plyr4 plyr4 changed the title Create 05-10_OpenID-Connect.md feat(proposal): Vela OpenID Connect Provider May 16, 2024
@plyr4
Copy link
Contributor

plyr4 commented May 17, 2024

some food for thought, theres this open source provider/consumer library we might be able to pull inspiration from for some of the lower level details
https://github.com/zitadel
it seems overkill, sure, but maybe a good resource

@plyr4
Copy link
Contributor

plyr4 commented May 17, 2024

we might want to also look into using csrf tokens that the server holds onto to protect against request forgery

this article from Google goes into a bit more detail https://developers.google.com/identity/openid-connect/openid-connect#createxsrftoken

the idea is that you exchange another "session token" that the user needs to pass along at each part of the process to ensure someone malicious didnt man in the middle the token

probably overkill for this first pass, and im not sure how to do the user identity bit but definitely something to think about if we want to build a "consumer" that accepts the id_token in exchance for an access_token

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants