Implement external credential process. (RFC 2730) #8934

ehuss · 2020-12-03T03:11:19Z

This adds a config setting for an external process to run to fetch the token for a registry. See unstable.md for more details.

As part of this, it adds a new logout command. This is currently gated on nightly with the appropriate -Z flag.

I have included four sample wrappers that integrate with the macOS Keychain, Windows Credential Manager, GNOME libsecret, and 1password. I'm not sure if we'll ultimately ship these, but I would like to. Primarily this provided a proof-of-concept to see if the design works.

Patch Walkthrough

This is a brief overview of the changes:

Adds the logout command. With cargo logout -Z unstable-options, this allows removing the token from .cargo/credentials. With cargo logout -Z credential-process, this launches the process with the erase argument to remove the token from storage.
Credential-process handling is in the ops/registry/auth.rs module. I think it is pretty straightforward, it just launches the process with the appropriate store/get/erase argument.
ops::registry::registry() now returns the RegistryConfig to make it easier to pass the config information around.
crates/credential/cargo-credential is a helper crate for writing credential processes.
A special shorthand of the cargo: prefix for a credential process will launch the named process from the libexec directory in the sysroot (or, more specifically, the libexec directory next to the cargo process). For example credential-process = "cargo:macos-keychain". My intent is to bundle these in the pre-built rust-lang distributions, and this should "just work" when used with rustup. I'm not sure how that will work with other Rust distributions, but I'm guessing they can figure it out. This should make it much easier for users to get started, but does add some integration complexity.

Questions

I'm on the fence about the name credential-process vs credentials-process, which sounds more natural? (Or something else?)
I'm uneasy about the behavior when both token and credential-process is specified (see warn_both_token_and_process test). Currently it issues a warning and uses token. Does that make sense? What about the case where you have registries.foo.token for a specific registry, but then have a general registry.credential-process for the default (it currently warns and uses the token, maybe it should not warn?)?
I am still pretty uneasy with writing FFI wrappers, so maybe those could get a little extra scrutiny? They seem to work, but I have not extensively tested them (I tried login, publish, and logout). I have not previously used these APIs, so I am not familiar with them.
Testing the wrappers I think will be quite difficult, because some require TTY interaction (and 1password requires an online account). Or, for example in the macOS case, it has GUI dialog box where I can use my fingerprint scanner. Right now, I just build them in CI to make sure they compile.
1password is a little weird in that it passes the token on the command-line, which is not very secure on some systems (other processes can see these sometimes). The only alternative I can think of is to not support cargo login and require the user to manually enter the token in the 1password GUI. I don't think the concern is too large (1password themselves seem to think it is acceptable). Should this be OK?
I'm a little uneasy with the design of cargo login, where it passes the token in stdin. If the wrapper requires stdin for user interaction (such as entering a password to unlock), this is quite awkward. There is a hack in the 1password example that demonstrates using /dev/tty and CONIN$, which seems to work, but I'm worried is fragile. I'm not very comfortable passing the token in environment variables, because those can be visible to other processes (like CLI args), but in some situations that shouldn't be too risky. Another option is to use a separate file descriptor/handle to pass the token in. Implementing that in Rust in a cross-platform way is not terribly easy, so I wanted to open this up for discussion first.

rust-highfive · 2020-12-03T03:11:23Z

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

alexcrichton

Looking good to me, thanks!

I don't know much about the specific FFI interfaces used here but all the crates look good to me modulo protocol-specific bugs.

alexcrichton · 2020-12-04T17:28:31Z

.github/workflows/main.yml

+    - run: cargo build --manifest-path crates/credential/cargo-credential-macos-keychain/Cargo.toml
+      if: matrix.os == 'macos-latest'
+    - run: cargo build --manifest-path crates/credential/cargo-credential-wincred/Cargo.toml
+      if: matrix.os == 'windows-latest'


I'm only just starting, but for convenience it'd be nice if these crates built on all platforms and then at runtime just returned errors on the wrong platform, that way things like cargo test --all would work well.

I'm a little uncomfortable doing that since for example the gnome package won't work if libsecret isn't installed, and I think it could cause confusing problems if it silently ignored the absence of that library.

What do you think about adding that kind of logic to Cargo? That is, you could somehow specify a binary only works on specific targets, and is otherwise ignored (similar to required-features). I suspect it will be a long while before Cargo can use a workspace (due to the lack of nested workspaces), so I imagine this won't be a concern for a while.

If you'd really prefer them to always compile, I'm fine with doing that, I just feel it could lead to confusing problems.

Nah it's not a strong preference on my part, mostly just cleaning up CI config and ideally making integration elsewhere easier. (rustbuild will need to encode these platform rules, right?)

FWIW I was imagining that we wouldn't silently ignore the lack of something like libsecret, but we would ignore winapi when building for Linux for example. As for target-specific binaries, seems like a reasonable feature to me to add!

alexcrichton · 2020-12-04T17:39:55Z

src/cargo/ops/registry/auth.rs

+        .map(|arg| {
+            arg.replace("{action}", action_str)
+                .replace("{name}", name)
+                .replace("{api_url}", api_url)


This is a slight forward-compatibility hazard in that there's no escaping availble to call the process with {action} literally as a string somewhere. Not really the end of the world and would be sort of a pain to handle, but figured it was at least worth mentioning.

alexcrichton · 2020-12-04T17:41:01Z

src/cargo/ops/registry/auth.rs

+                        exe.display()
+                    )
+                })?;
+            if let Some(end) = buffer.find('\n') {


I'd expect a call to .trim() here, but is this intentionally ignoring the second-and-after lines of output?

Hm, yea, I changed it so that it will return an error if there is more than one line.

alexcrichton · 2020-12-04T17:42:57Z

crates/credential/cargo-credential/src/lib.rs

+}
+
+fn doit(credential: impl Credential) -> Result<(), Error> {
+    let which = std::env::args()


This behavior of discovering the action may be worth mentioning in the crate docs?

alexcrichton · 2020-12-04T17:43:56Z

crates/credential/cargo-credential/Cargo.toml

+version = "0.1.0"
+authors = ["The Rust Project Developers"]
+edition = "2018"
+license = "MIT OR Apache-2.0"


This is intended for publication, right? If so want to fill in some metadata here?

(same for the other crates)

alexcrichton · 2020-12-04T17:45:04Z

crates/credential/cargo-credential-wincred/src/main.rs

+struct WindowsCredential;
+
+fn wstr(s: &str) -> Vec<u16> {
+    OsStr::new(s).encode_wide().chain(Some(0)).collect()


This may want to return an error as well if s contains a 0, because otherwise I think it'll get silently truncated

alexcrichton · 2020-12-04T17:47:01Z

crates/credential/cargo-credential-gnome-secret/build.rs

@@ -0,0 +1,3 @@
+fn main() {
+    pkg_config::probe_library("libsecret-1").unwrap();


This is one of my bigger worries about shipping these executables with rust-lang/rust. We typically don't make any assumptions about the execution environment, but this execution will require libsecret.so or something like that to run the process, right?

Or is there a way we could ask this to link statically when we build distribution binaries?

Although one though I had reading this, these crates should all be trivial to cargo install, so we could perhaps manage it that way?

Yea, this is a dynamic dependency, and the user will have to install the appropriate package on their system. I would feel a little uneasy statically linking this, primarily since I'm not very familiar with the GNOME ecosystem, but also because I'm not really sure it would work. There is infrastructure (the "Secret Service" and the gnome-keyring) that the user needs to have installed, and I suspect that the library that talks to them needs to be somewhat compatible.

I'm also not sure at this point how feasible it is to build this on rust-lang/rust, since they tend to use old Linux distributions which can be painful to use. I haven't yet tried. If it is too difficult to build, then I think we'll just have to tell the user to build it themselves (with cargo install), in which case they'll have to install libsecret anyways.

My feeling is that this will be an issue for documentation (it can include some sample instructions on what to install for various distributions).

In general, I see these sample implementations as a very "experimental" status. I wanted to see what issues may crop up, and have that guide how to proceed. If they don't work for some users, then they just won't work. Perhaps others can step up to either provide patches, or publish their own implementations on crates.io. If they stop working and go unmaintained, I think it would be reasonable just to delete them. (I would hope it doesn't come to that, I just want to emphasize that I think there is some flexibility here.) I have no idea how many people use GNOME secret storage, perhaps nobody will. 😄

Ah ok makes sense, if we don't ship these with rust-lang/rust (which I initially thought we would), then this seems fine to ignore. I'm curious then, though, if we ship via Cargo and cargo install, do we still need the libexec logic? Or should we just look in $PATH?

I would like to ship these wrappers, just not the libsecret one. I checked, and the Linux used on rust-lang/rust is too old, so I updated the documentation to indicate that it needs to be installed manually.

alexcrichton · 2020-12-04T17:48:56Z

crates/credential/cargo-credential-gnome-secret/src/main.rs

+            if !error.is_null() {
+                return Err(format!(
+                    "failed to get token: {}",
+                    CStr::from_ptr((*error).message).to_str()?


This technically looks like we'd need to free the error after this, right?

Given that this is a very short-lived process, though, seems fine to ignore destructors.

Yea, I think there is a g_error_free. I can add that if you want, though I wasn't too concerned about leaking in a one-shot process.

Nah yeah just wanted to note this, but skipping dtors seems fine.

ehuss · 2020-12-06T19:20:47Z

crates/credential/cargo-credential-gnome-secret/src/main.rs

+    SecretSchema {
+        name: b"org.rust-lang.cargo.registry\0".as_ptr() as *const gchar,
+        flags: SecretSchemaFlags::None,
+        attributes,
+    }
+}


I just had a general Rust question here. Is there a way to specify this struct literal that doesn't require so much awkwardness? In C, it would be defined as:

static const SecretSchema the_schema = { "org.rust-lang.cargo.registry", SECRET_SCHEMA_NONE, { { "registry", SECRET_SCHEMA_ATTRIBUTE_STRING }, { "url", SECRET_SCHEMA_ATTRIBUTE_STRING }, { "NULL", 0 }, } };

However, I couldn't think of a way to do the equivalent in Rust. Since the attributes are an array of length 32, I don't know how to fill out the 29 other entries.

Making this a const function may actually work? Other than that though I don't think there's a smaller method than this.

alexcrichton · 2020-12-07T17:17:45Z

Oh I meant to offer some answers to your questions last time but forgot:

I'm on the fence about the name credential-process vs credentials-process, which sounds more natural?

I'd go for credentials-process myself, since to me "credentials" is somewhat synonymous with "credential" and I colloquially always say "credentials". Typing this out, though, the error in my ways is more obvious.

'm uneasy about the behavior when both token and credential-process is specified

At least for the registries.foo.token and registry.credential-process, I think it makes sense to use the "more specific one" so w/e the registries.foo has. For when they're both specified at the same level, it might be worth emitting an error for now? That way no one will run into ambiguity and we can always relax it later.

Testing the wrappers I think will be quite difficult, because some require TTY interaction

I think this is fine. They're pretty small and easy to review, and we'd just need to be vigilant as reviewers that any modifications are tested by the contributor or tested locally by us.

1password is a little weird in that it passes the token on the command-line

This seems fine by me. If we're extra intrepid I'd open a feature request with them, though.

I'm a little uneasy with the design of cargo login, where it passes the token in stdin.

I think I agree that it'd be best if we could avoid stdin. My ideal solution would be the separate file descriptor/handle (and then we'd specify the fd or handle in an env var or something like that), and perhaps an external crate could ease the boilerplate? I may also be naive assuming something like this exists.

That being said I personally would think that the token being in an env var also isn't the end of the world.

Change it so that if both are specified, it is an error just to be safer for now. If token is specified for a registry, ignore the global credential-process. I'm still uncertain if this is the best behavior, but I think we can tweak it later if needed.

The rust-lang/rust build infrastructure uses a version of Linux that is too old to support building this.

ehuss · 2020-12-09T19:10:26Z

credential-process vs credentials-process

I waffled, but I feel like keeping it as-is. awscli uses the singular version (credential_process), so there's at least some precedent for it.

to use the "more specific one"

I changed it to use the more specific one, and error if both are specified. I think it can be tweaked if that is too awkward.

best if we could avoid stdin. My ideal solution would be the separate file descriptor/handle

Hm, I looked at this for a while, and it just seems extraordinarily difficult on windows (like maybe 100+ lines of code?). AIUI, you create a pipe with the bInheritHandle security attribute. Pray that no other processes or threads are spawned while you hold the pipe open. Convert the RawHandle to a string, and pass it in to the child. Have the child convert the string to a *mut void pointer (!), and use FromRawHandle to create a File object.

I....don't feel like doing all that. Would it be OK to maybe defer this to the future? The current implementation works now, and is pretty simple. I think if we wanted to use the file-descriptor route, we could extend it later by adding a {handle} CLI arg which would tell Cargo to create a separate pipe to send the token, and the CLI args would look like "cargo-credential-1password {action} --handle {handle}" or something like that.

alexcrichton

Yeah I think you're right about how it works on Windows, and you're not wrong that it feels a bit odd! In any case everything looks and sounds great to me!

Did you have any final bits or is this good to go? I added an unresolved question to the tracking issue about the {action} syntax and escaping.

ehuss · 2020-12-12T18:05:03Z

I think this is good to go for now. I added some other notes to the tracking issue.

alexcrichton · 2020-12-14T16:22:47Z

@bors: r+

bors · 2020-12-14T16:22:50Z

📌 Commit eabef56 has been approved by alexcrichton

bors · 2020-12-14T16:23:02Z

⌛ Testing commit eabef56 with merge 8917837...

bors · 2020-12-14T16:51:09Z

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 8917837 to master...

Update cargo 4 commits in d274fcf862b89264fa2c6b917b15230705257317..a3c2627fbc2f5391c65ba45ab53b81bf71fa323c 2020-12-07 23:08:44 +0000 to 2020-12-14 17:21:26 +0000 - Check if rerun-if-changed points to a directory. (rust-lang/cargo#8973) - Implement external credential process. (RFC 2730) (rust-lang/cargo#8934) - Revert recent build-std vendoring changes (rust-lang/cargo#8968) - Fix the unit dependency graph with dev-dependency `links` (rust-lang/cargo#8969)

Implement credential-process.

cc6df1d

rust-highfive assigned alexcrichton Dec 3, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 3, 2020

ehuss mentioned this pull request Dec 3, 2020

Tracking Issue for credential-process RFC 2730 #8933

Closed

16 tasks

alexcrichton reviewed Dec 4, 2020

View reviewed changes

ehuss commented Dec 6, 2020

View reviewed changes

ehuss added 5 commits December 6, 2020 11:40

Check for nul in Windows utf-16 wide-string.

8ac04d1

Add docs on how cargo-credential finds the action argument.

37e9c77

Add some credential manifest metadata and READMEs.

69c5af8

Validate that the credential process only outputs a single line (token).

bdbc8da

Update credential-process tracking issue number.

4b12011

ehuss force-pushed the token-process branch from 3cd4fe9 to 4b12011 Compare December 6, 2020 19:41

ehuss added 2 commits December 8, 2020 08:36

Note that cargo-credential-gnome-secret won't be available precompiled.

eabef56

The rust-lang/rust build infrastructure uses a version of Linux that is too old to support building this.

alexcrichton approved these changes Dec 10, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 14, 2020

bors merged commit 8917837 into rust-lang:master Dec 14, 2020

This was referenced Dec 14, 2020

Implement future incompatibility report support #8825

Merged

Implement experimental registry HTTP API from RFC #8890

Closed

ehuss mentioned this pull request Dec 16, 2020

Update cargo rust-lang/rust#80081

Merged

tarcieri mentioned this pull request Sep 20, 2021

Cargo alternative registry auth rust-lang/rfcs#3139

Merged

ehuss added this to the 1.50.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement external credential process. (RFC 2730) #8934

Implement external credential process. (RFC 2730) #8934

ehuss commented Dec 3, 2020

rust-highfive commented Dec 3, 2020

alexcrichton left a comment

alexcrichton Dec 4, 2020

ehuss Dec 6, 2020

alexcrichton Dec 7, 2020

alexcrichton Dec 4, 2020

alexcrichton Dec 4, 2020

ehuss Dec 6, 2020

alexcrichton Dec 4, 2020

alexcrichton Dec 4, 2020

alexcrichton Dec 4, 2020

alexcrichton Dec 4, 2020

alexcrichton Dec 4, 2020

alexcrichton Dec 4, 2020

ehuss Dec 6, 2020

alexcrichton Dec 7, 2020

ehuss Dec 9, 2020

alexcrichton Dec 4, 2020

ehuss Dec 6, 2020

alexcrichton Dec 7, 2020

ehuss Dec 6, 2020

alexcrichton Dec 7, 2020

alexcrichton commented Dec 7, 2020

ehuss commented Dec 9, 2020

alexcrichton left a comment

ehuss commented Dec 12, 2020

alexcrichton commented Dec 14, 2020

bors commented Dec 14, 2020

bors commented Dec 14, 2020

bors commented Dec 14, 2020

		@@ -0,0 +1,3 @@
		fn main() {
		pkg_config::probe_library("libsecret-1").unwrap();

Implement external credential process. (RFC 2730) #8934

Implement external credential process. (RFC 2730) #8934

Conversation

ehuss commented Dec 3, 2020

rust-highfive commented Dec 3, 2020

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Dec 7, 2020

ehuss commented Dec 9, 2020

alexcrichton left a comment

Choose a reason for hiding this comment

ehuss commented Dec 12, 2020

alexcrichton commented Dec 14, 2020

bors commented Dec 14, 2020

bors commented Dec 14, 2020

bors commented Dec 14, 2020