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

Asymmetric tokens #10771

Merged
merged 22 commits into from
Dec 29, 2022
Merged

Asymmetric tokens #10771

merged 22 commits into from
Dec 29, 2022

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Jun 18, 2022

Builds on and is blocked by #10592. This adds initial support for Asymmetric Tokens #10519.

@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 18, 2022
@Eh2406
Copy link
Contributor Author

Eh2406 commented Jun 18, 2022

Big shout out to @brycx for all the work getting pasetors ready for this PR! Literally could not have done this without your help!

@Eh2406 Eh2406 force-pushed the asymmetric_tokens branch 2 times, most recently from ae6834c to 429cfe5 Compare June 20, 2022 15:45
@Eh2406
Copy link
Contributor Author

Eh2406 commented Jun 20, 2022

@joshtriplett how aggressively do we want to tie this to registry_auth? I think we want them tied together when we stabilize it, but we could choose to be more aggressive and only have one Z flag.

@joshtriplett
Copy link
Member

@Eh2406 I think we should make them the same feature, since we shouldn't ship authenticated HTTP registries with non-asymmetric tokens.

@joshtriplett joshtriplett added the T-cargo Team: Cargo label Jun 21, 2022
@joshtriplett
Copy link
Member

Capturing an idea from the Cargo team meeting: we could make the asymmetric token a private field (and make sure the enum doesn't implement Display), so that it can't be read out and unintentionally transmitted; signing can use methods.

}
}

pub struct Mutation<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Drive-by comment: this would probably be better as an enum.

@bors
Copy link
Contributor

bors commented Jul 1, 2022

☔ The latest upstream changes (presumably #10802) made this pull request unmergeable. Please resolve the merge conflicts.

@Eh2406
Copy link
Contributor Author

Eh2406 commented Sep 22, 2022

I just noticed that we have a check for setting the token using the command line, this check should probably be extended to the secret key as well.

bail!("registry.token cannot be set through --config for security reasons");

@ehuss
Copy link
Contributor

ehuss commented Nov 19, 2022

With #10592 landed, is there anything needed to be ready for review other than resolving the merge conflicts?

@Eh2406
Copy link
Contributor Author

Eh2406 commented Nov 19, 2022

It needs to be rebased and updated. I remember that there was significant feedback. I'm planning to come back to this PR the end of this week.

Todos:

  • rebase and merge conflicts.
  • update to pasetors 0.6, use the new serde support.
  • Consistently refer to things as asymmetric tokens
  • consistently refer to secret keys and public keys, as that is the recommended terminology for PASETO. (Public vs. Private is easy to accidentally misspeak or type.)
  • add to registry_auth Z flag
  • try to make the asymmetric token a private field (and make sure the enum doesn't implement Display)
  • Mutation should be an enum
  • a check for setting the secret key using the command line
  • cash tokens so that the same token is not regenerated

Copy link
Contributor

@arlosi arlosi left a comment

Choose a reason for hiding this comment

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

Could you also update unstable.md to describe the feature?

src/cargo/sources/registry/http_remote.rs Outdated Show resolved Hide resolved
src/cargo/util/auth.rs Show resolved Hide resolved
crates/cargo-test-support/src/registry.rs Outdated Show resolved Hide resolved
crates/cargo-test-support/src/registry.rs Outdated Show resolved Hide resolved
crates/cargo-test-support/src/registry.rs Outdated Show resolved Hide resolved
@ehuss
Copy link
Contributor

ehuss commented Dec 29, 2022

🎉 Thanks! I appreciate that you clearly put in a lot of effort on this.

Can you collect any todo items, and create new issues for them? I think it would help to track what is left, what needs to be decided or resolved, and to organize discussion on those.

@bors r+

@bors
Copy link
Contributor

bors commented Dec 29, 2022

📌 Commit b6adac1 has been approved by ehuss

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 Dec 29, 2022
@bors
Copy link
Contributor

bors commented Dec 29, 2022

⌛ Testing commit b6adac1 with merge ef565ef...

bors added a commit that referenced this pull request Dec 29, 2022
Asymmetric tokens

Builds on and is blocked by #10592. This adds initial support for Asymmetric Tokens #10519.
@bors
Copy link
Contributor

bors commented Dec 29, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 29, 2022
@Eh2406
Copy link
Contributor Author

Eh2406 commented Dec 29, 2022

That seams spurious and unrelated.

@Eh2406
Copy link
Contributor Author

Eh2406 commented Dec 29, 2022

@bors retry

@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 Dec 29, 2022
@bors
Copy link
Contributor

bors commented Dec 29, 2022

⌛ Testing commit b6adac1 with merge 7fb01c6...

@bors
Copy link
Contributor

bors commented Dec 29, 2022

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 7fb01c6 to master...

@bors bors merged commit 7fb01c6 into rust-lang:master Dec 29, 2022
@bors bors mentioned this pull request Dec 29, 2022
16 tasks
weihanglo added a commit to weihanglo/rust that referenced this pull request Jan 4, 2023
8 commits in 2381cbdb4e9b07090f552d34a44a529b6e620e44..8c460b2237a6359a7e3335890db8da049bdd62fc
2022-12-23 12:19:27 +0000 to 2023-01-04 14:30:01 +0000
- test: revive nightly plugin tests to work (rust-lang/cargo#11534)
- Add note to release notes about rejecting multiple registries. (rust-lang/cargo#11531)
- Fix a typo `fresheness` -&gt; `freshness` (rust-lang/cargo#11529)
- Reasons for rebuilding (rust-lang/cargo#11407)
- Asymmetric tokens (rust-lang/cargo#10771)
- Use proper git URL for GitHub repos (rust-lang/cargo#11517)
- Add `registry.default` example (rust-lang/cargo#11516)
- Support vendoring with different revs from same git repo (rust-lang/cargo#10690)

Also update license exceptions and permitted dependencies
for new cargo dependency "pasetors".

A new dependency `getrandom` is added into `rustc-workspace-hacks`,
since it requires feature `js`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 4, 2023
Update cargo

8 commits in 2381cbdb4e9b07090f552d34a44a529b6e620e44..8c460b2237a6359a7e3335890db8da049bdd62fc
2022-12-23 12:19:27 +0000 to 2023-01-04 14:30:01 +0000
- test: revive nightly plugin tests to work (rust-lang/cargo#11534)
- Add note to release notes about rejecting multiple registries. (rust-lang/cargo#11531)
- Fix a typo `fresheness` -&gt; `freshness` (rust-lang/cargo#11529)
- Reasons for rebuilding (rust-lang/cargo#11407)
- Asymmetric tokens (rust-lang/cargo#10771)
- Use proper git URL for GitHub repos (rust-lang/cargo#11517)
- Add `registry.default` example (rust-lang/cargo#11516)
- Support vendoring with different revs from same git repo (rust-lang/cargo#10690)

Also update license exceptions and permitted dependencies
for new cargo dependency "pasetors".

A new dependency `getrandom` is added into `rustc-workspace-hacks`,
since it requires feature `js`.

r? `@ghost`
@ehuss ehuss added this to the 1.68.0 milestone Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants