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

feat(crates-io): expose HTTP headers and Error type #12310

Merged
merged 6 commits into from
Jul 20, 2023

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented Jun 25, 2023

What does this PR try to resolve?

This is part of #11521.

RFC 3231 mentions the authentication process could have an additional challenge-response mechanism to prevent potential replay attacks. The challenge usually comes from HTTP www-authenticate header as a opaque string. When a client gets a 401/403 response with such a challenge, it may attach the challenge to the payload and request again to anwser the challenge.

➡️ cargo  requests
⬅️ server responds with `www-authenticate` containing some opaque challenge string
➡️ cargo  automatically requests again without any user perception
⬅️ server responds ok

However, crates-io crate doesn't expose HTTP headers. There is no access to www-authenticate header.

This PR make it expose HTTP headers and the custom Error type, so cargo can access and do further on the authentication process.

How should we test and review this PR?

By commit.

This removes the dependency anyhow and uses our own custom Error enum, so that crates-io consumer can access Error::API::challenge field. It optionally uses thiserror to reduce boilerplates but this part can be dropped if we don't want.

@rustbot
Copy link
Collaborator

rustbot commented Jun 25, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-interacts-with-crates.io Area: interaction with registries A-registries Area: registries A-sparse-registry Area: http sparse registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 25, 2023
@weihanglo
Copy link
Member Author

cc @Eh2406

crates/crates-io/lib.rs Outdated Show resolved Hide resolved
crates/crates-io/lib.rs Show resolved Hide resolved
@weihanglo weihanglo changed the title feat(crates-io) Expose www-authenticate challenge feat(crates-io): expose Cargo challenge from www-authenticate header Jun 25, 2023
@weihanglo weihanglo marked this pull request as ready for review June 27, 2023 10:42
"percent-encoding",
"serde",
"serde_json",
"thiserror",
Copy link
Member Author

Choose a reason for hiding this comment

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

thiserror is already in the dependency graph. Just do we want it to appear as a direct dependency or not.

@@ -1,6 +1,6 @@
[package]
name = "crates-io"
version = "0.36.1"
version = "0.37.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

Side note: the version bump check in CI didn't work, since the new version in beta hasn't yet published. But in reality, the script should treat any version in beta as “published”.

crates/crates-io/lib.rs Outdated Show resolved Hide resolved
@Eh2406
Copy link
Contributor

Eh2406 commented Jun 29, 2023

This looks good to me. @arlosi, what are your thoughts?

@arlosi
Copy link
Contributor

arlosi commented Jun 30, 2023

  • Will this be supported for sparse registry (fetching metadata) as well?

  • How should this interact with credential providers? The way I've currently implemented it is that we'll pass the whole www-authenticate header to the provider. The credential provider would then re-parse the header to extract the nonce. Is that acceptable? Or should we do something different?

@weihanglo
Copy link
Member Author

weihanglo commented Jun 30, 2023

Will this be supported for sparse registry (fetching metadata) as well?

I don't think so. At least not at this moment? This PR doesn't implement the flow mentioned above. It just pokes the return type of crates-io crate. crates-io is only for requesting registry API. It doesn't do anything more than that. But for supporting sparse registry, maybe yes in the future.

BTW, in this PR from_www_authenticate_values() is something shared with sparse registry.

How should this interact with credential providers? The way I've currently implemented it is that we'll pass the whole www-authenticate header to the provider. The credential provider would then re-parse the header to extract the nonce. Is that acceptable? Or should we do something different?

Sorry I am forgetting the details of the fantastic overhaul of credential providers, could you share the doc or the working branch again? BTW if cargo unifies the interface by providing the Challenge struct, is that a bit more convenience for credential provider implementor?

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 5, 2023

Will this be supported for sparse registry (fetching metadata) as well?

According to the asymmetric tokens RFC as written, yes. (Which is not to say that it will happen in this PR.) Of course, if we discover during implementation that this is a bad idea... Implementation and RFC do not have to match. I would expect that only the most paranoid registries will intentionally use a challenge for read/index endpoints. On the other hand, implementing auth would only be made more complicated by requiring registries to have a different scheme for GET vs PUSH.

How should this interact with credential providers? The way I've currently implemented it is that we'll pass the whole www-authenticate header to the provider. The credential provider would then re-parse the header to extract the nonce. Is that acceptable? Or should we do something different?

Passing along the entire header allows registries to use standards cargo doesn't know about, or invent their own. It seems like the most flexible way to provide the data. It is entirely compatible with cargo passing the data it parsed in separate fields for credential providers that are using asymmetric tokens and do not want to do their own parsing.

@weihanglo
Copy link
Member Author

Passing along the entire header allows registries to use standards cargo doesn't know about, or invent their own. It seems like the most flexible way to provide the data. It is entirely compatible with cargo passing the data it parsed in separate fields for credential providers that are using asymmetric tokens and do not want to do their own parsing.

Does this imply that we should just add a new field www_authenticate or even a simple header on ResponseError::Api? Then we can move header handling into cargo lib.

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 5, 2023

That seams reasonable.

@weihanglo weihanglo changed the title feat(crates-io): expose Cargo challenge from www-authenticate header feat(crates-io): expose HTTP headers and Error type Jul 6, 2023
@weihanglo
Copy link
Member Author

@Eh2406 thanks for the reply. This is now ready for another review!

@weihanglo weihanglo removed the A-sparse-registry Area: http sparse registries label Jul 6, 2023
@ehuss
Copy link
Contributor

ehuss commented Jul 8, 2023

@Eh2406 Do you want to take assignment for this review? I'm happy to do a final review if you want.

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 18, 2023

Sorry for the delay. This looks good. Happy to have Eric do the honours. (We need to keep the API brake in mind.)

This removes the dependency `anyhow` and uses our own custom Error
enum, so that crates-io consumer can access `Error::API::challenge`
field.
Optionally use `thiserror` to reduce boilerplates but this part can
be dropped if we don't want.
@weihanglo
Copy link
Member Author

Thanks for the reminder! Bumped to 0.38.0.

@ehuss
Copy link
Contributor

ehuss commented Jul 20, 2023

@bors r=Eh2406

@bors
Copy link
Collaborator

bors commented Jul 20, 2023

📌 Commit 31b500c has been approved by Eh2406

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 20, 2023
@bors
Copy link
Collaborator

bors commented Jul 20, 2023

⌛ Testing commit 31b500c with merge b40be8b...

@bors
Copy link
Collaborator

bors commented Jul 20, 2023

☀️ Test successful - checks-actions
Approved by: Eh2406
Pushing b40be8b to master...

@bors bors merged commit b40be8b into rust-lang:master Jul 20, 2023
20 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2023
Update cargo

8 commits in 1b15556767f4b78a64e868eedf4073c423f02b93..7ac9416d82cd4fc5e707c9ec3574d22dff6466e5
2023-07-18 14:44:47 +0000 to 2023-07-24 14:29:38 +0000
- fix(cargo-credential): should enable feature `serde/derive` (rust-lang/cargo#12396)
- fix: encode URL params correctly for SourceId in Cargo.lock (rust-lang/cargo#12280)
- docs: format config override caveat as a note (rust-lang/cargo#12392)
- credential provider implementation (rust-lang/cargo#12334)
- feat(crates-io): expose HTTP headers and Error type (rust-lang/cargo#12310)
- chore: Don't update test data (rust-lang/cargo#12380)
- fix: only skip mtime check on `~/.cargo/{git,registry}` (rust-lang/cargo#12369)
- Update docs for artifact JSON debuginfo levels. (rust-lang/cargo#12376)

Since rust-lang/cargo#12334 makes built-in credential providers part of the cargo binary, it's no longer needed to build them in bootstrap.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jul 26, 2023
Update cargo

8 commits in 1b15556767f4b78a64e868eedf4073c423f02b93..7ac9416d82cd4fc5e707c9ec3574d22dff6466e5
2023-07-18 14:44:47 +0000 to 2023-07-24 14:29:38 +0000
- fix(cargo-credential): should enable feature `serde/derive` (rust-lang/cargo#12396)
- fix: encode URL params correctly for SourceId in Cargo.lock (rust-lang/cargo#12280)
- docs: format config override caveat as a note (rust-lang/cargo#12392)
- credential provider implementation (rust-lang/cargo#12334)
- feat(crates-io): expose HTTP headers and Error type (rust-lang/cargo#12310)
- chore: Don't update test data (rust-lang/cargo#12380)
- fix: only skip mtime check on `~/.cargo/{git,registry}` (rust-lang/cargo#12369)
- Update docs for artifact JSON debuginfo levels. (rust-lang/cargo#12376)

Since rust-lang/cargo#12334 makes built-in credential providers part of the cargo binary, it's no longer needed to build them in bootstrap.
@ehuss ehuss added this to the 1.73.0 milestone Jul 30, 2023
@weihanglo weihanglo deleted the asymmetric-challenge branch August 3, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-interacts-with-crates.io Area: interaction with registries A-registries Area: registries S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants