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

Discover plugins installed using Net tools #5990

Open
wants to merge 21 commits into
base: dev-feature-dot-net-tool-plugin-support
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/NuGet.Core/NuGet.Protocol/NuGet.Protocol.csproj
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>$(TargetFrameworksLibraryForSigning)</TargetFrameworks>
<TargetFrameworks>$(TargetFrameworksLibraryForSigning);$(NETCoreTargetFramework)</TargetFrameworks>
Nigusu-Allehu marked this conversation as resolved.
Show resolved Hide resolved
<TargetFramework />
<NoWarn>$(NoWarn);CS1591;CS1573;CS0012;RS0041</NoWarn>
<PackageTags>nuget protocol</PackageTags>
Expand Down
132 changes: 131 additions & 1 deletion src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Runtime.InteropServices;
using System.Threading;
using System.Threading.Tasks;
using NuGet.Common;
Expand All @@ -21,14 +24,21 @@ public sealed class PluginDiscoverer : IPluginDiscoverer
private IEnumerable<PluginDiscoveryResult> _results;
private readonly SemaphoreSlim _semaphore;
private readonly EmbeddedSignatureVerifier _verifier;
private readonly IEnvironmentVariableReader _environmentVariableReader;

public PluginDiscoverer(string rawPluginPaths, EmbeddedSignatureVerifier verifier)
: this(rawPluginPaths, verifier, EnvironmentVariableWrapper.Instance)
{
}

/// <summary>
/// Instantiates a new <see cref="PluginDiscoverer" /> class.
/// </summary>
/// <param name="rawPluginPaths">The raw semicolon-delimited list of supposed plugin file paths.</param>
/// <param name="verifier">An embedded signature verifier.</param>
/// <param name="environmentVariableReader"> A reader for environmental varibales. </param>
/// <exception cref="ArgumentNullException">Thrown if <paramref name="verifier" /> is <see langword="null" />.</exception>
public PluginDiscoverer(string rawPluginPaths, EmbeddedSignatureVerifier verifier)
internal PluginDiscoverer(string rawPluginPaths, EmbeddedSignatureVerifier verifier, IEnvironmentVariableReader environmentVariableReader)
{
if (verifier == null)
{
Expand All @@ -38,6 +48,7 @@ public PluginDiscoverer(string rawPluginPaths, EmbeddedSignatureVerifier verifie
_rawPluginPaths = rawPluginPaths;
_verifier = verifier;
_semaphore = new SemaphoreSlim(initialCount: 1, maxCount: 1);
_environmentVariableReader = environmentVariableReader;
}

/// <summary>
Expand Down Expand Up @@ -85,6 +96,13 @@ public async Task<IEnumerable<PluginDiscoveryResult>> DiscoverAsync(Cancellation
}

_pluginFiles = GetPluginFiles(cancellationToken);

if (string.IsNullOrEmpty(_rawPluginPaths))
{
// Nuget plugin configuration environmental variables were not used to discover the configuration files
_pluginFiles.AddRange(GetNetToolsPluginFiles());
Comment on lines +100 to +103
Copy link
Member

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?

Comment on lines 98 to +103
Copy link
Member

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.

}

var results = new List<PluginDiscoveryResult>();

for (var i = 0; i < _pluginFiles.Count; ++i)
Expand Down Expand Up @@ -140,6 +158,118 @@ private List<PluginFile> GetPluginFiles(CancellationToken cancellationToken)
return files;
}

/// <summary>
/// Gets auth plugins installed using dotnet tools. This is done by iterating through each file in directories found int eh
/// `PATH` environment variable.
/// The files are also expected to have a name that starts with `nuget-plugin-`
/// </summary>
/// <returns></returns>
private List<PluginFile> GetNetToolsPluginFiles()
Nigusu-Allehu marked this conversation as resolved.
Show resolved Hide resolved
{
var pluginFiles = new List<PluginFile>();
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(EnvironmentVariableConstants.PluginPaths)?.Split(Path.PathSeparator) ?? Array.Empty<string>();

if (paths.Length == 0)
{
// If NUGET_PLUGIN_PATHS is not specified, read all the paths in PATH
paths = _environmentVariableReader.GetEnvironmentVariable("PATH")?.Split(Path.PathSeparator) ?? Array.Empty<string>();
}

foreach (var path in paths)
{
if (Directory.Exists(path))
{
var directoryInfo = new DirectoryInfo(path);
var files = directoryInfo.GetFiles("nuget-plugin-*");

foreach (var file in files)
{
if (IsValidPluginFile(file))
{
var state = new Lazy<PluginFileState>(() => _verifier.IsValid(file.FullName) ? PluginFileState.Valid : PluginFileState.InvalidEmbeddedSignature);
Copy link
Contributor

@kartheekp-ms kartheekp-ms Sep 12, 2024

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.

Copy link
Contributor Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is correct.

Copy link
Contributor Author

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.

Copy link
Member

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.

PluginFile pluginFile = new PluginFile(file.FullName, state, isDotnetToolsPlugin: true);
pluginFiles.Add(pluginFile);
}
}
}
}

return pluginFiles;
}

/// <summary>
/// Checks whether a file is a valid plugin file for windows/Unix.
/// Windows: It should be either .bat or .exe
/// Unix: It should be executable
/// </summary>
/// <param name="fileInfo"></param>
/// <returns></returns>
private static bool IsValidPluginFile(FileInfo fileInfo)
{
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
return fileInfo.Extension.Equals(".exe", StringComparison.OrdinalIgnoreCase) ||
fileInfo.Extension.Equals(".bat", StringComparison.OrdinalIgnoreCase);
}
else
{
#if NET8_0_OR_GREATER
var fileMode = File.GetUnixFileMode(fileInfo.FullName);

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

@kartheekp-ms kartheekp-ms Sep 13, 2024

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.

Copy link
Contributor Author

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.

#endif
}
}

#if !NET8_0_OR_GREATER
/// <summary>
/// Checks whether a file is executable or not in Unix.
/// This is done by running bash code: `if [ -x {fileInfo.FullName} ]; then echo yes; else echo no; fi`
/// </summary>
/// <param name="fileInfo"></param>
/// <returns></returns>
private static bool IsExecutable(FileInfo fileInfo)
Nigusu-Allehu marked this conversation as resolved.
Show resolved Hide resolved
Nigusu-Allehu marked this conversation as resolved.
Show resolved Hide resolved
{
#pragma warning disable CA1031 // Do not catch general exception types
try
{
string output;
using (var process = new Process())
{
// Use a shell command to check if the file is executable
process.StartInfo.FileName = "/bin/bash";
process.StartInfo.Arguments = $"if [ -x {fileInfo.FullName} ]; then echo yes; else echo no; fi";
process.StartInfo.UseShellExecute = false;
process.StartInfo.RedirectStandardOutput = true;

process.Start();
if (!process.WaitForExit(1000) || process.ExitCode != 0)
{
return false;
}

output = process.StandardOutput.ReadToEnd().Trim();
}

// Check if the output is "yes"
return output.Equals("yes", StringComparison.OrdinalIgnoreCase);
}
catch
{
return false;
}
#pragma warning restore CA1031 // Do not catch general exception types
}
#endif

private IEnumerable<string> GetPluginFilePaths()
{
if (string.IsNullOrEmpty(_rawPluginPaths))
Expand Down
16 changes: 16 additions & 0 deletions src/NuGet.Core/NuGet.Protocol/Plugins/PluginFile.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,22 @@ public sealed class PluginFile
/// </summary>
public Lazy<PluginFileState> State { get; }

/// <summary>
/// Is the plugin file, a dotnet tools plugin file?
/// </summary>
public bool IsDotnetToolsPlugin { get; }

/// <summary>
/// Instantiates a new <see cref="PluginFile" /> class.
/// </summary>
/// <param name="filePath">The plugin's file path.</param>
/// <param name="state">A lazy that evaluates the plugin file state.</param>
/// <param name="isDotnetToolsPlugin">Is the plugin file, a dotnet tools plugin file?</param>
public PluginFile(string filePath, Lazy<PluginFileState> state, bool isDotnetToolsPlugin) : this(filePath, state)
{
IsDotnetToolsPlugin = isDotnetToolsPlugin;
}

/// <summary>
/// Instantiates a new <see cref="PluginFile" /> class.
/// </summary>
Expand Down
5 changes: 0 additions & 5 deletions src/NuGet.Core/NuGet.Protocol/Plugins/PluginManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -317,11 +317,6 @@ private void Initialize(IEnvironmentVariableReader reader,
#else
_rawPluginPaths = reader.GetEnvironmentVariable(EnvironmentVariableConstants.CorePluginPaths);
#endif
if (string.IsNullOrEmpty(_rawPluginPaths))
{
_rawPluginPaths = reader.GetEnvironmentVariable(EnvironmentVariableConstants.PluginPaths);
}

_connectionOptions = ConnectionOptions.CreateDefault(reader);

var idleTimeoutInSeconds = EnvironmentVariableReader.GetEnvironmentVariable(EnvironmentVariableConstants.IdleTimeout);
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
#nullable enable
NuGet.Protocol.Plugins.PluginFile.IsDotnetToolsPlugin.get -> bool
~NuGet.Protocol.Plugins.PluginFile.PluginFile(string filePath, System.Lazy<NuGet.Protocol.Plugins.PluginFileState> state, bool isDotnetToolsPlugin) -> void
Loading
Loading