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 minimal token validation #142

Closed
wants to merge 1 commit into from

Conversation

nanonyme
Copy link
Contributor

It's exceptionally normal error that token expires. Emit local error if it happens with token payload to help request a new token.

It's exceptionally normal error that token expires. Emit local error
if it happens with token payload to help request a new token.
Comment on lines +1003 to +1004
if datetime.datetime.now(datetime.timezone.utc) > expiration:
raise SystemExit(f"Token {payload} has expired")
Copy link
Contributor

@bbhtt bbhtt Dec 21, 2024

Choose a reason for hiding this comment

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

better to to just print out the expiry date here than the JSON blob

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 intent here is JSON blob contains information what exactly should be requested. It may have one or multiple prefixes, one or multiple branches etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also contains the name of token that was created.

Copy link
Contributor

@bbhtt bbhtt Dec 21, 2024

Choose a reason for hiding this comment

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

yes, but why would need that information in a "token has expired" error message? the error is that it expired.

if you want to log the capabilities of the token it should be somewhere else, not in this error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

Copy link
Contributor

@bbhtt bbhtt Dec 21, 2024

Choose a reason for hiding this comment

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

I'd just log them whenever the token is used with verbose: the ref prefix, target repo and capabilities, type and the branch are useful information. But logging the whole payload json blob might be problematic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like fairly invasive changes. If someone else wants to pick that up, fine. I think touching non-standard fields in blobs in client is actively dangerous since client might not be at all same version as server. JWT is for most intents and purposes supposed to be considered opaque blob by clients. Exp just happens to be a standard field and useful client-side.

Copy link
Contributor

Choose a reason for hiding this comment

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

I went ahead and improved the server's response a bit in case of expiry #144

image

@nanonyme
Copy link
Contributor Author

Seems something wider in scope is desired so I'm closing this one. I'm leaving behind the branch though the JWT unpacking is well documented.

@nanonyme nanonyme closed this Dec 21, 2024
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.

2 participants