Page MenuHomePhabricator

[dagster-aws] Fix issue with keys sticking around
ClosedPublic

Authored by natekupp on Fri, Oct 4, 6:05 PM.

Details

Reviewers
alangenfeld
Group Reviewers
Restricted Project
Commits
R1:491064d04791: [dagster-aws] Fix issue with keys sticking around
Test Plan

manual tested delete after both dagster-aws init, dagster-aws up, and just dagster-aws init

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

natekupp created this revision.Fri, Oct 4, 6:05 PM
alangenfeld added inline comments.
python_modules/libraries/dagster-aws/dagster_aws/cli/cli.py
434

why shell=True? I feel like thats a thing you are supposed to avoid. Should we do subprocess.check_call to propagate failures up

alangenfeld accepted this revision.Fri, Oct 4, 6:08 PM
This revision is now accepted and ready to land.Fri, Oct 4, 6:08 PM
natekupp added inline comments.Fri, Oct 4, 6:30 PM
python_modules/libraries/dagster-aws/dagster_aws/cli/cli.py
434

yah its a potential security risk because shell=true will pass unsanitized stuff directly to the underlying shell you're using—IMO not a more substantial risk than some of the other stuff we're doing in this script—in this case the risk would be that the user puts something nasty in their config yaml which is then passed along as the key_file_path here.

Could either do it this way or exceptions-as-control-flow where we have to try / except CalledProcessError since the key may not have been added to the SSH agent yet. Thoughts? you have a preference?

natekupp updated this revision to Diff 5622.Fri, Oct 4, 8:59 PM

well this was fun

natekupp updated this revision to Diff 5623.Fri, Oct 4, 9:01 PM

clean up

well that got a bit more complicated

python_modules/libraries/dagster-aws/dagster_aws_tests/cli_tests/test_cli.py
34–44

yaytests

natekupp updated this revision to Diff 5625.Fri, Oct 4, 9:17 PM

ensure SSH is running on buildkite (maybe??)

natekupp updated this revision to Diff 5626.Fri, Oct 4, 9:26 PM

maybe maybe

natekupp updated this revision to Diff 5627.Fri, Oct 4, 9:29 PM

permissions