-
Notifications
You must be signed in to change notification settings - Fork 688
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
Discover plugins installed using Net tools #5990
base: dev-feature-dot-net-tool-plugin-support
Are you sure you want to change the base?
Discover plugins installed using Net tools #5990
Conversation
This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch. |
…uGet/NuGet.Client into dev-nyenework-net-tools-plugin
test/NuGet.Core.Tests/NuGet.Protocol.Tests/Plugins/PluginDiscovererTests.cs
Outdated
Show resolved
Hide resolved
{ | ||
if (IsValidPluginFile(file)) | ||
{ | ||
var state = new Lazy<PluginFileState>(() => _verifier.IsValid(file.FullName) ? PluginFileState.Valid : PluginFileState.InvalidEmbeddedSignature); |
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.
.NET SDK recently enabled package signature verification for .NET tools. Hence, I think we can skip the _verifier.IsValid
check because .NET tools are published as NuGet packages, which can be signed by the package author.
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.
Is the suggestion to not check the validity of the package and just assume it is valid?
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.
Yes, that is correct.
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.
From my understanding, isValid checks whether the file has a valid embedded signature or not. Even though we do expect the files that come from .Net tools to have a valid signature. Isn't possible some file that was not installed by .Net tools exists in the path and it does not have a valid signature. Or it could be a .Net tools package that has been tempered with and does not have a valid signature anymore.
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 "embedded signature" is an authenticode signature, which is not related to signed packages.
It typically costs hundreds of dollars a year to buy a code signing certificate, putting it out of reach for most non-corporate NuGet plugin developers. Even when a developer does have a certificate available for the production code, it's typically not available during development and debugging. I hit this today while trying to develop a new plugin, and I had to hardcode Valid
state just to be able to develop the plugin.
At the very least we should add an environment variable to allow skipping the authenticode checks, so that plugin developers can actually develop. However, I'm in favour of removing the authenticode checks altogether. If someone wants to allow only signed executables to run on a Windows machine, they can configure it via group policy.
string[] paths = Array.Empty<string>(); | ||
|
||
// The path to the plugins installed using dotnet tools, should be specified in the NUGET_PLUGIN_PATHS environment variable. | ||
paths = _environmentVariableReader.GetEnvironmentVariable("NUGET_PLUGIN_PATHS")?.Split(Path.PathSeparator) ?? Array.Empty<string>(); |
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.
If the NUGET_PLUGIN_PATHS
environment variable is already set, the _rawpluginpaths
parameter passed to the constructor will contain a list of paths to plugin executables that are installed as .NET tools going forward.
NuGet.Client/src/NuGet.Core/NuGet.Protocol/Plugins/PluginManager.cs
Lines 320 to 323 in fef6562
if (string.IsNullOrEmpty(_rawPluginPaths)) | |
{ | |
_rawPluginPaths = reader.GetEnvironmentVariable(EnvironmentVariableConstants.PluginPaths); | |
} |
I think we should remove the logic in PluginManager
for discovering .NET tool plugins because they have different verification logic. For example, the file name should start with nuget-plugin-*
, the executable bit should be set for non-Windows platforms, and the file should have a .exe or .bat extension on Windows.
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 removed it and now the logic is
- If
NUGET_NETFX_PLUGIN_PATHS
orNUGET_NETCORE_PLUGIN_PATHS
are set, use them. - Otherwise if
NUGET_PLUGIN_PATHS
is set, assume it is pointing to the directory that contains the dotnet tools installed plugins. If none of the env variables are set, go a head and scan for the plugins from the directories listed in PATH.
This limits NUGET_PLUGIN_PATHS
to be used for .NET tools plugins only. Would that not be a problem for users who use it for non .NET tools plugins previously?
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.
Great question, Nigusu. NuGet/Home#13242 (comment) conversation in the spec, led us to the decision of using the existing environment variable (NUGET_PLUGIN_PATHS) for .NET tools plugins. There are few references to this environment variable in other repositories such as JetBrains, OneGet to name a few https://github.com/search?q=NUGET_PLUGIN_PATHS&type=code. The idea is to allow customers to install .NET tools as tool path tools instead of global tools. Maybe we can add a breaking change label to the issue so that it can be part of the release notes.
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 follow why we have to differentiate between dotnet tool and non dotnet tool with NUGET_PLUGIN_PATHS.
Don't we just execute the with the full path of whatever was passed in there and how it starts it not a concern?
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.
On .NET (core) plugins using the old discovery method are executed via dotnet {file}.dll
, whereas the new executables are executed directly, since they're executables.
(fileMode & UnixFileMode.GroupExecute) != 0 || | ||
(fileMode & UnixFileMode.OtherExecute) != 0); | ||
#else | ||
return fileInfo.Exists && (fileInfo.Attributes & FileAttributes.ReparsePoint) == 0 && IsExecutable(fileInfo); |
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.
Please help me understand the reasoning behind the (fileInfo.Attributes & FileAttributes.ReparsePoint == 0)
check. My internet search suggested it is related to symbolic links, etc., but I happy to learn more.
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.
You're right. I wanted to ensure the file both exists and is a real file (not a symbolic link) to minimize the risk of running malicious code. For example, imagine a symbolic link located in one of the directories listed in the user's PATH
environment variable. The link has a name like nuget-plugin-*
, making it appear like a normal plugin, but it actually points to a malicious file in a different directory. If we run that symlink, we unintentionally execute the malicious file from the other location. We could say it is up to the user to ensure there is no malicious files in the PATH folders. However, I don't see a reason why we should leave that vulnerability open.
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.
It's great that you're thinking about security! Let's talk about this offline. I don't know how I feel about discussing security publicly. Maybe it's fine, and a good learning resource for people, but maybe we don't want to give bad guys an insight into our security decisions.
if (string.IsNullOrEmpty(_rawPluginPaths)) | ||
{ | ||
// Nuget plugin configuration environmental variables were not used to discover the configuration files | ||
_pluginFiles.AddRange(GetNetToolsPluginFiles()); |
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 XMLdoc for rawPluginPaths
says:
The raw semicolon-delimited list of supposed plugin file paths
But I don't understand when the value will be null or not null, or where the value comes from, and therefore I'm having a hard time understanding why we look for .NET tool plugins only when it's empty.
Is rawPluginPaths
the value of the NUGET_PLUGIN_PATHS
or NUGET_NETFX_PLUGIN_PATHS
environment variables when they're set?
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.
Is rawPluginPaths the value of the NUGET_PLUGIN_PATHS or NUGET_NETFX_PLUGIN_PATHS environment variables when they're set?
Yes Andy, rawPluginPaths
is the value of the NUGET_NETFX_PLUGIN_PATHS
or NUGET_NETCORE_PLUGIN_PATHS
or NUGET_PLUGIN_PATHS
environment variables when they are set. There is a precedence for these environment variables which is documented here.
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.
perhaps _overridePath
is clearer.
Plus XMLDoc comments that explains this, because even reading the docs, it's not super clear where the value for this _rawPluginPaths
property comes from. Even knowing the override feature, it sounds to me like this value should never be null, and when the CLI arg is not provided, it should contain the default plugin path.
(also I'm lazy, so I don't want to read the docs, if the comments explain that the CLI arg allows the customer to override normal discovery, and discover only plugins in a specific directory, we won't need to context switch to the docs, and this is assuming the person reading the code even realises that there are relevant docs)
_pluginFiles = GetPluginFiles(cancellationToken); | ||
|
||
if (string.IsNullOrEmpty(_rawPluginPaths)) | ||
{ | ||
// Nuget plugin configuration environmental variables were not used to discover the configuration files | ||
_pluginFiles.AddRange(GetNetToolsPluginFiles()); |
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.
Today, I've been trying to create a cred provider. I used the nuget-plugin-*.exe
file convention, but setting NUGET_NETFX_PLUGIN_PATHS
or NUGET_PLUGIN_PATHS
to the bin directory of my project, or setting it to the full path of the exe itself, I couldn't get NuGet to use my plugin, without adding the project's bin directory to the path (and using your test branch for this feature that does execution, in addition to discovery). However, when doing this, it also discovered other cred providers I have installed, which was making debuggign my plugin much more difficult.
It can be done in a follow up, but I think there should be a way to disable normal plugin discovery, and only discover plugins I want. The NUGET_PLUGIN_PATHS
environment variable appears to do that for the older discovery method, so there should be a way for this new one as well.
{ | ||
if (IsValidPluginFile(file)) | ||
{ | ||
var state = new Lazy<PluginFileState>(() => _verifier.IsValid(file.FullName) ? PluginFileState.Valid : PluginFileState.InvalidEmbeddedSignature); |
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 "embedded signature" is an authenticode signature, which is not related to signed packages.
It typically costs hundreds of dollars a year to buy a code signing certificate, putting it out of reach for most non-corporate NuGet plugin developers. Even when a developer does have a certificate available for the production code, it's typically not available during development and debugging. I hit this today while trying to develop a new plugin, and I had to hardcode Valid
state just to be able to develop the plugin.
At the very least we should add an environment variable to allow skipping the authenticode checks, so that plugin developers can actually develop. However, I'm in favour of removing the authenticode checks altogether. If someone wants to allow only signed executables to run on a Windows machine, they can configure it via group policy.
@@ -1,6 +1,6 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
<PropertyGroup> | |||
<TargetFrameworks>$(TargetFrameworksLibraryForSigning)</TargetFrameworks> | |||
<TargetFrameworks>$(TargetFrameworksLibraryForSigning);$(NETCoreTargetFramework)</TargetFrameworks> |
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.
Adding a framework to one of our assemblies is a significant change, and one that had both me & Kartheek wondering what it's for.
It's something that should be a part of the PR description.
I think there's some side-effects we need to work through, some documented in https://github.com/NuGet/NuGet.Client/blob/dev/docs/updating-target-frameworks.md:
- The previous frameworks were net472, ns2.0, net5.0 and now net8.0.
The test frameworks are net472;net8.0;netcoreapp3.1
Each test framework will be running against the most compatible NuGet.Protocol framework.
net472 tests net472, net8.0 tests net8.0, netcoreapp3.1 tests ns2.0.
This leaves net5.0 untested.
- There's also the source build considerations. This change may lead to some duplicates in the source build work that may break them.
Let's explore what options we have and make the best decision.
Some potential ideas we can discuss (there's probably more)
- Say we won't test net5.0 from NuGet.Protocol
- Bump all the frameworks from net5.0 to net8.0 (breaking change for our packages, but I it could simplify our testing)
- make NuGet.Protocol target net8.0 only instead of net5.0
/// The files are also expected to have a name that starts with `nuget-plugin-` | ||
/// </summary> | ||
/// <returns></returns> | ||
private List<PluginFile> GetNetToolsPluginFiles() |
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 is probably a method that can be tested independently of the rest.
/// </summary> | ||
/// <param name="fileInfo"></param> | ||
/// <returns></returns> | ||
private static bool IsValidPluginFile(FileInfo fileInfo) |
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.
Similar as the previous one, I think a bunch of these can be unit tested.
That way you're not testing all combinations in integration tests.
@@ -317,11 +317,6 @@ private void Initialize(IEnvironmentVariableReader reader, | |||
#else | |||
_rawPluginPaths = reader.GetEnvironmentVariable(EnvironmentVariableConstants.CorePluginPaths); | |||
#endif | |||
if (string.IsNullOrEmpty(_rawPluginPaths)) |
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.
Can you help me understand why we're changing the location of this call from PluginManager to PluginDiscoverer?
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 change was made as per #5990 (comment) conversation.
/// <summary> | ||
/// Is the plugin file, a dotnet tools plugin file? | ||
/// </summary> | ||
public bool IsDotnetToolsPlugin { get; } |
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.
Is this class ever used outside of this assembly?
It seems to me that we never really needed it to the public.
We can consider not making public changes.
Bug
Fixes: NuGet/Home#13740
Description
This PR makes sure NuGet discovers credential providers installed using .NET tools.
dotnet tool install --global should install the tool and add its path to the PATH environmental varibale. Iterate through all the paths in PATH and all the files in the path to find the plugins. By design, we expect these plugins to be named nuget-plugin*.
PR Checklist