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 note about ghcr.io for HOMEBREW_DOCKER_REGISTRY_TOKEN #16794

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

azatoth
Copy link
Contributor

@azatoth azatoth commented Mar 2, 2024

It's not clear that when using HOMEBREW_DOCKER_REGISTRY_TOKEN directly against ghcr.io that you should use your PAT base64-encoded as the bearer token.

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Thanks @azatoth! Definitely think this needs changing, just let's riff off exactly what to.

Comment on lines 169 to 174
Use this bearer token for authenticating with a Docker registry proxying GitHub Packages.
Preferred over `HOMEBREW_DOCKER_REGISTRY_BASIC_AUTH_TOKEN`.

*Note:* when authenticating against ghcr.io, \
the `HOMEBREW_DOCKER_REGISTRY_TOKEN` must be a base64 \
encoded GitHub Personal Access Token (PAT).
Copy link
Member

Choose a reason for hiding this comment

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

Not convinced this description makes that much sense after this? If you're "authenticating against ghcr.io" is this "a Docker registry proxying GitHub Packages"? It doesn't sound like it. Maybe the whole description needs a bigger overhaul?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, this is related to the underlying issue I had, trying to access a private tap hosted on a private GitHub repo. And while optimally, there should be a way to be able to assign different tokens for different taps, at the moment this environment variable is the only way to address brew to authenticate again ghcr.io.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should default to HOMEBREW_GITHUB_API_TOKEN and/or read from the repository URL like #16649. Avoiding the user from having to manually run base64 seems ideal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should default to HOMEBREW_GITHUB_API_TOKEN

While it's highly possibly it's the same token, conceptually it's wrong as the docker registry doesn't have anything to do with the API and vice versa. Wouldn't defaulting to HOMEBREW_GITHUB_PACKAGES_TOKEN be more logical (which in itself then might need to be promoted out from development towards more general use)?

and/or read from the repository URL

I assume it makes sense to allow it to read from the URL, even though having the token in the URL isn't something I personally would ever do.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't defaulting to HOMEBREW_GITHUB_PACKAGES_TOKEN be more logical (which in itself then might need to be promoted out from development towards more general use)?

Yup, this would make sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • default to HOMEBREW_GITHUB_API_TOKEN

Don't you mean HOMEBREW_GITHUB_PACKAGES_TOKEN?

Copy link
Member

Choose a reason for hiding this comment

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

No, that's another option we could use but if HOMEBREW_GITHUB_API_TOKEN is setup correctly we should be able to Base64 it and use it accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that's another option we could use but if HOMEBREW_GITHUB_API_TOKEN is setup correctly we should be able to Base64 it and use it accordingly.

I think I'm a bit confused, I thought the API token was for accessing the API and PACKAGES token to access packages in the event you would want to have different tokes with limited permissions in each

Copy link
Member

Choose a reason for hiding this comment

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

HOMEBREW_GITHUB_PACKAGES_TOKEN is currently only used for uploading packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • default to HOMEBREW_GITHUB_API_TOKEN and do the Base64 internally inside of Homebrew

I decided to try this route;
I did rebase against master, but I don't know what your policy for PRs actually is in this regard.

Library/Homebrew/env_config.rb Outdated Show resolved Hide resolved
Library/Homebrew/env_config.rb Outdated Show resolved Hide resolved
@azatoth azatoth requested a review from MikeMcQuaid March 4, 2024 10:19
@Bo98
Copy link
Member

Bo98 commented Mar 5, 2024

(Prior discussion for context: https://github.com/orgs/Homebrew/discussions/5171)

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Would like to see comments addressed or further discussion, thanks!

Copy link

github-actions bot commented Apr 9, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Apr 9, 2024
@azatoth
Copy link
Contributor Author

azatoth commented Apr 9, 2024

Would like to see comments addressed or further discussion, thanks!

I might have misunderstood you, but I thought I had addressed your comments, and I don't know what more I can say to further the issue.

It's not clear that when using HOMEBREW_DOCKER_REGISTRY_TOKEN directly
against ghcr.io that you should use your PAT base64-encoded as the
bearer token.
And adjusting the documentation to match
@azatoth azatoth force-pushed the docker-registry-token-documentation branch from 76301fa to 55e9913 Compare April 10, 2024 10:09
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Great work @azatoth, almost done here!

Comment on lines +168 to +171
description: <<~EOS,
Use this bearer token for authenticating with a Docker registry proxying GitHub Packages.
Preferred over `HOMEBREW_DOCKER_REGISTRY_BASIC_AUTH_TOKEN`.
EOS
Copy link
Member

Choose a reason for hiding this comment

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

Please put this all on one line again, thanks.

Use this bearer token for authenticating with a Docker registry proxying GitHub Packages.
Preferred over `HOMEBREW_DOCKER_REGISTRY_BASIC_AUTH_TOKEN`.
EOS
default_text: "`QQ==` unless `HOMEBREW_DOCKER_REGISTRY_BASIC_AUTH_TOKEN` is set.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
default_text: "`QQ==` unless `HOMEBREW_DOCKER_REGISTRY_BASIC_AUTH_TOKEN` is set.",
default_text: "`HOMEBREW_DOCKER_REGISTRY_BASIC_AUTH_TOKEN` if set or `QQ==` otherwise.",

"<https://docs.github.com/en/rest/overview/rate-limits-for-the-rest-api>" \
"\n\n *Note:* Homebrew doesn't require permissions for any of the scopes, but some " \
"developer commands may require additional permissions.",
description: <<~EOS,
Copy link
Member

Choose a reason for hiding this comment

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

Please put this all one one line too.

*Note:* If set, this token will also be used to authenticating against `ghcr.io` unless
`HOMEBREW_DOCKER_REGISTRY_TOKEN` or `HOMEBREW_DOCKER_REGISTRY_BASIC_AUTH_TOKEN` has been set.
In this case, the token will require the `packages` permission to be set.
`HOMEBREW_GITHUB_PACKAGES_TOKEN` is not used for this permission, but only for uploading packages.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`HOMEBREW_GITHUB_PACKAGES_TOKEN` is not used for this permission, but only for uploading packages.

Remove this and move to the HOMEBREW_GITHUB_PACKAGES_TOKEN description (if there is one).

*Note:* Homebrew doesn't require permissions for any of the scopes,
but some developer commands may require additional permissions.

*Note:* If set, this token will also be used to authenticating against `ghcr.io` unless
Copy link
Member

Choose a reason for hiding this comment

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

Love this, great work!

Use this personal access token for the GitHub API, for features such as `brew search`.
You can create one at <https://github.com/settings/tokens>.
If set, GitHub will allow you a greater number of API requests.
For more information, see: "<https://docs.github.com/en/rest/overview/rate-limits-for-the-rest-api>"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For more information, see: "<https://docs.github.com/en/rest/overview/rate-limits-for-the-rest-api>"
For more information, see: <https://docs.github.com/en/rest/overview/rate-limits-for-the-rest-api>

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

Successfully merging this pull request may close these issues.

None yet

4 participants