Skip to content

Fix: use bearer auth when pulling binaryTarget or packages from a package registry - #7662

Merged
MaxDesiatov merged 1 commit into
swiftlang:mainfrom
pwallrich:fix/bearer-auth
Jun 15, 2024
Merged

MaxDesiatov merged 1 commit into
swiftlang:mainfrom
pwallrich:fix/bearer-auth

Conversation

@pwallrich

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

@swift-ci test

@MaxDesiatov MaxDesiatov left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
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
Copy Markdown
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!

guard let (user, password) = self.authentication(for: url) else {
return nil
}
guard user != "token" else {

Copy link
Copy Markdown
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
Copy Markdown
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

yim-lee commented Jun 17, 2024

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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