Skip to content

Fix nuget config not being honored #20930

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 17, 2021
Merged

Fix nuget config not being honored #20930

merged 3 commits into from
Sep 17, 2021

Conversation

wli3
Copy link

@wli3 wli3 commented Sep 10, 2021 •
edited
Loading

Copy user level nuget config to override folder

Tested end to end locally. Pretty hard to add unit test for permission code

@ghost ghost added the Area-Infrastructure label Sep 10, 2021
@wli3 wli3 requested review from sfoslund and mhutch September 10, 2021 23:47
KalleOlaviNiemitalo
KalleOlaviNiemitalo reviewed Sep 12, 2021
View reviewed changes
Copy link
Contributor

@KalleOlaviNiemitalo KalleOlaviNiemitalo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I feel suspicious about the hardcoded /tmp/dotnet_sudo_home/ — what if that directory has been maliciously created by a different, unprivileged user, who still has write access — but perhaps that was discussed in another PR.

src/Cli/dotnet/SudoEnvironmentDirectoryOverride.cs Outdated
Environment.SetEnvironmentVariable("HOME", SudoHomeDirectory);

CopyUserNuGetConfigToOverridenHome(homeBeforeOverride);
Copy link
Contributor

Choose a reason for hiding this comment

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

The spelling is "overridden".

sfoslund
sfoslund reviewed Sep 13, 2021
View reviewed changes
src/Cli/dotnet/SudoEnvironmentDirectoryOverride.cs
@@ -33,7 +37,57 @@ public static void OverrideEnvironmentVariableToTmp(ParseResult parseResult)
if (!OperatingSystem.IsWindows() && IsRunningUnderSudo() && IsRunningWorkloadCommand(parseResult))
{
Directory.CreateDirectory(SudoHomeDirectory);

var homeBeforeOverride = Path.Combine(Environment.GetEnvironmentVariable("HOME"));
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I must have missed this in the first check in of this change, but directly using the HOME env var doesn't match the logic we use for calculating the home dir here: https://github.com/dotnet/sdk/blob/release/6.0.1xx/src/Common/CliFolderPathCalculatorCore.cs#L35, should we be taking those other env vars into account?

Copy link
Author

Choose a reason for hiding this comment

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

That's fine. If the user override the home directory, they are in control of the situation. We just need to make sure the default make sense.

Fix nuget config not being honored
d38e558 
Copy user level nuget config to override folder
@wli3 wli3 force-pushed the fix-ignore-nugetconfig branch from 4e53840 to d38e558 Compare September 13, 2021 17:14
@wli3
Copy link
Author

wli3 commented Sep 13, 2021

@sfoslund looks good?

@wli3
Copy link
Author

wli3 commented Sep 14, 2021

@KalleOlaviNiemitalo thank you. I added logic to ensure the owner is root

@wli3 wli3 force-pushed the fix-ignore-nugetconfig branch from 01d5f50 to cb31351 Compare September 14, 2021 23:02
@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Sep 15, 2021 •
edited
Loading

@wli3 lstat might be more secure for that; wouldn't be tricked by a user-owned symlink to a root-owned directory. But I did not check whether you already prevent that in some other way, e.g. whether Directory.CreateDirectory would throw in that case.

KalleOlaviNiemitalo
KalleOlaviNiemitalo reviewed Sep 15, 2021
View reviewed changes
src/Cli/dotnet/SudoEnvironmentDirectoryOverride.cs Outdated
Comment on lines 114 to 129
private static bool IsOtherUserWritable(StatInterop.FileStatus fileStat)
{
return (fileStat.Mode & (int) StatInterop.Permissions.S_IWOTH) == 0;
}

private static bool IsGroupWritable(StatInterop.FileStatus fileStat)
{
return (fileStat.Mode & (int) StatInterop.Permissions.S_IWGRP) == 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The names of these methods are misleading.

Copy link
Author

Choose a reason for hiding this comment

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

could you give me more detail? Any suggestions?

Copy link
Author

Choose a reason for hiding this comment

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

@joeloff @KalleOlaviNiemitalo now I get it. Rename to "...CannotWrite"

@wli3 wli3 force-pushed the fix-ignore-nugetconfig branch from cb31351 to d239c27 Compare September 15, 2021 22:36
@wli3
Copy link
Author

wli3 commented Sep 15, 2021

@joeloff @sfoslund could you both have a look again? especially on d239c27

@wli3 wli3 force-pushed the fix-ignore-nugetconfig branch from d239c27 to e4cce51 Compare September 15, 2021 22:40
@wli3 wli3 changed the base branch from release/6.0.1xx to release/6.0.1xx-rc2 September 16, 2021 15:05
@wli3 wli3 force-pushed the fix-ignore-nugetconfig branch from b508d9d to ce76b1d Compare September 16, 2021 15:05
@wli3
Copy link
Author

wli3 commented Sep 16, 2021

@joeloff could I get an approval if the change looks good?

@wli3 wli3 force-pushed the fix-ignore-nugetconfig branch from ce76b1d to 54f18e9 Compare September 16, 2021 17:39
@wli3
Copy link
Author

wli3 commented Sep 16, 2021

/azp run

@wli3 wli3 enabled auto-merge September 16, 2021 22:50
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wli3 wli3 merged commit 5f1937d into dotnet:release/6.0.1xx-rc2 Sep 17, 2021
@wli3 wli3 deleted the fix-ignore-nugetconfig branch September 17, 2021 15:23
@wli3 wli3 mentioned this pull request Sep 17, 2021
Closed
@marcpopMSFT marcpopMSFT mentioned this pull request Sep 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KalleOlaviNiemitalo KalleOlaviNiemitalo KalleOlaviNiemitalo left review comments

@joeloff joeloff joeloff approved these changes

@sfoslund sfoslund sfoslund approved these changes

@mhutch mhutch Awaiting requested review from mhutch

Assignees
No one assigned
Labels
Area-Infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wli3 @KalleOlaviNiemitalo @joeloff @sfoslund