Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[SM-1129] Run command with secrets #621
Changes from 21 commits
07e1fe9
391d6b1
bd75024
48f3841
1e5d63a
a52b989
710ae61
f3ad7f3
c7d29ad
211d067
26af620
0f4d996
7516a45
a19e820
bafdaad
5fc71d3
6ad88b8
accc97e
b78284b
c2ee84c
acbffe6
9eb8e9f
4dfde95
9fc608b
1ab92be
ce96046
8e51cf9
68306b3
2e673b6
b6e676a
65b5492
f483b30
64c30c6
0e87279
606185c
30da24c
8cdf289
cd56439
14fa8df
cb9903a
9511ba2
55c9e72
0c6128a
09bdee7
9ae6078
4a4a8d8
cb6f987
eb29e7d
2da9ca9
deebd5d
2c5a279
a836b65
17a4d28
14a9e50
c93bf8f
5be08ed
e06e48f
3df1c6d
68973d0
83eb725
61b41fc
9f6a165
3080dac
23ab8f4
3fcd32c
c7aad3f
eda00aa
9839b94
7362764
f9629b8
15b1114
4d4922f
1c6ffbc
75e3d8b
e86aa2e
b86d026
f4c2598
f9456b8
e31c689
4a5aeb3
5bdf99c
f9c9946
a75f53e
4770dbf
87fc76d
bc1a6ff
a0f78a3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Check warning on line 92 in crates/bws/src/main.rs
Codecov / codecov/patch
crates/bws/src/main.rs#L92
Check warning on line 99 in crates/bws/src/main.rs
Codecov / codecov/patch
crates/bws/src/main.rs#L99
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
Check warning on line 244 in crates/bws/src/main.rs
Codecov / codecov/patch
crates/bws/src/main.rs#L244
Check warning on line 635 in crates/bws/src/main.rs
Codecov / codecov/patch
crates/bws/src/main.rs#L632-L635
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
Check warning on line 639 in crates/bws/src/main.rs
Codecov / codecov/patch
crates/bws/src/main.rs#L637-L639
Check warning on line 642 in crates/bws/src/main.rs
Codecov / codecov/patch
crates/bws/src/main.rs#L641-L642
Check warning on line 648 in crates/bws/src/main.rs
Codecov / codecov/patch
crates/bws/src/main.rs#L645-L648
Check warning on line 658 in crates/bws/src/main.rs
Codecov / codecov/patch
crates/bws/src/main.rs#L650-L658
Check warning on line 660 in crates/bws/src/main.rs
Codecov / codecov/patch
crates/bws/src/main.rs#L660
Check warning on line 667 in crates/bws/src/main.rs
Codecov / codecov/patch
crates/bws/src/main.rs#L663-L667
Check warning on line 673 in crates/bws/src/main.rs
Codecov / codecov/patch
crates/bws/src/main.rs#L669-L673
Check warning on line 678 in crates/bws/src/main.rs
Codecov / codecov/patch
crates/bws/src/main.rs#L675-L678
Check warning on line 685 in crates/bws/src/main.rs
Codecov / codecov/patch
crates/bws/src/main.rs#L681-L685
Check warning on line 693 in crates/bws/src/main.rs
Codecov / codecov/patch
crates/bws/src/main.rs#L688-L693
Check warning on line 701 in crates/bws/src/main.rs
Codecov / codecov/patch
crates/bws/src/main.rs#L695-L701
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bwdil thoughts ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:bws run
can specify a project, but with access token we have access to every single of themThere was a problem hiding this comment.
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 thatbws run
invokes (another process) getting hijacked and then stealing the access token.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
Codecov / codecov/patch
crates/bws/src/main.rs#L704-L718
There was a problem hiding this comment.
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
Check warning on line 729 in crates/bws/src/main.rs
Codecov / codecov/patch
crates/bws/src/main.rs#L720-L729