-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix: use bearer auth when pulling binaryTarget or packages from a package registry #7662
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
Conversation
When logging in to a package registry bearer auth works. However when fetching binaryTargets or pulling packages from a registry basic auth is used always. So the header will basically be "Basic token:acces_token" This either breaks services that don't use basic auth or leads to nasty workarounds like first creating a netrc file with "token" as the user and after login changing the user back to the actual user. Assuming that basic auth is allowed with an identiy token. When using "bearer" auth, when the user is "token" these issues are fixed.
@swift-ci test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
@pwallrich mind cherry-picking it for |
Yes, I'll try to open the release PR. Do I need to do anything regarding the failed linux smoke test? |
@swift-ci test |
…kage registry (swiftlang#7662) Use bearer auth when pulling binary targets or a package from a package registry and the user is `token`. ### Motivation: When logging in to a package registry bearer auth works. However when fetching binaryTargets or pulling packages from a registry basic auth is used always. This either breaks services that don't use basic auth or leads to nasty workarounds like first creating a netrc file with "token" as the user and after login changing the user back to the actual user. Assuming that basic auth is allowed with an identiy token. ### Modifications: Check if the user is `token` when creating the authorization header and use Bearer auth in these cases. ### Result: When the user is `token` the `Authorization` header will be set to `Bearer {{access_token}}` instead of `Basic token:{{access_token}}`. Before: <img width="698" alt="Screenshot 2024-06-13 at 22 49 08" src="https://github.com/apple/swift-package-manager/assets/13999931/0f3eef32-55e3-417f-b129-c138ca452b08"> After: <img width="699" alt="Screenshot 2024-06-13 at 22 48 38" src="https://github.com/apple/swift-package-manager/assets/13999931/d98daaa7-1535-40d0-96ea-a6736f5d1e3e">
…kage registry (swiftlang#7662) Use bearer auth when pulling binary targets or a package from a package registry and the user is `token`. ### Motivation: When logging in to a package registry bearer auth works. However when fetching binaryTargets or pulling packages from a registry basic auth is used always. This either breaks services that don't use basic auth or leads to nasty workarounds like first creating a netrc file with "token" as the user and after login changing the user back to the actual user. Assuming that basic auth is allowed with an identiy token. ### Modifications: Check if the user is `token` when creating the authorization header and use Bearer auth in these cases. ### Result: When the user is `token` the `Authorization` header will be set to `Bearer {{access_token}}` instead of `Basic token:{{access_token}}`. Before: <img width="698" alt="Screenshot 2024-06-13 at 22 49 08" src="https://github.com/apple/swift-package-manager/assets/13999931/0f3eef32-55e3-417f-b129-c138ca452b08"> After: <img width="699" alt="Screenshot 2024-06-13 at 22 48 38" src="https://github.com/apple/swift-package-manager/assets/13999931/d98daaa7-1535-40d0-96ea-a6736f5d1e3e">
Hi @yim-lee, would you have a moment to have a look at this PR, as the original SE proposal author to confirm that this change makes sense? Thanks! |
@@ -71,6 +71,9 @@ extension AuthorizationProvider { | |||
guard let (user, password) = self.authentication(for: url) else { | |||
return nil | |||
} | |||
guard user != "token" else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using token
as username for bearer auth is specific to registry auth. Adding this change would mean applying that logic for all SwiftPM auth. Is this what we want @MaxDesiatov?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @bnbarham. I feel like we need to revert it if you think the behavior change (even though for making it consistent) is undesired.
Binaries are not supported by the package registry spec yet. Can you please elaborate on how you reproduce the problem?
This is sort of how registry auth is supposed to work: the credentials are either found in netrc file or Keychain. I am sorry I don't quite understand why you have to change the username? |
Took a while to reproduce the original issue again. The package registry stuff is already removed in the project since we had this issue. A bit more detailed: Package.swift from our package-registry
We would now want to include these packages with
If we do so with Next step is downloading the binaryTarget that is hosted on the same server. This fails, since the binaryArtifactManager doesn't know anything about the package-registry settings and does basic auth with the user Therefore we can't use the package-registry for any packages that contain a binaryTarget hosted on the same server. That's why we had to remove the whole package-registry stuff and went back to repos hosted in git that only download binaryTargets from the server. And that's what the change in this PR solves and why I opened it. I admit that the solution is not the best one. A better one could be to also have a configuration for binaryTargets or to be able to pass
The workaround we tried is
This leads to the package-registry stuff still doing bearer auth (since that's what's specified in the configuration) and the binaryTarget being download with basic auth with the correct username. It works, but is a bit annoying and error prone since you have to manage the netrc yourself and can't rely on the auto generated entry. |
Unfortunately, this change seems too disruptive. Until a better solution is found, we'll have to revert this. |
Use bearer auth when pulling binary targets or a package from a package registry and the user is
token
.Motivation:
When logging in to a package registry bearer auth works. However when fetching binaryTargets or pulling packages from a registry basic auth is used always.
This either breaks services that don't use basic auth or leads to nasty workarounds like first creating a netrc file with "token" as the user and after login changing the user back to the actual user. Assuming that basic auth is allowed with an identiy token.
Modifications:
Check if the user is
token
when creating the authorization header and use Bearer auth in these cases.Result:
When the user is
token
theAuthorization
header will be set toBearer {{access_token}}
instead ofBasic token:{{access_token}}
.Before:

After:
