Open auspicacious opened 6 months ago
Note that the Cargo book shows the use of stdin, see https://doc.rust-lang.org/cargo/reference/publishing.html#before-your-first-publish
We can't remove the argument due to our compatibility guarantees.
Possible improvements from where we are at:
cargo login -h
with one of
[token]
Also, in case you aren't aware, the token is stored in plain text. See https://doc.rust-lang.org/cargo/reference/registry-authentication.html for configuring an alternative storage. I'm working to deprecate us defaulting to plain text storage in #13343
Also, in case you aren't aware, the token is stored in plain text.
Yes, that is clear from the documentation, but there is still a significant difference between a single, well-known file with restricted permissions and the many unknowns that putting something on the command-line brings.
Thanks for linking to the registry -- I inferred while searching for other bugs that Cargo had support for other storage mechanisms but hadn't looked into it. (Which is good, and makes it even more compelling to deprecate this pattern!)
We can't remove the argument due to our compatibility guarantees.
[token]
could be logging that token to the CI system's build log. For many open-source projects, those logs may in fact be public. CircleCI operates this way. If I remember correctly, they make an effort to recognize and purge secrets from the log. However, this is a very fragile layer of defense in depth, indeed.Possible improvements from where we are at:
I think that both of these are reasonable if worded well. However, that is predicated on the decision that the compatibility guarantee trumps security issues and we have to work around it. I'm not entirely convinced of that.
Is it possible that the other, related security issue in #7813 will be fixed soon? It would be better to be able to update the Rust and Cargo books to show a reasonably secure pattern.
In other words, personally, if I were to update the documentation, the only way I would be comfortable writing it today would be the defensive Bash formula I mentioned above:
IFS='' read -r -s token
cargo login <<< "$token"
unset token
because that pattern will not echo to the console.
Is there a list somewhere to track deprecated things that should be removed in the next major version of Cargo?
The version of Cargo follows the release of Rust itself, hence unlikely to have major version bump unless Rust also did that. That being said, Cargo could and has been phasing out deprecated features slowly.
The other alternative is having asymmetric token being used by default, and phase out plain text token. That is tracked in https://github.com/rust-lang/cargo/issues/10519.
The other alternative is having asymmetric token being used by default, and phase out plain text token.
Wow, yes, having asymmetric tokens will solve many of the problems of symmetric tokens. And I got to learn about PASETO.
That being said, Cargo could and has been phasing out deprecated features slowly.
Okay, so it's not exactly strict SemVer, but hopefully something a bit closer to Git's concept of "porcelain" and "plumbing" commands.
I'll summarize the work that's been discussed here. 1, 2, 3 probably will not affect Cargo's CLI in a backwards-incompatible way, but 4, 5, 6 probably will. This looks like rough chronological order to me.
cargo login -h
, and cargo help login
to deprecate the passing of tokens on the command line. (I would personally prefer that these docs somehow reference #7813 as an acknowledgement that this is still a work in progress)There's a lot of good news in that list!
The scope of this issue is, I think, limited to 1.
If it's OK with the others on this thread, I will try to take on the documentation changes myself. Given the evidence that much more significant improvements are on the way, I will try to keep the documentation focused on discouraging passing tokens on the command-line and avoid introducing the Bash formula that I mentioned above.
Does that sound reasonable, @epage @weihanglo ?
@auspicacious Nice write-up!
I'd like to correct my words. Cargo is guaranteed not to have breaking changes, pretty much SemVer (?). Usually only when them are identified as bugs or security vulnerabilities will we remove them, after a long deprecation period, after we scratched our head and couldn't come up with any better solution. Things in Cargo.toml might usually be fixable across edition boundaries. However, CLI breakage are generally not acceptable, as there is no way to check editions of a CLI flag now.
Back to the plan. Looks pretty great. However, as you said, 4, 5, and 6, especially 5 and 6, are not backward-compatible, so unlikely to execute them.
Sorry for giving you a wrong impression of how the project handle deprecations.
I've created rust-lang/book#3866 to track the Book side of this.
At this point, it sounds like this issue is only about cargo login -h
. Is that correct?
@epage Thanks, it looks like the Book authors will take up the edit.
At this point, it sounds like this issue is only about
cargo login -h
. Is that correct?
cargo login -h
and cargo help login
(the manpage) will both need to be updated, I think. The manpage uses neutral language right now, and should probably be updated to discourage use.
@weihanglo
Usually only when them are identified as bugs or security vulnerabilities will we remove them, after a long deprecation period ... as you said, 4, 5, and 6, especially 5 and 6, are not backward-compatible, so unlikely to execute them.
I must continue to argue that this is a security vulnerability and that deprecation and removal is necessary. If you look at the most common and heavily scrutinized programs that deal with secret tokens, such as sudo
, gpg
, or cryptsetup
, it would be unthinkable for them to support secret input on the command-line in the way that Cargo does. sudo
and gpg
, in fact, have elaborate subsystems dedicated to passphrase input.
Cargo is a critical part of supply-chain security and should be held to the same standard -- not to mention that it sets an example for other Rust programs and programmers.
To that end, it seems like there is a lot of progress happening. Asymmetric tokens will hopefully bring many benefits, but those benefits can only be realized by removing the old system, and I hope that the old system is removed someday.
I understand if the scope of this GitHub issue should be limited to changes in the documentation. However, this seems like a situation where breaking the CLI and removing this functionality would actually help users, by alerting them to and protecting them from, for instance, the potential CI system leaks that I mentioned above.
I must continue to argue that this is a security vulnerability and that deprecation and removal is necessary
If this is a vulnerability, it should not be discussed here but reported through the standard reporting process. They can make an appropriate determination. See https://www.rust-lang.org/policies/security
If this is a vulnerability, it should not be discussed here but reported through the standard reporting process. They can make an appropriate determination. See https://www.rust-lang.org/policies/security
Fair enough. I sent a message after this comment. The team has been in contact, but I am waiting for a determination.
From the Rust Security Response WG. I've highlighted a suggestion they made that I don't think has been mentioned elsewhere:
While we agree that the current implementation and design of cargo login is sub-optimal, we don't see it as a security vulnerability requiring a coordinated disclosure and fix. The approach outlined in the issue to update the documentation and help messages, plus a deprecation warning shown when passing the token through the CLI discouraging its use, sounds good to us.
We leave the decision on whether to eventually remove support for passing the token to the CLI to the Cargo team.
Problem
I am new to Rust and working through the Rust Book.
cargo login
's default syntax encourages users to pass their secret registry authentication token on the command line, e.g.cargo login [token]
. The Rust Book also encourages this syntax.The
cargo login
manpage makes it clear that this value requires protection:However, in general, passing any secret as a command-line argument creates significant and unnecessary security risk. It is significant for at least these reasons:
cargo login
command is running, its arguments are visible to all users on the system. A malicious user monitoring processes could easily steal the token.And it is unnecessary because there are other options available that do not create this risk:
cargo login
could be modified to prompt the user for input, using normal shell conventions for entering a password, and not echoing text back to the console. #7813 touches on this.pass
and pipe it intocargo login
to avoid exposure.cargo login
, if usingbash
, and possibly some other shells, the user could use this combination of commands to avoid exposure:I am honestly surprised that I could not find any duplicate issues for this, and I apologize if I have missed one that explains the rationale behind the current design. Likewise, I am not using the security vulnerability disclosure process for this bug report, as it is based entirely on public and obvious information.
However, I do believe that this is insecure by design, and encourages Rust developers to adopt patterns that are widely known to be insecure.
Steps
No response
Possible Solution(s)
No response
Notes
No response
Version
No response