Skip to content

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

Merged
merged 1 commit into from
Jun 15, 2024

Conversation

pwallrich
Copy link
Contributor

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:
Screenshot 2024-06-13 at 22 49 08

After:
Screenshot 2024-06-13 at 22 48 38

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.
@pwallrich pwallrich changed the title Fix bearer auth when pulling binaryTarget or packages from a package registry Fix: use bearer auth when pulling binaryTarget or packages from a package registry Jun 13, 2024
@MaxDesiatov
Copy link
Contributor

@swift-ci test

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Thank you!

@MaxDesiatov
Copy link
Contributor

@pwallrich mind cherry-picking it for release/6.0? You'll have to follow the release branch template for release branch PRs.

@pwallrich
Copy link
Contributor Author

Yes, I'll try to open the release PR.

Do I need to do anything regarding the failed linux smoke test?

@MaxDesiatov
Copy link
Contributor

@swift-ci test

@MaxDesiatov MaxDesiatov enabled auto-merge (squash) June 15, 2024 19:25
@MaxDesiatov MaxDesiatov merged commit 312b752 into swiftlang:main Jun 15, 2024
5 checks passed
pwallrich added a commit to pwallrich/swift-package-manager that referenced this pull request Jun 16, 2024
…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">
pwallrich added a commit to pwallrich/swift-package-manager that referenced this pull request Jun 16, 2024
…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">
@MaxDesiatov MaxDesiatov requested a review from yim-lee June 16, 2024 22:05
@MaxDesiatov
Copy link
Contributor

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 {
Copy link
Contributor

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?

Copy link
Contributor

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.

@yim-lee
Copy link
Contributor

yim-lee commented Jun 17, 2024

However when fetching binaryTargets or pulling packages from a registry basic auth is used always.

Binaries are not supported by the package registry spec yet.

Can you please elaborate on how you reproduce the problem?

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.

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?

@pwallrich
Copy link
Contributor Author

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:
We mainly use the package-registry to host prebuilt xcframeworks.
These are wrapped in a Swift Package that is also in the package-registry.

Package.swift from our package-registry

Package(
  "name": "example-package",
  "products": [ .library(name: "example-package", targets: "example-package") ],
  targets: [ .binaryTarget(name: "example-package", url: "{{package-registry-url}}/path/to/example.zip, checksum: "abc") ]
)

We would now want to include these packages with

dependencies: [
   .package(id: "registryname.example-package", from: "1.0.0")

If we do so with swift package resolve the wrapper package is downloaded successfully using Bearer Auth because of the package registry auth mechanism.

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 token.

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 --bearer-auth to swift package resolve

I am sorry I don't quite understand why you have to change the username?

The workaround we tried is

  • login to package-registry with a token which creates the netrc entry with the user token
  • change the user in the netrc entry to an actual user
  • now call swift package --netrc resolve

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.

@MaxDesiatov
Copy link
Contributor

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.

Unfortunately, this change seems too disruptive. Until a better solution is found, we'll have to revert this.

MaxDesiatov added a commit that referenced this pull request Sep 4, 2024
…om a package registry" (#7811)

Reverts #7662

It would be a breaking change if someone is using `token` as their
username, and we don't have a migration path or any other means to
clarify this breaking change to the users.
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.

3 participants