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.

Sign up for GitHub

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

Implement external credential process. (RFC 2730) #8934

Merged
merged 8 commits into from
Dec 14, 2020
Merged

Implement external credential process. (RFC 2730) #8934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cc6df1d
Implement credential-process.
ehuss Dec 1, 2020
8ac04d1
Check for nul in Windows utf-16 wide-string.
ehuss Dec 4, 2020
37e9c77
Add docs on how cargo-credential finds the action argument.
ehuss Dec 4, 2020
69c5af8
Add some credential manifest metadata and READMEs.
ehuss Dec 4, 2020
bdbc8da
Validate that the credential process only outputs a single line (token).
ehuss Dec 4, 2020
4b12011
Update credential-process tracking issue number.
ehuss Dec 6, 2020
395edf2
Change behavior when both token and credential-process is specified.
ehuss Dec 8, 2020
eabef56
Note that cargo-credential-gnome-secret won't be available precompiled.
ehuss Dec 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@ jobs:
- run: rustup update stable && rustup default stable
- run: rustup component add rustfmt
- run: cargo fmt --all -- --check
- run: cd crates/cargo-test-macro && cargo fmt --all -- --check
- run: cd crates/cargo-test-support && cargo fmt --all -- --check
- run: cd crates/crates-io && cargo fmt --all -- --check
- run: cd crates/resolver-tests && cargo fmt --all -- --check
- run: cd crates/cargo-platform && cargo fmt --all -- --check
- run: cd crates/mdman && cargo fmt --all -- --check
- run: |
for manifest in `find crates -name Cargo.toml`
do
echo check fmt for $manifest
cargo fmt --all --manifest-path $manifest -- --check
done

test:
runs-on: ${{ matrix.os }}
Expand Down Expand Up @@ -58,7 +58,7 @@ jobs:
- run: rustup target add ${{ matrix.other }}
- run: rustup component add rustc-dev llvm-tools-preview rust-docs
if: startsWith(matrix.rust, 'nightly')
- run: sudo apt update -y && sudo apt install gcc-multilib -y
- run: sudo apt update -y && sudo apt install gcc-multilib libsecret-1-0 libsecret-1-dev -y
if: matrix.os == 'ubuntu-latest'
- run: rustup component add rustfmt || echo "rustfmt not available"

Expand All @@ -67,6 +67,13 @@ jobs:
- run: cargo test --features 'deny-warnings' -p cargo-test-support
- run: cargo test -p cargo-platform
- run: cargo test --manifest-path crates/mdman/Cargo.toml
- run: cargo build --manifest-path crates/credential/cargo-credential-1password/Cargo.toml
- run: cargo build --manifest-path crates/credential/cargo-credential-gnome-secret/Cargo.toml
if: matrix.os == 'ubuntu-latest'
- 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'
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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!


resolver:
runs-on: ubuntu-latest
Expand Down
4 changes: 4 additions & 0 deletions crates/cargo-test-support/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1544,6 +1544,10 @@ fn substitute_macros(input: &str) -> String {
("[INSTALLED]", " Installed"),
("[REPLACED]", " Replaced"),
("[BUILDING]", " Building"),
("[LOGIN]", " Login"),
("[LOGOUT]", " Logout"),
("[YANK]", " Yank"),
("[OWNER]", " Owner"),
];
let mut result = input.to_owned();
for &(pat, subst) in &macros {
Expand Down
21 changes: 17 additions & 4 deletions crates/cargo-test-support/src/paths.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,27 @@ pub trait CargoPathExt {
}

impl CargoPathExt for Path {
/* Technically there is a potential race condition, but we don't
* care all that much for our tests
*/
fn rm_rf(&self) {
if self.exists() {
let meta = match self.symlink_metadata() {
Ok(meta) => meta,
Err(e) => {
if e.kind() == ErrorKind::NotFound {
return;
}
panic!("failed to remove {:?}, could not read: {:?}", self, e);
}
};
// There is a race condition between fetching the metadata and
// actually performing the removal, but we don't care all that much
// for our tests.
if meta.is_dir() {
if let Err(e) = remove_dir_all::remove_dir_all(self) {
panic!("failed to remove {:?}: {:?}", self, e)
}
} else {
if let Err(e) = fs::remove_file(self) {
panic!("failed to remove {:?}: {:?}", self, e)
}
}
}

Expand Down
8 changes: 8 additions & 0 deletions crates/credential/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# Cargo Credential Packages

This directory contains Cargo packages for handling storage of tokens in a
secure manner.

`cargo-credential` is a generic library to assist writing a credential
process. The other directories contain implementations that integrate with
specific credential systems.
13 changes: 13 additions & 0 deletions crates/credential/cargo-credential-1password/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
[package]
name = "cargo-credential-1password"
version = "0.1.0"
authors = ["The Rust Project Developers"]
edition = "2018"
license = "MIT OR Apache-2.0"
repository = "https://github.com/rust-lang/cargo"
description = "A Cargo credential process that stores tokens in a 1password vault."

[dependencies]
cargo-credential = { path = "../cargo-credential" }
serde = { version = "1.0.117", features = ["derive"] }
serde_json = "1.0.59"
Loading