fix(login): Deprecate CLI token

[epage]

What does this PR try to resolve?

This came up in #13623 to avoid putting tokens into shell history.

How should we test and review this PR?

The exact approach to deprecation can vary

• Include <token> in at least some docs for discovery (most likely the man page)
• Don't warn yet

etc

I also suspect we could reorganize cargo help login but wanted to decouple that from this change.

Additional information

[rustbot]

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[weihanglo]

This looks good to me. Have we had any consensus or FCP for this change?

[epage]

Have we had any consensus or FCP for this change?

I do not believe we've discussed this as a team yet. Open to FCP or team meeting discussion.

[Turbo87]

FWIW I've seen @woodruffw use https://developer.1password.com/docs/cli/reference/commands/read/ to use command line options like this in a seemingly safe way. I wonder if we really need to deprecate this option, or if a strong warning in the documentation/help text would be sufficient.

though to be fair, this is probably irrelevant for the cargo login case, since CI systems should probably use env vars to pass in the token instead of saving it via cargo login and regular users can use stdin and are unlikely to regularly call cargo login for this to matter much 😅

[weihanglo]

I wonder if we really need to deprecate this option, or if a strong warning in the documentation/help text would be sufficient.

Do you suggest keeping doc/helptext for discoverable reasons? I am fine with that as well :)

[epage]

I feel like cargo login -h help output would need to be carefully tweaked to avoid people seeing [TOKEN] and immediately thinking thats what they need. I'm not even sure thats possible as we've already anchored the users conversation with the help by the usage / argument placeholder column.

[woodruffw]

FWIW I've seen @woodruffw use developer.1password.com/docs/cli/reference/commands/read to use command line options like this in a seemingly safe way. I wonder if we really need to deprecate this option, or if a strong warning in the documentation/help text would be sufficient.

Yep, I frequently use that or similar (e.g. $(gh auth token) for GitHub)!

Ultimately, passing secrets between processes without an actual authenticated IPC channel/system secret manager is pretty difficult to do in a completely secure way 😅 -- even cargo login $(op read ...) can be sniffed by any other user-level process via /proc/<pid>/cmdline, as can any environment variable via /proc/<pid>/environ. Whether or not those are considered attacker-readable depends on the threat model.

IMO it'd be reasonable to nudge users towards providing tokens via stdin, while still allowing people to pass it via argv (perhaps with a loud warning about the risk of token exposure when passing a literal token rather than via a subshell?).

[weihanglo]

To clarify, because of the stability guarantee Cargo cannot remove the flag (see #13623 (comment)). Deprecation here means, well, a big warning 😆.

[woodruffw]

To clarify, because of the stability guarantee Cargo cannot remove the flag (see #13623 (comment)). Deprecation here means, well, a big warning 😆.

Ah, sorry for the confusion! I badly misread the diff as actually removing the positional, not merely hiding it 😅

[Turbo87]

To clarify, because of the stability guarantee Cargo cannot remove the flag (see #13623 (comment)). Deprecation here means, well, a big warning 😆.

I wasn't aware either that the plan is not to remove the flag. The word "deprecated" to me means that it would go away in a future version.

I think I would personally prefer the second option mentioned in #13623 (comment):

help message mentioning stdin as an alternative for more secure operation

[weihanglo]

@rfcbot fcp merge

This PR deprecates the optional <token> argument in cargo login, in the sense of:

• Remove the mention of the <token> argument from cargo login help text and doc.
• Give a warning when the <token> argument is used

  warn: cargo login <token>` is deprecated in favor of reading `<token>` from stdin`.
  

Folks, take your time to check in boxes. This is not an urgent FCP.

[rfcbot]

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Eh2406
• @Muscraft
• @Rustin170506
• @arlosi
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

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.

[provrb]

Useful change. LGTM

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔