[SM-1129] Run command with secrets #621

tangowithfoxtrot · 2024-02-21T20:12:00Z

Type of change

Objective

Add a run command to allow running processes with secrets injected.

Example:
bws run 'docker compose up -d', bws run -- docker compose up -d, or from stdin: echo 'docker compose up -d' | bws run

Where the compose file is expecting secrets:

version: "3.9"
services:
  echo:
    image: hashicorp/http-echo
    container_name: echo
    ports:
      - "127.0.0.1:5678:5678"
    command: [
        "-text",
        "Local DB user: $LOCAL_DB_USER\nLocal DB pass: $LOCAL_DB_PASS", # secrets from Secrets Manager
      ]

Any command can be run as well: bws run -- npm run start, bws run -- ./some_script.sh, etc.

A --shell option is provided to override the default shell (sh on UNIX-like OSes, and powershell on Windows) where the process is executed.

A --no-inherit-env option is provided for additional safety in cases where you want to pass the minimum amount of values into a process. $PATH is always passed though, as omitting it would cause nearly every command to fail.

Code changes

crates/bws/src/main.rs: Add the run command and associated args.

Before you submit

Please add unit tests where it makes sense to do so

bitwarden-bot · 2024-02-21T20:17:26Z

Checkmarx One – Scan Summary & Details – f722b8ba-cad2-4b02-81cf-7b215dfc87da

No New Or Fixed Issues Found

codecov · 2024-02-21T20:17:38Z

Codecov Report

Attention: Patch coverage is 0% with 84 lines in your changes are missing coverage. Please review.

Project coverage is 61.30%. Comparing base (34b7213) to head (1ab92be).

Files	Patch %	Lines
crates/bws/src/main.rs	0.00%	84 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #621      +/-   ##
==========================================
- Coverage   61.77%   61.30%   -0.48%     
==========================================
  Files         172      172              
  Lines       10927    11011      +84     
==========================================
  Hits         6750     6750              
- Misses       4177     4261      +84

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ractive stdin

crates/bws/src/main.rs

dani-garcia

Nice, looks like a great start to me, left some comments with a few small improvements.

dani-garcia · 2024-04-30T09:41:58Z

crates/bws/src/main.rs

+            let shell = match std::env::consts::OS {
+                os if os == "linux" || os == "macos" || os.contains("bsd") => {
+                    shell.unwrap_or_else(|| "sh".to_string())
+                }
+                "windows" => shell.unwrap_or_else(|| "powershell".to_string()),
+                _ => unreachable!(),
+            };


Would it be better to invert this condition like this?

Suggested change

let shell = match std::env::consts::OS {

os if os == "linux" || os == "macos" || os.contains("bsd") => {

shell.unwrap_or_else(|| "sh".to_string())

}

"windows" => shell.unwrap_or_else(|| "powershell".to_string()),

_ => unreachable!(),

};

let shell = match std::env::consts::OS {

"windows" => shell.unwrap_or_else(|| "powershell".to_string()),

_ => shell.unwrap_or_else(|| "sh".to_string())

};

Any other platform that might fall under the unreachable line (maybe android?) probably has some kind of basic sh, so at least it would work.

dani-garcia · 2024-04-30T09:44:42Z

crates/bws/src/main.rs

@@ -222,6 +238,15 @@ enum DeleteCommand {
    Secret { secret_ids: Vec<Uuid> },
 }

+#[derive(Subcommand, Debug)]
+enum RunCommand {


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

dani-garcia · 2024-04-30T09:49:35Z

crates/bws/src/main.rs

+            no_inherit_env,
+            project_id,
+        } => {
+            let shell = match std::env::consts::OS {


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

dani-garcia · 2024-04-30T09:56:30Z

crates/bws/src/main.rs

+            }
+
+            let user_command = if command.is_empty() {
+                if atty::is(Stream::Stdin) {


This makes me think that when using the run command without extra arguments they'll be read from stdin, but for me it just prints the help. I tried running it as:

bws run

bws run --

bws run -- ""

bws run ""

Is there anything I'm missing?

dani-garcia · 2024-04-30T09:58:19Z

crates/bws/src/main.rs

+                .collect::<std::collections::HashMap<String, String>>();
+
+            let valid_key_regex =
+                regex::Regex::new("^[a-zA-Z_][a-zA-Z0-9_]*$").expect("valid regex");


We have the same regex in the render.rs function, should we extract it to an is_valid_posix_name function somewhere?

dani-garcia · 2024-04-30T09:59:38Z

crates/bws/src/main.rs

+            let environment = secrets
+                .iter()
+                .map(|s| (s.key.clone(), s.value.clone()))
+                .collect::<std::collections::HashMap<String, String>>();


If you use into_iter here, you can avoid the key and value clones

Suggested change

let environment = secrets

.iter()

.map(|s| (s.key.clone(), s.value.clone()))

.collect::<std::collections::HashMap<String, String>>();

let environment = secrets

.into_iter()

.map(|s| (s.key, s.value))

.collect::<std::collections::HashMap<String, String>>();

dani-garcia · 2024-04-30T10:04:41Z

crates/bws/src/main.rs

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


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

tangowithfoxtrot added 9 commits February 20, 2024 11:37

feat: add bws run command

07e1fe9

Merge branch 'main' into run-command-with-secrets

391d6b1

allow reading from stdin

bd75024

allow other shells

48f3841

print stdout+stderr as they happen

1e5d63a

inherit output instead of piping it manually

a52b989

return child process exit status

710ae61

display warning about problematic key names

f3ad7f3

add --no-inherit-env option

c7d29ad

tangowithfoxtrot added 6 commits February 21, 2024 12:22

appease clippy 📎

211d067

fix: wrong default shell on Windows

26af620

fix: determine OS at runtime (not compile time)

0f4d996

update warning message

7516a45

fix: run command hanging if no command was supplied; don't allow inte…

a19e820

…ractive stdin

Merge branch 'main' into run-command-with-secrets

bafdaad

differsthecat changed the title ~~Run command with secrets~~ [SM-1129] Run command with secrets Mar 6, 2024

tangowithfoxtrot added 3 commits March 18, 2024 03:59

Merge branch 'main' into run-command-with-secrets

5fc71d3

fix: panic on invalid shell

6ad88b8

cargo fmt

accc97e

github-advanced-security bot found potential problems Mar 18, 2024

View reviewed changes

crates/bws/src/main.rs Fixed Show fixed Hide fixed

tangowithfoxtrot added 3 commits March 18, 2024 13:10

fix: clippy::nonminimal_bool

b78284b

fix: clippy::nonminimal_bool

c2ee84c

Merge remote-tracking branch 'origin/main' into run-command-with-secrets

acbffe6

github-advanced-security bot found potential problems Apr 25, 2024

View reviewed changes

crates/bws/src/main.rs Fixed Show fixed Hide fixed

tangowithfoxtrot added 4 commits April 29, 2024 17:05

update hermit-abi

9eb8e9f

use expect instead of unwrap

4dfde95

cargo fmt

9fc608b

Merge branch 'main' into run-command-with-secrets

1ab92be

tangowithfoxtrot marked this pull request as ready for review April 29, 2024 19:04

tangowithfoxtrot requested review from dani-garcia, Hinton and a team April 29, 2024 19:04

dani-garcia reviewed Apr 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SM-1129] Run command with secrets #621

[SM-1129] Run command with secrets #621

tangowithfoxtrot commented Feb 21, 2024 •

edited

bitwarden-bot commented Feb 21, 2024 •

edited by github-actions bot

codecov bot commented Feb 21, 2024 •

edited

dani-garcia left a comment

dani-garcia Apr 30, 2024

dani-garcia Apr 30, 2024

dani-garcia Apr 30, 2024

dani-garcia Apr 30, 2024

dani-garcia Apr 30, 2024

dani-garcia Apr 30, 2024

dani-garcia Apr 30, 2024

[SM-1129] Run command with secrets #621

Are you sure you want to change the base?

[SM-1129] Run command with secrets #621

Conversation

tangowithfoxtrot commented Feb 21, 2024 • edited

Type of change

Objective

Code changes

Before you submit

bitwarden-bot commented Feb 21, 2024 • edited by github-actions bot

No New Or Fixed Issues Found

codecov bot commented Feb 21, 2024 • edited

Codecov Report

dani-garcia left a comment

Choose a reason for hiding this comment

dani-garcia Apr 30, 2024

Choose a reason for hiding this comment

dani-garcia Apr 30, 2024

Choose a reason for hiding this comment

dani-garcia Apr 30, 2024

Choose a reason for hiding this comment

dani-garcia Apr 30, 2024

Choose a reason for hiding this comment

dani-garcia Apr 30, 2024

Choose a reason for hiding this comment

dani-garcia Apr 30, 2024

Choose a reason for hiding this comment

dani-garcia Apr 30, 2024

Choose a reason for hiding this comment

tangowithfoxtrot commented Feb 21, 2024 •

edited

bitwarden-bot commented Feb 21, 2024 •

edited by github-actions bot

codecov bot commented Feb 21, 2024 •

edited