Discover plugins installed using Net tools #5990

Nigusu-Allehu · 2024-08-24T19:20:23Z

Bug

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

Meaningful title, helpful description and a linked NuGet/Home issue
Added tests
Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

dotnet-policy-service · 2024-09-03T06:37:03Z

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

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs

…uGet/NuGet.Client into dev-nyenework-net-tools-plugin

src/NuGet.Core/NuGet.Protocol/NuGet.Protocol.csproj

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs

test/NuGet.Core.Tests/NuGet.Protocol.Tests/Plugins/PluginDiscovererTests.cs

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs

kartheekp-ms · 2024-09-12T00:47:00Z

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs

+                    {
+                        if (IsValidPluginFile(file))
+                        {
+                            var state = new Lazy<PluginFileState>(() => _verifier.IsValid(file.FullName) ? PluginFileState.Valid : PluginFileState.InvalidEmbeddedSignature);


.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.

Is the suggestion to not check the validity of the package and just assume it is valid?

Yes, that is correct.

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.

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.

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs

kartheekp-ms · 2024-09-13T18:42:33Z

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs

+            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>();


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.

I removed it and now the logic is

If NUGET_NETFX_PLUGIN_PATHS or NUGET_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?

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs

kartheekp-ms · 2024-09-13T19:12:37Z

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs

+                    (fileMode & UnixFileMode.GroupExecute) != 0 ||
+                    (fileMode & UnixFileMode.OtherExecute) != 0);
+#else
+                return fileInfo.Exists && (fileInfo.Attributes & FileAttributes.ReparsePoint) == 0 && IsExecutable(fileInfo);


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.

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.

zivkan · 2024-09-19T07:44:12Z

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs

+                if (string.IsNullOrEmpty(_rawPluginPaths))
+                {
+                    // Nuget plugin configuration environmental variables were not used to discover the configuration files
+                    _pluginFiles.AddRange(GetNetToolsPluginFiles());


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?

zivkan · 2024-09-19T07:49:28Z

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs

                _pluginFiles = GetPluginFiles(cancellationToken);
+
+                if (string.IsNullOrEmpty(_rawPluginPaths))
+                {
+                    // Nuget plugin configuration environmental variables were not used to discover the configuration files
+                    _pluginFiles.AddRange(GetNetToolsPluginFiles());


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.

zivkan · 2024-09-19T07:55:20Z

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs

+                    {
+                        if (IsValidPluginFile(file))
+                        {
+                            var state = new Lazy<PluginFileState>(() => _verifier.IsValid(file.FullName) ? PluginFileState.Valid : PluginFileState.InvalidEmbeddedSignature);


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.

Discover Net tools installed plugins

4db9964

Nigusu-Allehu requested a review from a team as a code owner August 24, 2024 19:20

Nigusu-Allehu marked this pull request as draft August 24, 2024 19:20

Nigusu-Allehu self-assigned this Aug 26, 2024

dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 3, 2024

Nigusu-Allehu removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 3, 2024

Nigusu-Allehu added 2 commits September 3, 2024 11:09

Read unix executable plugins

bd202c4

Add tests

b199f4e

Nigusu-Allehu marked this pull request as ready for review September 3, 2024 19:21

Nigusu-Allehu and others added 4 commits September 3, 2024 12:22

cleanup

c48c415

Linux test

859754a

Linux test

2a9ed74

Merge branch 'dev-nyenework-net-tools-plugin' of https://github.com/N…

c573bb9

…uGet/NuGet.Client into dev-nyenework-net-tools-plugin

zivkan reviewed Sep 3, 2024

View reviewed changes

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs Show resolved Hide resolved

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs Outdated Show resolved Hide resolved

Use GetUnixFileMode

f533021

Nigusu-Allehu requested a review from zivkan September 4, 2024 22:56

Nigusu-Allehu and others added 8 commits September 5, 2024 10:15

Fix test

42ff98f

white space

bbdee18

Add more tests

e7776f2

Merge branch 'dev-nyenework-net-tools-plugin' of https://github.com/N…

405a28f

…uGet/NuGet.Client into dev-nyenework-net-tools-plugin

fix tests

7a6d5e9

Comment

c4638c4

use /bin/bash

8c51a6c

Fix assert

1d7ba24

Nigusu-Allehu added this to the 6.12 milestone Sep 5, 2024

Nigusu-Allehu changed the title ~~Discover Net tools installed plugins~~ Discover plugins installed using Net tools Sep 9, 2024

kartheekp-ms reviewed Sep 10, 2024

View reviewed changes

src/NuGet.Core/NuGet.Protocol/NuGet.Protocol.csproj Show resolved Hide resolved

zivkan reviewed Sep 10, 2024

View reviewed changes

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs Show resolved Hide resolved

test/NuGet.Core.Tests/NuGet.Protocol.Tests/Plugins/PluginDiscovererTests.cs Outdated Show resolved Hide resolved

Cleanup testing

425767e

Nigusu-Allehu requested review from zivkan and kartheekp-ms September 10, 2024 17:59

kartheekp-ms reviewed Sep 11, 2024

View reviewed changes

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs Outdated Show resolved Hide resolved

kartheekp-ms reviewed Sep 12, 2024

View reviewed changes

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs Outdated Show resolved Hide resolved

kartheekp-ms reviewed Sep 12, 2024

View reviewed changes

Add NUGET_PLUGIN_PATHS

668ba03

Nigusu-Allehu requested a review from kartheekp-ms September 12, 2024 18:51

Nigusu-Allehu and others added 2 commits September 12, 2024 12:07

Clean code

1e2e41e

use empty body constructor

a9fcab7

kartheekp-ms reviewed Sep 13, 2024

View reviewed changes

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs Outdated Show resolved Hide resolved

kartheekp-ms reviewed Sep 13, 2024

View reviewed changes

src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs Show resolved Hide resolved

kartheekp-ms reviewed Sep 13, 2024

View reviewed changes

NuGetPluginPaths Env variable used by dotnet tools only

561db34

Nigusu-Allehu requested a review from kartheekp-ms September 13, 2024 22:26

zivkan reviewed Sep 19, 2024

View reviewed changes

nkolev92 added the Merge next release PRs that should not be merged until the dev branch targets the next release label Sep 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discover plugins installed using Net tools #5990

Discover plugins installed using Net tools #5990

Nigusu-Allehu commented Aug 24, 2024 •

edited

Loading

dotnet-policy-service bot commented Sep 3, 2024

kartheekp-ms Sep 12, 2024 •

edited

Loading

Nigusu-Allehu Sep 12, 2024

kartheekp-ms Sep 13, 2024

Nigusu-Allehu Sep 13, 2024

zivkan Sep 19, 2024

kartheekp-ms Sep 13, 2024

Nigusu-Allehu Sep 13, 2024 •

edited

Loading

kartheekp-ms Sep 13, 2024 •

edited

Loading

Nigusu-Allehu Sep 13, 2024

zivkan Sep 19, 2024

zivkan Sep 19, 2024

zivkan Sep 19, 2024

	if (string.IsNullOrEmpty(_rawPluginPaths))
	{
	_rawPluginPaths = reader.GetEnvironmentVariable(EnvironmentVariableConstants.PluginPaths);
	}

Discover plugins installed using Net tools #5990

Are you sure you want to change the base?

Discover plugins installed using Net tools #5990

Conversation

Nigusu-Allehu commented Aug 24, 2024 • edited Loading

Bug

Description

PR Checklist

dotnet-policy-service bot commented Sep 3, 2024

kartheekp-ms Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nigusu-Allehu Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

kartheekp-ms Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nigusu-Allehu commented Aug 24, 2024 •

edited

Loading

kartheekp-ms Sep 12, 2024 •

edited

Loading

Nigusu-Allehu Sep 13, 2024 •

edited

Loading

kartheekp-ms Sep 13, 2024 •

edited

Loading