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

[SM-1129] Run command with secrets #621

Merged
merged 87 commits into from
Aug 16, 2024
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
87 commits
Select commit Hold shift + click to select a range
07e1fe9
feat: add `bws run` command
tangowithfoxtrot Feb 20, 2024
391d6b1
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Feb 21, 2024
bd75024
allow reading from stdin
tangowithfoxtrot Feb 21, 2024
48f3841
allow other shells
tangowithfoxtrot Feb 21, 2024
1e5d63a
print stdout+stderr as they happen
tangowithfoxtrot Feb 21, 2024
a52b989
inherit output instead of piping it manually
tangowithfoxtrot Feb 21, 2024
710ae61
return child process exit status
tangowithfoxtrot Feb 21, 2024
f3ad7f3
display warning about problematic key names
tangowithfoxtrot Feb 21, 2024
c7d29ad
add `--no-inherit-env` option
tangowithfoxtrot Feb 21, 2024
211d067
appease clippy 📎
tangowithfoxtrot Feb 21, 2024
26af620
fix: wrong default shell on Windows
tangowithfoxtrot Feb 22, 2024
0f4d996
fix: determine OS at runtime (not compile time)
tangowithfoxtrot Feb 23, 2024
7516a45
update warning message
tangowithfoxtrot Feb 23, 2024
a19e820
fix: run command hanging if no command was supplied; don't allow inte…
tangowithfoxtrot Feb 23, 2024
bafdaad
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Feb 27, 2024
5fc71d3
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Mar 18, 2024
6ad88b8
fix: panic on invalid shell
tangowithfoxtrot Mar 18, 2024
accc97e
`cargo fmt`
tangowithfoxtrot Mar 18, 2024
b78284b
fix: `clippy::nonminimal_bool`
tangowithfoxtrot Mar 18, 2024
c2ee84c
fix: `clippy::nonminimal_bool`
tangowithfoxtrot Apr 25, 2024
acbffe6
Merge remote-tracking branch 'origin/main' into run-command-with-secrets
tangowithfoxtrot Apr 25, 2024
9eb8e9f
update hermit-abi
tangowithfoxtrot Apr 29, 2024
4dfde95
use `expect` instead of `unwrap`
tangowithfoxtrot Apr 29, 2024
9fc608b
cargo fmt
tangowithfoxtrot Apr 29, 2024
1ab92be
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Apr 29, 2024
ce96046
Update crates/bws/src/main.rs
tangowithfoxtrot May 28, 2024
8e51cf9
Update crates/bws/src/main.rs
tangowithfoxtrot May 28, 2024
68306b3
fmt
tangowithfoxtrot May 28, 2024
2e673b6
remove unused RunCommand
tangowithfoxtrot May 28, 2024
b6e676a
extract valid posix key detection so that it can be reused
tangowithfoxtrot May 29, 2024
65b5492
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot May 29, 2024
f483b30
Revert "Merge branch 'main' into run-command-with-secrets"
tangowithfoxtrot May 29, 2024
64c30c6
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot May 29, 2024
0e87279
move run command to cli.rs
tangowithfoxtrot May 29, 2024
606185c
re-add which to cargo
tangowithfoxtrot May 29, 2024
30da24c
cargo fmt
tangowithfoxtrot Jun 4, 2024
8cdf289
rm redundant child.wait
tangowithfoxtrot Jun 4, 2024
cd56439
fail if duplicate keynames
tangowithfoxtrot Jun 11, 2024
14fa8df
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Jun 11, 2024
cb9903a
unset BWS_ACCESS_TOKEN in run command by default
tangowithfoxtrot Jun 11, 2024
9511ba2
feat: uuids-as-keynames
tangowithfoxtrot Jun 11, 2024
55c9e72
appease clippy
tangowithfoxtrot Jun 12, 2024
0c6128a
use `contains_key` instead of `entry`
tangowithfoxtrot Jun 12, 2024
09bdee7
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Jun 21, 2024
9ae6078
refactor run command
tangowithfoxtrot Jun 21, 2024
4a4a8d8
`cargo fmt`
tangowithfoxtrot Jun 21, 2024
cb6f987
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Jun 27, 2024
eb29e7d
Revert "Merge branch 'main' into run-command-with-secrets"
tangowithfoxtrot Jun 27, 2024
2da9ca9
unit tests for helper functions
tangowithfoxtrot Jun 27, 2024
deebd5d
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Jun 27, 2024
2c5a279
import ClientSecretsExt
tangowithfoxtrot Jun 27, 2024
a836b65
use `std::io::IsTerminal` instead of `atty` crate
tangowithfoxtrot Jul 1, 2024
17a4d28
supress unused import warning in unit tests
tangowithfoxtrot Jul 1, 2024
14a9e50
fix: rm unneeded `which` as build dep
tangowithfoxtrot Jul 1, 2024
c93bf8f
fix: follow var name convention
tangowithfoxtrot Jul 1, 2024
5be08ed
fix: use documentation comments
tangowithfoxtrot Jul 1, 2024
e06e48f
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Jul 1, 2024
3df1c6d
fix: formatting
tangowithfoxtrot Jul 2, 2024
68973d0
chore: upgrade which
tangowithfoxtrot Jul 2, 2024
83eb725
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Jul 5, 2024
61b41fc
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Jul 9, 2024
9f6a165
fix: rm redundant check for empty command
tangowithfoxtrot Jul 9, 2024
3080dac
feat: bail instead of exiting
tangowithfoxtrot Jul 9, 2024
23ab8f4
fix: provide default path for windows in edge-case where path isn't set
tangowithfoxtrot Jul 9, 2024
3fcd32c
fix: exit early if duplicates are detected
tangowithfoxtrot Jul 9, 2024
c7aad3f
chore: update lock file
tangowithfoxtrot Jul 9, 2024
eda00aa
fix: clippy; use `if let` instead of `match` for single pattern
tangowithfoxtrot Jul 10, 2024
9839b94
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Jul 10, 2024
7362764
chore: formatting
tangowithfoxtrot Jul 11, 2024
f9629b8
fix: return error instead of exiting in run.rs
tangowithfoxtrot Jul 11, 2024
15b1114
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Jul 11, 2024
4d4922f
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Jul 15, 2024
1c6ffbc
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Jul 16, 2024
75e3d8b
Update crates/bws/src/command/run.rs
tangowithfoxtrot Jul 16, 2024
e86aa2e
Update crates/bws/src/main.rs
tangowithfoxtrot Jul 16, 2024
b86d026
Update crates/bws/src/command/run.rs
tangowithfoxtrot Jul 16, 2024
f4c2598
Merge branch 'run-command-with-secrets' of https://github.com/bitward…
tangowithfoxtrot Jul 16, 2024
f9456b8
fix: add itertools dep to cargo
tangowithfoxtrot Jul 16, 2024
e31c689
chore: rm unused import
tangowithfoxtrot Jul 16, 2024
4a5aeb3
chore: update cargo lock file
tangowithfoxtrot Jul 16, 2024
5bdf99c
we don't need to handle invalid posix regex
tangowithfoxtrot Jul 16, 2024
f9c9946
refactor: remove unused enum
tangowithfoxtrot Jul 16, 2024
a75f53e
chore: `cargo fmt`
tangowithfoxtrot Jul 16, 2024
4770dbf
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Jul 16, 2024
87fc76d
fix: Windows PowerShell crash when `--no-inherit-env` is passed
tangowithfoxtrot Aug 8, 2024
bc1a6ff
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Aug 8, 2024
a0f78a3
Merge branch 'main' into run-command-with-secrets
tangowithfoxtrot Aug 15, 2024
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
37 changes: 36 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions crates/bws/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ repository.workspace = true
license-file.workspace = true

[dependencies]
atty = "0.2.14"
Hinton marked this conversation as resolved.
Show resolved Hide resolved
bat = { version = "0.24.0", features = [
"regex-onig",
], default-features = false }
Expand Down Expand Up @@ -44,6 +45,8 @@ tokio = { version = "1.36.0", features = ["rt-multi-thread", "macros"] }
toml = "0.8.10"
uuid = { version = "^1.7.0", features = ["serde"] }

which = "6.0.0"
Hinton marked this conversation as resolved.
Show resolved Hide resolved

[dev-dependencies]
tempfile = "3.10.0"

Expand Down
128 changes: 127 additions & 1 deletion crates/bws/src/main.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::{path::PathBuf, process, str::FromStr};
use std::{io::Read, path::PathBuf, process, str::FromStr};

use atty::Stream;
use bitwarden::{
auth::{login::AccessTokenLoginRequest, AccessToken},
client::client_settings::ClientSettings,
Expand Down Expand Up @@ -27,6 +28,7 @@
use config::ProfileKey;
use render::{serialize_response, Output};
use uuid::Uuid;
use which::which;

#[derive(Parser, Debug)]
#[command(name = "bws", version, about = "Bitwarden Secrets CLI", long_about = None)]
Expand Down Expand Up @@ -84,6 +86,20 @@
#[command(subcommand)]
cmd: SecretCommand,
},
#[command(long_about = "Run a command with secrets injected")]
Run {
#[arg(help = "The command to run")]
command: Vec<String>,

Check warning on line 92 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L92

Added line #L92 was not covered by tests
#[arg(long, help = "The shell to use")]
shell: Option<String>,
#[arg(
long,
help = "Don't inherit environment variables from the current shell"
)]
no_inherit_env: bool,

Check warning on line 99 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L99

Added line #L99 was not covered by tests
#[arg(long, help = "The ID of the project to use")]
project_id: Option<Uuid>,
},
#[command(long_about = "Create a single item (deprecated)", hide(true))]
Create {
#[command(subcommand)]
Expand Down Expand Up @@ -222,6 +238,15 @@
Secret { secret_ids: Vec<Uuid> },
}

#[derive(Subcommand, Debug)]
enum RunCommand {
Copy link
Member

Choose a reason for hiding this comment

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

This enum doesn't seem to be used anywhere, only the Run variant above is used, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. You're right. Removed it.

Command {
command: Vec<String>,

Check warning on line 244 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L244

Added line #L244 was not covered by tests
project_id: Option<Uuid>,
shell: Option<String>,
},
}

#[tokio::main(flavor = "current_thread")]
async fn main() -> Result<()> {
env_logger::Builder::from_env(env_logger::Env::default().default_filter_or("info")).init();
Expand Down Expand Up @@ -603,6 +628,107 @@
}
}

Commands::Run {
command,
shell,
no_inherit_env,
project_id,

Check warning on line 635 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L632-L635

Added lines #L632 - L635 were not covered by tests
} => {
let shell = match std::env::consts::OS {
Copy link
Member

Choose a reason for hiding this comment

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

The process_command function is getting quite big already, do you mind splitting this to a separate function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I can probably move this stuff out of here in a similar fashion to how it's being handled in #809.

os if os == "linux" || os == "macos" || os.contains("bsd") => {
shell.unwrap_or_else(|| "sh".to_string())

Check warning on line 639 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L637-L639

Added lines #L637 - L639 were not covered by tests
}
"windows" => shell.unwrap_or_else(|| "powershell".to_string()),
_ => unreachable!(),

Check warning on line 642 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L641-L642

Added lines #L641 - L642 were not covered by tests
};
tangowithfoxtrot marked this conversation as resolved.
Show resolved Hide resolved

if which(&shell).is_err() {
eprintln!("Error: shell '{}' not found", shell);
std::process::exit(1);
}

Check warning on line 648 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L645-L648

Added lines #L645 - L648 were not covered by tests

let user_command = if command.is_empty() {
if atty::is(Stream::Stdin) {
dani-garcia marked this conversation as resolved.
Show resolved Hide resolved
eprintln!("{}", Cli::command().render_help().ansi());
std::process::exit(1);
}

let mut buffer = String::new();
std::io::stdin().read_to_string(&mut buffer)?;
buffer

Check warning on line 658 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L650-L658

Added lines #L650 - L658 were not covered by tests
} else {
command.join(" ")

Check warning on line 660 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L660

Added line #L660 was not covered by tests
};

if user_command.is_empty() {
let mut cmd = Cli::command();
eprintln!("{}", cmd.render_help().ansi());
std::process::exit(1);
}

Check warning on line 667 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L663-L667

Added lines #L663 - L667 were not covered by tests

let res = if let Some(project_id) = project_id {
client
.secrets()
.list_by_project(&SecretIdentifiersByProjectRequest { project_id })
.await?

Check warning on line 673 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L669-L673

Added lines #L669 - L673 were not covered by tests
} else {
client
.secrets()
.list(&SecretIdentifiersRequest { organization_id })
.await?

Check warning on line 678 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L675-L678

Added lines #L675 - L678 were not covered by tests
};

let secret_ids = res.data.into_iter().map(|e| e.id).collect();
let secrets = client
.secrets()
.get_by_ids(SecretsGetRequest { ids: secret_ids })
.await?

Check warning on line 685 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L681-L685

Added lines #L681 - L685 were not covered by tests
.data;

let environment = secrets
.iter()
.map(|s| (s.key.clone(), s.value.clone()))
.collect::<std::collections::HashMap<String, String>>();
tangowithfoxtrot marked this conversation as resolved.
Show resolved Hide resolved

let valid_key_regex = regex::Regex::new("^[a-zA-Z_][a-zA-Z0-9_]*$").unwrap();

Check warning on line 693 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L688-L693

Added lines #L688 - L693 were not covered by tests
Fixed Show fixed Hide fixed

for key in environment.keys() {
if !valid_key_regex.is_match(key) {
eprintln!(
"Warning: secret '{}' does not have a POSIX-compliant name",
key
);
}

Check warning on line 701 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L695-L701

Added lines #L695 - L701 were not covered by tests
}

let mut command = process::Command::new(shell);
command
.arg("-c")
.arg(&user_command)
.stdout(process::Stdio::inherit())
.stderr(process::Stdio::inherit());

if no_inherit_env {
let path = std::env::var("PATH").unwrap_or_else(|_| "/bin:/usr/bin".to_string());
command.env_clear();
command.env("PATH", path); // PATH is always necessary
command.envs(&environment);
} else {
command.envs(&environment);
Copy link
Contributor

Choose a reason for hiding this comment

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

for additional security, do we want to remove "BWS_ACCESS_TOKEN" from inherited env variables ?

Copy link
Contributor Author

@tangowithfoxtrot tangowithfoxtrot Jun 3, 2024

Choose a reason for hiding this comment

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

I have also wondered this. I added the --no-inherit-env argument for a similar reason, but I suppose I could remove that variable by default as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bwdil thoughts ?

Copy link

Choose a reason for hiding this comment

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

@mzieniukbw please unpack what you mean by 'for additional security' what is your concern? About having BWS_ACCESS_TOKEN inherited and how it gets subsequently used thereafter?

What are the security gains you're trying to achieve and what's the risk?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bwdil My though is that whatever get's executed as part of bws run is probably something that needs to run one off and just need read access to some of the secrets.
With BWS_ACCESS_TOKEN we expose much more than intended:

  • Access to all secrets that the token have access to - bws run can specify a project, but with access token we have access to every single of them
  • With access token you can do write operations - which is really bad in case it get's to wrong hands.

Copy link

Choose a reason for hiding this comment

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

@mzieniukbw if BWS_ACCESS_TOKEN is not required for subsequent commands that get spawned then it can be cleared from env to limit exposure. I think your concern is something that bws run invokes (another process) getting hijacked and then stealing the access token.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'll remove the BWS_ACCESS_TOKEN var by default as well then. Thanks for calling this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BWS_ACCESS_TOKEN is now unset by default.

}

Check warning on line 718 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L704-L718

Added lines #L704 - L718 were not covered by tests

let mut child = command.spawn().expect("failed to execute process");

let exit_status = child.wait().expect("process failed to execute");
let exit_code = exit_status.code().unwrap_or(1);

let _ = child.wait();
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is needed, we're already calling child.wait() above, any other calls will just return immediately, I think


if exit_code != 0 {
process::exit(exit_code);
}

Check warning on line 729 in crates/bws/src/main.rs

View check run for this annotation

Codecov / codecov/patch

crates/bws/src/main.rs#L720-L729

Added lines #L720 - L729 were not covered by tests
}

Commands::Config { .. } | Commands::Completions { .. } => {
unreachable!()
}
Expand Down
Loading