C#: Automatically use configured private registry feeds by mbg · Pull Request #18850 · github/codeql · GitHub

When private package registries are configured for Default Setup, we initialise the Dependabot proxy to provide an authenticated proxy for those registries. As of #18029, the C# extractor is able to make use of the authenticated Dependabot proxy to connect to those private package registries.

However, in some scenarios, the address of the private package registries is not actually stored in the nuget.config files for a project. Since we only restore dependencies from feeds that are configured in nuget.config files, we are unable to restore dependencies from private registries in this case. Ordinary CI workflows for such projects rely on injecting the feed addresses into the configuration.

This PR modifies the NugetPackageRestorer to "inject" any private package registry feeds that are configured through the UI into the package restore process by passing them to dotnet on the command-line. This allows private package registries to be used without needing the address to be stored in the repository.

The CodeQL Action that initialises the Dependabot proxy provides the private registry feed URLs as an output as of github/codeql-action#2652 (and annotates them with their type as of github/codeql-action#2672). A future version of the Default Setup workflow will propagate that output to the analyze step / CodeQL extractors.

Approach

The approach I chose for this implementation is to detect when the CODEQL_PROXY_URLS environment is set, extract nuget feeds from the JSON array that is expected to be stored in it, and then pass those feed URLs, along with others that we discovered in the nuget.config files, to dotnet on the command-line.

Using the command-line has the advantage that we do not need to modify (and later clean-up) configuration files on disk, but the disadvantage that dotnet will only use feeds given to it on the command-line and ignore those in configuration files entirely. This means that we must provide all feeds that we want to use on the command-line.

To minimise the risk of breaking what was already working before, nuget feeds are only passed to dotnet on the command-line when the Dependabot proxy is configured (i.e. when private package registries are being used). If not, then the old behaviour of implicitly using nuget.config files continues to be used.

…ne arguments

Is there any particular reason/benefit to declare this as a struct?

No particular reason. I thought I was mirroring a similar setup elsewhere in the codebase, but happy to change it.

Do we expect the array to be null? Should we log if it is null?

The main reason for the check is just to make sure we don't work with a null value. We could log it, but I have no strong view on it either way.

Thank you @mbg !
I really appreciate the PR explanation and inline code comments - it makes it much easier to review.
This adds an extra layer of configuration; Is it intended to work in conjunction with the other environment variables feeds (for instance excluding a feed from a reachability check)?

Consider constructing the "source" string at this location instead. Otherwise it will be constructed once for each project that is restored in the parallel restore loop.

Done in d564529

To avoid timeouts, maybe there should be a feed validation check similar to:

codeql/csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/NugetPackageRestorer.cs

Line 692 in 72346cc

    
           var allFeedsReachable = explicitFeeds.All(feed => excludedFeeds.Contains(feed) || IsFeedReachable(feed, initialTimeout, tryCount));

Done in 4448369

Co-authored-by: Michael Nebel <michaelnebel@github.com>

This allows the string of package feeds to be constructed once and used repeatedly in the parallel restore loop as well.

…a given set of feeds.

Thank you for looking into the comments and thank you for doing this!!! 👍

If the dependabot proxy is set, we also explicitly add the public nuget org feed as a source - so we should probably check whether that feed is reachable as well.

I have changed this so that the public feed is not manually added when private registries are configured in 4d3b024, since explicitFeeds should already contain it. I checked and dotnet nuget list source does return it unless there is a <clear /> entry in the list of packageSources in nuget.config.

Actually, there's more subtly to this. GetNugetFeeds doesn't include it, because dotnet nuget list source only includes the feeds that are explicitly configured in a given configuration file if it is given an --configfile argument. GetNugetFeedsFromFolder does include the public feed (unless there's a <clear /> element), but we don't check whether these "inherited" feeds are reachable already.

So, we didn't previously check that the public feed is reachable anyway. @tamasvajk left a comment saying that we could check that they are reachable, but don't because of authentication requirements(?).

I think the real lesson for my changes here is that I should really use allFeeds as argument for RestoreProjects rather than just explicitFeeds.

OK that also still doesn't quite work with the current logic, because GetAllFeeds only invokes GetNugetFeedsFromFolder in folders that contain nuget.config files to begin with. In our case, we may not have a nuget.config file at all, so GetNugetFeedsFromFolder never gets called, even though it would give us the public feed as expected.

If the reachability of the public nuget feed wasn't checked before, then maybe there is no need to check it now either.
However, I think its availability is checked when it is added as a fallback feed (not as an inherited feed).

If the reachability of the public nuget feed wasn't checked before, then maybe there is no need to check it now either.

In any case, it's probably a separate discussion to the changes in this PR. I have also now removed that part of the code that manually added the public feed in be95d33.

However, I think its availability is checked when it is added as a fallback feed (not as an inherited feed).

That's true.

Maybe rename one of the CheckFeeds methods. The naming is a bit confusing when their type signatures are so similar.

Done in d2b88ae

Co-authored-by: Michael Nebel <michaelnebel@github.com>

…fig` files anywhere

To fix the problem, we should catch specific exceptions that are likely to be thrown by the operation being performed. In this case, FileInfo(config).Directory?.FullName can throw exceptions such as ArgumentException, PathTooLongException, NotSupportedException, UnauthorizedAccessException, and DirectoryNotFoundException. We should catch these specific exceptions and log them accordingly.

Great work! Thank you!

Nice improvement!

@mbg : We should also run DCA before merging.

DCA looks good, I think. So I will go ahead and merge this.

DCA looks good, I think. So I will go ahead and merge this.

It looks like the changes was not tested by the DCA run.
When testing extractor changes database caching needs to be disabled and it also needs to be specified that the extractor should be built based on the branch. 😃
In the wizard this looks like

mbg self-assigned this Feb 24, 2025

github-actions Bot added the C# label Feb 24, 2025

mbg added 7 commits March 14, 2025 13:39

C#: Add CODEQL_PROXY_URLS environment variable

6b2f348

C#: Add list of registries to DependabotProxy

63d5517

C#: Parse environment variables to obtain list of registry URLs

11efb55

C#: Allow specifying package feeds for dotnet restore as command li…

726123c

…ne arguments

C#: Propagate explicit feeds to RestoreProjects

0db6a26

C#: Fix test failures

6b15f77

C#: Only provide feeds on command line if Dependabot proxy is enabled

a8dde15

mbg force-pushed the mbg/csharp/inject-proxy-urls branch from 379f635 to a8dde15 Compare March 14, 2025 13:47

github-advanced-security AI found potential problems Mar 14, 2025

View reviewed changes

Comment thread csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependabotProxy.cs Fixed

Comment thread csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DotNet.cs Fixed

mbg added 3 commits March 14, 2025 14:02

C#: Fix .ToList() being called on null

9560593

C#: Narrow Exception to JsonException

b6c74fe

C#: Use StringBuilder for feed arguments in GetRestoreArgs

284f612

github-advanced-security AI found potential problems Mar 14, 2025

View reviewed changes

Comment thread csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DotNet.cs Fixed

mbg marked this pull request as ready for review March 14, 2025 14:37

mbg requested a review from a team as a code owner March 14, 2025 14:37

tamasvajk reviewed Mar 17, 2025

View reviewed changes

michaelnebel reviewed Mar 17, 2025

View reviewed changes

mbg and others added 5 commits March 17, 2025 14:24

Apply suggestions from code review

51874b8

Co-authored-by: Michael Nebel <michaelnebel@github.com>

C#: Change RegistryConfig to a record class

7a92a72

C#: Change RestoreSettings to have general extraArgs parameter

d564529

This allows the string of package feeds to be constructed once and used repeatedly in the parallel restore loop as well.

C#: Refactor CheckFeeds to have an overloaded variant that accepts …

92eab47

…a given set of feeds.

C#: Check that private package registry feeds are reachable

4448369

mbg requested review from michaelnebel and tamasvajk March 24, 2025 17:56

michaelnebel reviewed Mar 25, 2025

View reviewed changes

mbg and others added 3 commits March 25, 2025 10:02

Apply suggestions from code review

7cea2ad

Co-authored-by: Michael Nebel <michaelnebel@github.com>

C#: Rename overloaded CheckFeeds method and fix comment

d2b88ae

C#: Do not manually add public feed when private registries are used

4d3b024

mbg added 2 commits March 25, 2025 10:44

C#: Use allFeeds rather than explicitFeeds for RestoreProjects

73ca2eb

C#: Obtain all feeds from source directory if there are no `nuget.con…

be95d33

…fig` files anywhere

github-advanced-security AI found potential problems Mar 25, 2025

View reviewed changes

C#: Accept changes to .expected files

fe1c098

mbg requested a review from michaelnebel March 25, 2025 14:41

michaelnebel approved these changes Mar 26, 2025

View reviewed changes

mbg merged commit 2aee47b into main Mar 27, 2025

mbg deleted the mbg/csharp/inject-proxy-urls branch March 27, 2025 10:11

mbg mentioned this pull request Apr 9, 2025

Go: Support private registries via GOPROXY #19248

Merged

+                      /// <summary>
+                      /// Represents configurations for package registries.
+                      /// </summary>
+                      public struct RegistryConfig

+                                  // The value of the environment variable should be a JSON array of objects, such as:
+                                  // [ { "type": "nuget_feed", "url": "https://nuget.pkg.github.com/org/index.json" } ]
+                                  var array = JsonConvert.DeserializeObject<List<RegistryConfig>>(registryURLs);
+                                  if (array != null)

+                              // we have discovered from analysing `nuget.config` files.
+                              sources = configuredSources ?? new();
+                              sources.Add(PublicNugetOrgFeed);
+                              this.dependabotProxy?.RegistryURLs.ForEach(url => sources.Add(url));

+                              // we have discovered from analysing `nuget.config` files.
+                              sources = configuredSources ?? new();
+                              sources.Add(PublicNugetOrgFeed);
+                              this.dependabotProxy?.RegistryURLs.ForEach(url => sources.Add(url));

+                      /// </summary>
+                      /// <param name="explicitFeeds">Outputs the set of explicit feeds.</param>
+                      /// <returns>True if all feeds are reachable or false otherwise.</returns>
                       private bool CheckFeeds(out HashSet<string> explicitFeeds)

+                          // in addition to the ones that are configured in `nuget.config` files.
+                          this.dependabotProxy?.RegistryURLs.ForEach(url => feedsToCheck.Add(url));
+                          var allFeedsReachable = this.CheckFeeds(feedsToCheck);

+                                  .Where(folder => folder != null)
+                                  .SelectMany(folder => GetFeeds(() => dotnet.GetNugetFeedsFromFolder(folder!)))
+                                  .ToHashSet();
+                          }

@@ -830,5 +830,21 @@
                                     }
-                                    catch (Exception exc)
+                                    catch (ArgumentException exc)
                                     {
-                                        logger.LogWarning($"Failed to get directory of '{config}': {exc}");
+                                        logger.LogWarning($"Failed to get directory of '{config}' due to an argument exception: {exc}");
+                                    }
+                                    catch (PathTooLongException exc)
+                                    {
+                                        logger.LogWarning($"Failed to get directory of '{config}' due to a path too long exception: {exc}");
+                                    }
+                                    catch (NotSupportedException exc)
+                                    {
+                                        logger.LogWarning($"Failed to get directory of '{config}' due to a not supported exception: {exc}");
+                                    }
+                                    catch (UnauthorizedAccessException exc)
+                                    {
+                                        logger.LogWarning($"Failed to get directory of '{config}' due to an unauthorized access exception: {exc}");
+                                    }
+                                    catch (DirectoryNotFoundException exc)
+                                    {
+                                        logger.LogWarning($"Failed to get directory of '{config}' due to a directory not found exception: {exc}");
                                     }

+                          else
+                          {
+                              // If we haven't found any `nuget.config` files, then obtain a list of feeds from the root source directory.
+                              allFeeds = GetFeeds(() => dotnet.GetNugetFeedsFromFolder(this.fileProvider.SourceDir.FullName)).ToHashSet();

C#: Automatically use configured private registry feeds by mbg · Pull Request #18850 · github/codeql · GitHub

C#: Automatically use configured private registry feeds - #18850

C#: Automatically use configured private registry feeds#18850mbg merged 21 commits intomaingithub/codeql:mainfrom mbg/csharp/inject-proxy-urlsgithub/codeql:mbg/csharp/inject-proxy-urlsCopy head branch name to clipboard

Conversation

mbg commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check notice

Copilot Autofix

Autofix patch

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelnebel commented Mar 26, 2025

Uh oh!

mbg commented Mar 27, 2025

Uh oh!

michaelnebel commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

C#: Automatically use configured private registry feeds#18850
mbg merged 21 commits into
mainfrom
mbg/csharp/inject-proxy-urls

mbg commented Feb 24, 2025 •
edited
Loading

michaelnebel commented Mar 27, 2025 •
edited
Loading