Skip to content

Conversation

@ecrupper
Copy link
Contributor

No description provided.

@ecrupper ecrupper self-assigned this Mar 15, 2023
@ecrupper ecrupper requested a review from a team as a code owner March 15, 2023 17:59
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

A couple of comments / questions.

I can't offer an informed opinion on the implementation (HMAC vs pub/private key pair). I'm fine with both/either. Whatever is both maintainable and secure enough.

- specification of IP address in the claims to allow restriction by IP
- stricter expiration logic

In the case where the admins would prefer to continue using the symmetric token, all the above worker design logic will use the `VELA_SERVER_SECRET` token, which will not expire and not hinder executors' ability to pull from the queue.
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean that the VELA_SERVER_SECRET can be used as a persistent worker auth token?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the idea is that if it's provided (and there is a matching VELA_SECRET in the server env) then the server will honor that as a non-expiring auth method

- The server secret would be dropped from the container environment altogether if platform admins opt in to this enhancement.
- All auth tokens (check in + registration) have a strict expiration.
- Registration tokens can only be generated by a platform administrator
- Registration tokens should expire very quickly
Copy link
Member

Choose a reason for hiding this comment

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

Any suggestions about how quickly? Would this be configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default will be 1m since the accompanying CLI changes will be quick enough to not have that be a problem. But yes it will be configurable.

Copy link
Contributor

@jbrockopp jbrockopp left a comment

Choose a reason for hiding this comment

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

LGTM

@ecrupper ecrupper merged commit 2b96d58 into main Mar 29, 2023
@ecrupper ecrupper deleted the proposal/worker-auth branch March 29, 2023 18:08
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.

5 participants