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

contrib: Fix GitHub token check to allow fine-grained tokens #22963

Merged
merged 1 commit into from Jan 10, 2023

Conversation

gentoo-root
Copy link
Contributor

Classic GitHub tokens can be passed in the HTTP Authorization header in multiple ways, including as the username or as the password in Basic authentication. However, beta "Fine-grained personal access tokens" are not accepted as the username, which breaks the check in gitlib.sh.

For the purpose of that check, pass the token as the password, allowing both classic and beta tokens to pass the check.

Signed-off-by: Maxim Mikityanskiy maxim@isovalent.com

Classic GitHub tokens can be passed in the HTTP Authorization header in
multiple ways, including as the username or as the password in Basic
authentication. However, beta "Fine-grained personal access tokens" are
not accepted as the username, which breaks the check in gitlib.sh.

For the purpose of that check, pass the token as the password, allowing
both classic and beta tokens to pass the check.

Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
@gentoo-root gentoo-root requested a review from a team as a code owner January 6, 2023 16:46
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 6, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jan 6, 2023
@gentoo-root gentoo-root added release-note/misc This PR makes changes that have no direct user impact. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. kind/community-contribution This was a contribution made by a community member. labels Jan 6, 2023
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good to me. I suppose there is no security implication related to the change? The token is still passed in a secure way, right?

@gentoo-root
Copy link
Contributor Author

I suppose there is no security implication related to the change? The token is still passed in a secure way, right?

Of course! Generally speaking, passing the token as a password instead of a username can only be more secure, not less secure. In this case, though, both ways are equally secure, because username:password is converted to base64 and passed with the Authorization: HTTP header as a whole, and the script also treats them equally as a pair, without making a distinction.

@qmonnet qmonnet added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 10, 2023
@aditighag aditighag merged commit 7696c16 into cilium:master Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants