Skip to content

Conversation

omskscream
Copy link

@omskscream omskscream commented Sep 23, 2025

FCP comment

What does this PR try to resolve?

Part of #15274
This PR adds support for reading auth token from stdin as an alternative to the existing --token command line option. This is needed to deprecate the mentioned --token option.
Which is, in turn, needed to avoid tokens being saved in shell history.

How to test and review this PR?

Please check added test case in first and second commits to see how behavior changes.
(I hope I understand the "tests and commits" guidance better this time 🤔)

r? @epage

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. A-cli-help Area: built-in command-line help A-documenting-cargo-itself Area: Cargo's documentation Command-publish S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 23, 2025
@omskscream omskscream force-pushed the publish/read-token-from-stdin branch from cb6b55b to 0010ead Compare September 23, 2025 19:26
.get_one::<String>("token")
.cloned()
.or_else(|| {
if std::io::stdin().is_terminal().not()
Copy link
Author

Choose a reason for hiding this comment

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

I would really like to know why most windows-based and one mac-based "Test" checks were terminated in Github before I added this not a terminal condition here like in similar implementation in login command. Process hanged waiting input with no input provided? 🤔

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

(I hope I understand the "tests and commits" guidance better this time 🤔)

Yes that seems correct :)

And thank you for the contribution.

@omskscream omskscream force-pushed the publish/read-token-from-stdin branch from 0010ead to a6972e5 Compare September 24, 2025 13:56
@omskscream omskscream force-pushed the publish/read-token-from-stdin branch from a6972e5 to 1a213cb Compare September 24, 2025 14:00
@weihanglo weihanglo added the T-cargo Team: Cargo label Sep 24, 2025
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks. I'll do an FCP now.

@weihanglo
Copy link
Member

@rfcbot fcp merge

I'd like to propose we

  • (this PR) Support reading the registry token from stdin for cargo publish
  • (future PR) Deprecate the --token option in cargo publish in terms of
    • Remove the mention of the --token option from help text and doc.
    • Give a warning when the --token option is used

Note that cargo login already does it since 1.86 and an FCP was done: #15057 (comment).

@rust-rfcbot
Copy link
Collaborator

rust-rfcbot commented Sep 24, 2025

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

Concerns:

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.

@rust-rfcbot rust-rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Sep 24, 2025
@epage
Copy link
Contributor

epage commented Sep 24, 2025

@weihanglo
Copy link
Member

https://github.com/rust-lang/cargo/actions/runs/17979102378/job/51140134992?pr=16001

#7 [ 2/10] RUN apk add --no-cache openssh git
#7 0.138 fetch https://dl-cdn.alpinelinux.org/alpine/v3.22/main/x86_64/APKINDEX.tar.gz
#7 0.284 fetch https://dl-cdn.alpinelinux.org/alpine/v3.22/community/x86_64/APKINDEX.tar.gz
#7 0.563 (1/22) Installing brotli-libs (1.1.0-r2)
#7 0.585 (2/22) Installing c-ares (1.34.5-r0)
#7 0.601 (3/22) Installing libunistring (1.3-r0)
#7 0.626 (4/22) Installing libidn2 (2.3.7-r0)
#7 0.642 (5/22) Installing nghttp2-libs (1.65.0-r0)
#7 0.658 (6/22) Installing libpsl (0.21.5-r3)
#7 0.673 (7/22) Installing zstd-libs (1.5.7-r0)
#7 0.693 (8/22) Installing libcurl (8.14.1-r1)
#7 0.712 (9/22) Installing libexpat (2.7.2-r0)
#7 0.727 (10/22) Installing pcre2 (10.43-r1)
#7 0.746 (11/22) Installing git (2.49.1-r0)
#7 0.820 (12/22) Installing git-init-template (2.49.1-r0)
#7 0.835 (13/22) Installing openssh-keygen (10.0_p1-r8)
#7 0.853 (14/22) Installing ncurses-terminfo-base (6.5_p20250503-r0)
#7 0.871 (15/22) Installing libncursesw (6.5_p20250503-r0)
#7 0.888 (16/22) Installing libedit (20250104.3.1-r1)
#7 0.904 (17/22) Installing openssh-client-common (10.0_p1-r8)
#7 0.933 (18/22) Installing openssh-client-default (10.0_p1-r8)
#7 0.953 (19/22) Installing openssh-sftp-server (10.0_p1-r8)
#7 0.969 (20/22) Installing openssh-server-common (10.0_p1-r8)
#7 0.984 (21/22) Installing openssh-server (10.0_p1-r8)
#7 1.014 (22/22) Installing openssh (10.0_p1-r8)
#7 1.031 Executing busybox-1.37.0-r18.trigger
#7 1.037 OK: 26 MiB in 38 packages
#7 DONE 2.9s

#8 [ 3/10] RUN ssh-keygen -A
#8 0.122 OpenSSL version mismatch. Built against 3050003f, you have 30500010
#8 ERROR: process "/bin/sh -c ssh-keygen -A" did not complete successfully: exit code: 255

A fix is on the way: #16010

…icitly when `login` is run before `publish`

(login uses token creds for simplicity)
@epage
Copy link
Contributor

epage commented Sep 24, 2025

@rfcbot resolve behavior-change

Copy link
Member

@0xPoe 0xPoe left a comment

Choose a reason for hiding this comment

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

Thanks! :shipit:


This command requires you to be authenticated with either the `--token` option
or using {{man "cargo-login" 1}}.
Instead of `--token` option, _token_ value might also be provided via stdin.
Copy link
Contributor

Choose a reason for hiding this comment

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

I was a little confused reading this. I'm wondering if it would help to be more explicit about only reading stdin if it is not a terminal. Maybe something like this?

Suggested change
Instead of `--token` option, _token_ value might also be provided via stdin.
Instead of using the `--token` option, the _token_ value may be piped in via stdin.

@ehuss
Copy link
Contributor

ehuss commented Sep 29, 2025

@rfcbot concern heuristics

I worry that this is making a heavy assumption that in non-interactive environments stdin is connected to /dev/null (or equivalent), thus reading an "empty" token. I don't know how universally that works. I believe things like cron set stdin to /dev/null, but do all environments do the same? What about the myriad CI environments? What about various Windows environments? I worry that cargo publish could get stuck in some situations trying to read from stdin. IsTerminal uses a bunch of heuristics that may not always be correct.

I also worry that cargo publish won't get much widespread testing on nightly. If it hits stable, and then people have problems, what do we do? Once it is stable, we can't (easily) just revert it.

I understand there is some concern that there would be a lack of symmetry with cargo login, but I think cargo login is a fundamentally different command or action than cargo publish.

Can we consider some explicit means of saying "read from stdin"?

@omskscream
Copy link
Author

Can we consider some explicit means of saying "read from stdin"?

Hi, and thanks for your concern. As I mentioned in a conversation earlier, my main reason for this was to keep token related behavior consistent across cargo commands.
Note that there're also cargo owner and cargo yank commands that accept --token argument from command line. I would assume that these will be updated the same way as cargo publish in the future, and would prefer them to share same interface -- they are more closely related than cargo login probably.

However, regarding the main concern I agree that explicitness might be less risky option here.
I can introduce a new command line flag for cargo publish, e.g. --token-stdin whose presence will be the required condition for reading token from stdin and no stdin read should happen if the flag is not set.
Another option might be to reuse existing --token option and treat missing value as a condition for reading from stdin. However, I feel that it might be confusing, especially for planned "deprecation" of --token option. I also feel that it is more prone to errors, e.g. when user forgets to provide --token value, then cargo tries to read token from stdin but actually reads nothing, and it's not clear what error to show then. It's a change of an existing behavior, so there might be unpredictable consequences somewhere.

To summarize: yeah, let's make this feature safer with a new command line flag? I'm in 🙂
Would also like to hear opinions on this approach change from people who already approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command-line interface, option parsing, etc. A-cli-help Area: built-in command-line help A-documenting-cargo-itself Area: Cargo's documentation Command-publish disposition-merge FCP with intent to merge proposed-final-comment-period An FCP proposal has started, but not yet signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cargo Team: Cargo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants