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

Fetching cargo registry tokens from external processes #2730

Open
wants to merge 5 commits into
base: master
from

Conversation

@pietroalbini
Copy link
Member

commented Jul 22, 2019

Rendered

This RFC proposes a new cargo configuration key to fetch registry tokens from external processes, allowing to store them in password managers or other secret storage systems:

# .cargo/credentials
[registry]
token-from-process = "pass tokens/crates-io | head -n 1"

Thanks @Eh2406 for the feedback on the draft!

@leo-lb

This comment has been minimized.

Copy link

commented Jul 22, 2019

Cool, please do add this! Unifying everything in GNOME keyrings and the likes is really nice.

@pietroalbini pietroalbini force-pushed the pietroalbini:cargo-token-process branch from a1ed3d7 to 0471313 Jul 22, 2019

@kornelski

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

macOS Keychain has access controls per application, and enforces that by verifying signature of the calling application.

The problem is, if you run a subprocess that uses Keychain, the Keychain will use identity of the subprocess. That breaks the chain of trust of the Keychain, because this way any application can just call the subprocess to get the token.

So regardless of this method, for macOS I'd prefer the solution mentioned in the alternatives section.

@pietroalbini

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

The problem is, if you run a subprocess that uses Keychain, the Keychain will use identity of the subprocess. That breaks the chain of trust of the Keychain, because this way any application can just call the subprocess to get the token.

Didn't know about that, but still, this PR would be an improvement because the users will be prompted every time the token is needed (if they don't "always allow"), providing a small layer of protection. Does Keychain require binaries to be signed to identify the requesting process? In that case, an RFC adding explicit Keychain support would also need to figure out the signing on our infra.

@tarcieri

This comment has been minimized.

Copy link

commented Jul 23, 2019

@kornelski I think it'd be great if cargo had built-in OS specific backends for secret storage on operating systems that have some sort of keychain abstraction.

However, that seems like an orthogonal nice-to-have, and there are several places I could see using something like this particular PR. One is "cloud shell" environments where tokens can be encrypted using the cloud's KMS service, and an OAuth credential used to talk to the KMS to decrypt tokens prior to use (logging each usage to an audit log).

As a sidebar and not-too-serious suggestion, there are also ways to circumvent the Keychain Services identity delegation-to-a-subprocess problem with megahax (instead of spawning a subprocess, you can load it into memory with NSCreateObjectFileImageFromMemory and link it into the running executable with NSLinkModule)

@kornelski

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

@pietroalbini That "allow/always allow" window pops up on unsigned executables or when the signature doesn't match.
Intended use of the Keychain does require having binaries signed. I know it's an open issue, and I'm hoping it'll get resolved too.

@tarcieri I get usefulness of having easy extensibility. I just wanted to point out that for proper macOS integration it's not entirely sufficient.

@tarcieri

This comment has been minimized.

Copy link

commented Jul 23, 2019

@kornelski as a separate issue, adding Keychain Services-backed secret storage to cargo (and abstractions for multiple token providers) is something I'd be interested in helping work on.

pietroalbini added some commits Jul 24, 2019

@pietroalbini

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

Renamed the configuration entry to token-from-process, as it's a bit more clear and the pattern is nicer for future extensions (like token-from-keychain = true).

@lukaslueg

This comment has been minimized.

Copy link

commented Jul 24, 2019

As far as I can see we are not required to promise that cargo will execute the subprocess in a shell; the user can specify to execute /bin/sh -c ... by himself if he does, in fact, need shell functionality (e.g. piping to awk as in the given example).

I recommend making explicit in the RFC whether the subprocess will inherit cargo's stdin or not. A working stdin allows the subprocess to ask for a password.

Otherwise an error message will be shown to the user, along with the standard output.

That is, the subprocesses' stdout is shown to the user? Shouldn't stdout and stderr simply be inherited so the subprocess can - if possible - print something to the tune of "please put your 2fa-thongy into the usb-thingy" while waiting for input.

@pietroalbini

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

As far as I can see we are not required to promise that cargo will execute the subprocess in a shell; the user can specify to execute /bin/sh -c ... by himself if he does, in fact, need shell functionality (e.g. piping to awk as in the given example).

How do we call the command without running it through a shell though? std::process::Command requires the args to be splitted, so we either implement splitting in Cargo (which doesn't really make sense), require something like token-from-process = ["program", "arg1", "arg2"] which is not that ergonomic or defer to the shell.

I recommend making explicit in the RFC whether the subprocess will inherit cargo's stdin or not. A working stdin allows the subprocess to ask for a password.

That is, the subprocesses' stdout is shown to the user? Shouldn't stdout and stderr simply be inherited so the subprocess can - if possible - print something to the tune of "please put your 2fa-thongy into the usb-thingy" while waiting for input.

Good points! Clarified that stderr and stdin will be inherited, while stdout will be captured to read the token.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

One thing that's worth pointing out is that Cargo already has support for invoking external processes to get credential information, it just only uses it for git authentication. The "credential helper" support that is located in git2 is intended to support Git configuration for external tokens for authentication with git servers.

It'd be great if that basically ended up being the same as whatever Cargo uses natively, and Cargo could be refactored to literally share the same code as well.

One downside of the implementation in git2 is that the deduction of how to invoke the command is pretty arcane and likely buggy to the point of just being downright confusing. I think that's something we'll want to fix/improve on if possible for the Cargo implementation, and I'm not sure if it's as easy as simply saying "pass to sh on Unix and cmd on Windows". That makes documentation about this feature, for example, more difficult because then you have to have conditional configuration depending on the platform you're running on (and cmd really is particularly weird in the syntax it accepts).

@pietroalbini

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

One thing that's worth pointing out is that Cargo already has support for invoking external processes to get credential information, it just only uses it for git authentication. The "credential helper" support that is located in git2 is intended to support Git configuration for external tokens for authentication with git servers.

It'd be great if that basically ended up being the same as whatever Cargo uses natively, and Cargo could be refactored to literally share the same code as well.

Do you mean reusing/sharing just the code used to call the process or the whole git credential helper protocol? I feel like the protocol itself is a bit overkill for what we want to do here.

One downside of the implementation in git2 is that the deduction of how to invoke the command is pretty arcane and likely buggy to the point of just being downright confusing. I think that's something we'll want to fix/improve on if possible for the Cargo implementation, and I'm not sure if it's as easy as simply saying "pass to sh on Unix and cmd on Windows". That makes documentation about this feature, for example, more difficult because then you have to have conditional configuration depending on the platform you're running on (and cmd really is particularly weird in the syntax it accepts).

Yeah, git2's approach of calling sh and if it fails splitting by whitespace is not ideal. I don't think using the system's native shell (sh or cmd) is that bad in terms of documentation, as most of the commands we could put in the example are going to either be disqualified for endorsing a specific product or be tied to the host system anyway.

Thinking about it a bit more, @lukaslueg's idea of just skipping the shell could work if we define in the RFC that the command will be split according to the POSIX standard. There is a crate that already implements the splitting code according to that.

@lukaslueg

This comment has been minimized.

Copy link

commented Jul 26, 2019

First and foremost this is an educated opinion, but: I would rather avoid having a string that cargo will execute by calling a hoped-for-to-be-there shell and just hand stuff to it.

Splitting the string does not require much code, is easily tested and does not rot. As an alternative, I don't think ["cmd", "arg1"] is that bad, especially if the single-string-syntax is as valid as the list-syntax.

token-from-process = "/usr/bin/helper"
token-from-process = ["/usr/bin/helper", "--quiet"]

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

@pietroalbini ah yeah to clarify I mean mostly to bring up what Cargo does today as an example of how Cargo's already doing this in some capacity. If the git protocol makes sense for us we could adopt it pretty easily, but if it doesn't make sense then I don't see any reason we should bend over backwards to do so.

We have, in rustc and cargo, avoided any form of shell escaping in all (afaik) ways of passing multiple arguments. I'd personally like to continue doing so, and the general solution we have is that there's a convenience "give me a bunch of arguments quickly" option which splits on whitespace, and then there's a "give me one argument at a time" interface which does no splitting. (e.g. -Clink-args vs -Clink-arg). Basically what @lukaslueg proposed in terms of TOML configuration is what Cargo already does internally for a number of other configuration values, and I think it would fit nicely here.

It's also worth pointing out that git provides a default built-in credential cache (I think it's like git credential-cache as a subcommand), and it is probably worthwhile to consider that Cargo should grow its own default credential manager. This would serve as a test case for Cargo, a reasonable default (although it should probably still be disabled by default), and something to put in the documentation which is cross-platform.

@zpgaal

This comment has been minimized.

Copy link

commented Jul 30, 2019

For the shell selection, a new attribute might work. ( just thinking outload without knowing too much detail about the subject)
'''
[registry.windows]
default-shell="cmd"
[registry.linux]
defaul-shell="bash"
[registry]
...
'''

This way it has a non hard coded configuration that is separated from the actual parameters and cross platform.
Also this concept can be generalized for other command moving this setting into some "common" cargo config file. Tough it may make things harder to read

@pietroalbini pietroalbini force-pushed the pietroalbini:cargo-token-process branch from b746def to 020f794 Aug 8, 2019

@pietroalbini

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

Updated the RFC to reflect @lukaslueg's idea.

issued to let the user know about that.

The `token-from-process` key accepts either a string containing the binary to
call or a list containing the binary name and the arguments to provide to it.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Aug 8, 2019

Member

FWIW Cargo's intepretation of this today is that a bare string is a whitespace-separated list of arguments and a list of strings is the exact list of arguments. We'll probably want to stay consistent with the rest of Cargo in that respect?

This comment has been minimized.

Copy link
@pietroalbini

pietroalbini Aug 8, 2019

Author Member

My worry about splitting by whitespace is it just breaks in unexpected ways if people wants to use a shell:

token-from-process = 'bash -c "echo foo | head -n 1"'

With some CLI password managers this is going to be really common, and splitting ["bash", "-c", "\"echo", "foo", "|", "head" "-n", "1\""] is just unintuitive. I'd prefer to either properly split quotes (by using crates like shlex) or just not split at all.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Aug 8, 2019

Member

I think that's possible, yes, but I think it's more important to either to stay consistent with Cargo or fully commit to some form of parsing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.