-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Cargo alternative registry auth #3139
Conversation
This seems reasonable. The RFC should explicitly state that authorization headers are only provided for |
Also, the RFC suggests that there's nowhere to pass this information for git, but if you're using git via |
… on https requirements
Thanks for taking a look! I've updated the RFC to include more details on requiring HTTPS, as well as a section on how we might handle adding an Authorization header for git over https in the future. |
How will credentials be stored? Will they be stored based on the registry name, or the registry URL? (I think it should be based on the registry URL). (I saw the section about |
When stored in a file (not using |
Is there potential for abuse here? Imagine that Alice has a [registries.mycustom]
token = "secretkey for mycustom registry" And she has a shared code repository with a version controlled [registries]
mycustom = { index = "https://my-intranet:8080/git/index" } Now what happens if a malicious user (who had commit access to the repo) makes the following change to the config.toml file: [registries]
mycustom = { index = "https://my-evil-server:8080/git/index" } The next time Alice does a (It should be noted that a malicious user with commit access can steal secrets in many other ways, but maybe we could see if this particular case can be handled) |
The malicious user would have to correctly guess what the registry was called locally, but that seems completely plausible. I believe this attack would also work with stable cargo for the existing authenticated operations such as If we want to do something here (such as storing the credentials by index URL), it should probably be handled via a separate RFC. |
If the I don't think this issue is blocking for the RFC (to handling it elsewhere sounds fine to me) |
Oh, of course that would work 🤦♂️. |
Thank you for pointing this out!
For example adding a dependency on a library with a build script that sends all of
I think the discussion of if this particular attack is worth fixing, given that the attacker is already in, belongs in its own thread. |
…potential confusion
Sorry for the slowness of the responses here, there is a vast amount I do not know about these topics. My instinct is to read about all the possibilities before putting my foot in my mouth. I have bean thinking about how to replace this protocol when (if) it gets replaced with something better. For example how can a server say "I don't want the plain token, I want that new post-quantum hardware based crypto signature that Cargo started recommending in RFC-98765". Obviously, we can put that off for RFC-98765 to decide. But I wonder if it is worth future proving this RFC, with something like "if the 401 has the 'BIKE_SHED' header set and not set to '1' then Cargo will error" |
I thought about the versioning issue a bit more, and I think it makes more sense for the client (Cargo) to set a request header with the protocol version, such as "Cargo-Protocol-Version: 1". Then the server can send back the HTTP 401 if it just needs authentication. Otherwise, if the server can't support the requested Version, it could send back an HTTP 400 (Bad Request), with details in the response body. Requiring the server to include identifying information (such as a protocol version), could also identify the server as hosting a cargo registry before authentication, which may be undesirable. |
We (the Cargo Team) have been reviewing and discussing this RFC. While it would provide an MVP of registry authentication, there are some additional properties we'd like to have, which we believe would substantially improve the security of the Cargo ecosystem by raising the "baseline" security of registries. This makes much more of a difference if all authenticated registries have that baseline, because if these properties are optional, and configuration ever led Cargo to treat a registry as not having them, that could leak secret information. Based on some review with security experts, we think it should be possible to achieve these properties with minimal additional complexity. This comment will go over the new properties (highlighting the key ones and leaving the rest available for reference under the expandable "Other properties"), and then present a proposed solution that provides these properties. We're hoping a variation on this proposal will work for everyone who needs authenticated registries, without being a big imposition. If you're building a registry and there's a critical property we haven't thought of, which would make this solution a non-starter for you, please let us know. Otherwise, we think it'd be worth adapting the RFC to propose this solution (and document these requirements). Key additional design properties for Cargo registry authentication
Other properties
Proposed ImplementationBased on discussion of these properties and potential implementations with security experts, we think it's possible to achieve all of these properties by using somthing like the PASETO v4.public format. Cargo will have a user ID and public/private key pair associated with each registry. (Generated and registered however one currently gets a token from the registry.) When authenticating to a registry, Cargo will generate a PASETO in the v4.public format, with specific "claims" (key-value pairs in the PASETO's JSON payload). The claims within the PASETO will include at least the current time and expiration time (1 hour to allow for clock skew). (We can also consider adding additional claims to bind the token to, such as CA fingerprint.) The "footer" will include a user ID and the registry base URL. The registry server will validate the PASETO, and check the footer and claims:
There are not currently implementations of PASETO v4.public for Rust, but I have bean told by the maintainers of both Rust PASETO libraries that it is coming in the next month or so. The point of an RFC is to discuss. Does this meet our suggested properties? Are there issues with the properties we're proposing? Are there other properties we should make sure to account for? Are there other claims we should bind tokens to? Are there ways to make it better? |
PASETO is a fine choice. That said, I'll note one potential drawback is v4 removed support for ECDSA as a digital signature algorithm. This is mainly noteworthy if you would like to support using hardware tokens for private key storage (I say this having written many actively maintained pure Rust bindings to various hardware tokens). The landscape there looks a little like:
ECDSA would let anyone who owns any "non-blue" Yubikey made in the past 5 years or a recent Mac use hardware-backed key storage, with optional PIN/TouchID protection. That said Ed25519 is certainly a fine algorithm choice and eventually should become more ubiquitous in regard to hardware token support going forward. In selecting a new token format though, there's a somewhat related issue worth considering: crates.io token scopes (RFC #2947. On that RFC, I suggested using the Biscuit credential format: https://github.com/CleverCloud/biscuit Biscuit is a post-1.0 credential format based on public key cryptography which provides an expressive and flexible policy logic exposed as a sort of query engine for making access control decisions. It also has powerful delegation features which would make it possible to do things like give a build system time-limited access to build and publish a new release of a particular version of a particular crate, ideas I covered in a Bay Area Rust Meetup talk in 2017 (albeit using Macaroons in that case). Biscuit's principal implementation is in Rust. There are additional implementations in Java and Go. One drawback of trying to deploy Biscuit right now is that there are breaking changes underway in the form of Biscuit 2.0. The changes largely relate to simplifying the cryptography, removing some fancier cryptography which made for more compact credentials and replacing it with a more simple and boring approach which should ease writing additional implementations in other languages. All that said, excited to see this move forward, and I will be interested in writing some hardware token integrations for this feature when it ships. cc @Geal |
Biscuit would indeed fit well in that model: instead of registering a public key over to the registry, the registry gives a token validated by its own root key, giving access to your crates. Before any request, the credential process would attenuate (create a new token with less rights) that token to add an expiration date and bind it to the URL. The server does not need to look up a key, only validate the token (and check revocation ids). Biscuit 2.0 should be released soon, only cosmetics change remain. The various implementation status: Rust (2.0 done, not released yet), Go (1.0), Java (in progress on 2.0), Haskell (2.0), Swift (in progress on 2.0) and C bindings to the Rust library. I'd be happy to help adapt it in registries, otherwise yes, Paseto would work :) |
Will P-384 satisfy support for hardware security tokens? If so, PASETO v3.public may also be worth considering. |
YubiKeys support P-384. However, the macOS SEP does not. |
With EdDSA coming to FIPS 186-5 (currently still a draft), Ed25519 is probably the correct choice in the long-term. Both PASETO (v4.public) and Biscuit (2.0) support that algorithm, and its anticipated inclusion in FIPS means hardware security tokens are incentivized to support it too. I think Ed25519 support will become ubiquitous. In the short term, if ECDSA support is needed (i.e. for hardware security tokens that people own today), PASETO v3.public gives us ECDSA over P-384. This doesn't help with MacOS SEP, but it covers a lot of hardware security tokens that people currently own and use, and is already specified and, in a few programming languages, already implemented. Biscuit 2.0 doesn't currently support ECDSA at all, but I see @tarcieri opened an issue recommending P-256 support in Biscuit for this exact use case. From my perspective, the choice boils down to something like this:
Given all of what I've written above, I think both proposals are reasonable. Ed25519 is a good signature algorithm, and both options use it. If hardware security token support is a major concern, then that may inform a decision between the two today (although if the industry settles on Ed25519 in the coming years, it may eventually be a moot point). Note: I haven't had the chance to dig into Biscuit's design in detail. It's entirely possible I'm overlooking something important. This is just an initial analysis of the cryptographic algorithms offered. |
I didn't have the time to evaluate the proposal fully yet, but it seems to be missing easy support for token scanning. Crates.io tokens are hardcoded to start with The way to identify the tokens as being registry tokens must also be configurable, i.e. it should not be an hardcoded |
@pietroalbini a leading prefix seems like something it's easy to add/remove from the encoded form prior to handing it off to a library that implements a particular token format for decoding or immediately after encoding |
Definitely! We just need to make sure Cargo recognizes it and strips it. |
There are 2 parts we may want to tag.
On a different note, for existing registry providers, do we need to specify in the RFC a way to transition existing customers? Or can you just make a breaking change with the new behavior? |
Thanks for discussing the RFC! I added a paragraph to reference the ongoing work to improve token security and make it clear that this RFC doesn't specify the token format. |
@rfcbot merge |
Team member @Eh2406 has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
Thanks for the clarification! This should help make it clear that this RFC does not obligate cargo to support shared-secret bearer tokens for authentication to private registries. With that change, I think this RFC is ready to merge. |
This RFC does not specify or change the format of the Authorization Token. For the purposes of this RFC, tokens are opaque; no particular format or protocol is specified, and third-party registry authentication should not assume support for any particular format. This includes shared-secret tokens, even though crates.io and the existing publish support for third-party registries currently supports such bearer tokens. Future RFCs (such as [RFC2789](https://github.com/rust-lang/rfcs/pull/3231)) may update the format and protocol used for tokens. | ||
|
||
## Interaction with HTTP registries | ||
The approved (but currently unimplemented) [RFC2789](https://github.com/rust-lang/rfcs/pull/2789) enables Cargo to fetch the index over HTTP. When fetching `config.json` from an HTTP index, if Cargo receives an `HTTP 401` response, the request will be re-attempted with the Authorization header included. If no authorization token is available, Cargo will suggest that the user run `cargo login` to add one. The `HTTP 401` response from the registry server may also include an `X-Cargo-Token-Url: ` header to specify where the user should go to get a token. In that case, `cargo` can display a more helpful message such as "please paste the Token found on https://example.com/token-url-from-header below" |
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.
Can we replace X-Cargo-Token-Url
with a more generic X-Cargo-Authentication-Error-Message
or something of that sort?
This RFC does not specify or change the format of the Authorization Token. For the purposes of this RFC, tokens are opaque; no particular format or protocol is specified, and third-party registry authentication should not assume support for any particular format. This includes shared-secret tokens, even though crates.io and the existing publish support for third-party registries currently supports such bearer tokens. Future RFCs (such as [RFC2789](https://github.com/rust-lang/rfcs/pull/3231)) may update the format and protocol used for tokens. | ||
|
||
## Interaction with HTTP registries | ||
The approved (but currently unimplemented) [RFC2789](https://github.com/rust-lang/rfcs/pull/2789) enables Cargo to fetch the index over HTTP. When fetching `config.json` from an HTTP index, if Cargo receives an `HTTP 401` response, the request will be re-attempted with the Authorization header included. If no authorization token is available, Cargo will suggest that the user run `cargo login` to add one. The `HTTP 401` response from the registry server may also include an `X-Cargo-Token-Url: ` header to specify where the user should go to get a token. In that case, `cargo` can display a more helpful message such as "please paste the Token found on https://example.com/token-url-from-header below" |
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.
On the name X-Cargo-Token-Url
:
- Per IETF RFC 6648,
x-
prefixes are deprecated. - The convention in mixed-case header names is to spellings like “URL” rather than “Url”. (In HTTP/2, it’s all lowercase, and increasingly people are using all lowercase even in HTTP/1, but for some reason even new headers still seem to be consistently defined in mixed case.)
- Also the colon-space present in the text at present is inappropriate, not being part of the header field name.
I would suggest Cargo-Token-URL
as the header field name to begin with, if that’s the semantics stayed with. I agree with @johnterickson that it might be better to just have an arbitrary error message rather than assuming it’ll be in the form of a URL.
I also then feel like noting that there’s kind of already a place for this: the WWW-Authenticate
header, which is already required on 401 responses (RFC 7235, §4.1). I think it would not be unreasonable to define a new challenge scheme Cargo
, with a parameter url
or message
or some such thing—perhaps even allow either, with message
being more flexible but url
potentially allowing greater tooling assistance. One could argue this is a mild abuse of the system since this scheme wouldn’t ever be used in the Authorization
request header but only the WWW-Authenticate
response header, but I think it’s pretty reasonable. All up, a real example could look like this:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: Basic realm="example.com", Cargo url="https://example.com/token-url-from-header"
In the end, I don’t think there’s a great deal to choose between the two approaches (custom header or custom challenge scheme). But no X-
, please.
This design was based on my understanding that Cargo needs to read Cargo needs to read This RFC, as merged, adds one round trip to HTTP indexs. The round trip to get the |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
Huzzah! The @rust-lang/cargo team has decided to accept this RFC. To track further discussion, subscribe to the tracking issue here: |
Can the RFC be updated to have a link to the tracking issue? rust-lang/cargo#10474 |
@Eh2406 Sorry, I'm confused. The link should be at the top of the RFC: https://rust-lang.github.io/rfcs/3139-cargo-alternative-registry-auth.html |
Sorry, It is. I don't know why I did not see that before. Sorry. |
This RFC adds authorization for alternative registries, taking in to account the recent developments in HTTP-based registries (RFC2789).
Zulip discussion
Internals thread
Rendered