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

fix: Removing overridden AWS_CONFIG_FILE path and base environment variabl… #247

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

baxeaz
Copy link
Contributor

@baxeaz baxeaz commented Mar 26, 2024

What was the problem/requirement? (What/Why)

AWS_CONFIG_FILE was overridden in vfs.py's launch environment to be the "default" config file rather than the session created config file which we want to provide the VFS.

We were passing through the base os.environ variables unnecessarily, potentially exposing things we don't intend to even though the VFS is a trusted process we own.

Using POpen with sudo requires the -E option to properly persist the environment variables we pass through to the launched process.

What was the solution? (How)

Removing overridden AWS_CONFIG_FILE and base environment variables.

Launch VFS with -E option when using sudo -u to run as job-user

What is the impact of this change?

VFS should properly pick up provided credentials through installed config file's credential_process setting.

How was this change tested?

New tests added, all unit tests pass. Reenabled BealineWorkerImageTesting tests, ran with new deadline-cloud, all tests pass.

Was this change documented?

No

Is this a breaking change?

No

@baxeaz baxeaz requested a review from a team as a code owner March 26, 2024 14:58
@baxeaz baxeaz changed the title Removing overridden AWS_CONFIG_FILE path and base environment variabl… fix: Removing overridden AWS_CONFIG_FILE path and base environment variabl… Mar 26, 2024
…riables from deadline_vfs POpen launch env and using -E option to persist environment through sudo

Signed-off-by: Brian Axelson <[email protected]>
@ddneilson ddneilson added the security Pull requests that could impact security label Mar 26, 2024
Copy link
Contributor

@ddneilson ddneilson left a comment

Choose a reason for hiding this comment

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

Great! This looks good to me. We're explicitly passing the env var dictionary to Popen so there's not a risk of leaking env vars from this process into the VFS process.

@baxeaz baxeaz merged commit 4a7be81 into mainline Mar 26, 2024
18 checks passed
@baxeaz baxeaz deleted the vfslaunchenv branch March 26, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Pull requests that could impact security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants