C#: Set proxy environment variables, if Dependabot proxy is detected by mbg · Pull Request #18029 · github/codeql · GitHub

This PR is part of work to enable private package registries to be used in Default Setup.

The existing Default Setup workflow will initialise the Dependabot package proxy, if a private package registry configuration is set. The host, port, and certificate used by the proxy are then passed to CodeQL in the analyze step.

The changes in this PR modify the C# extractor to recognise when the corresponding environment variables are set. If so, we use the data from those environment variables to:

Construct the proxy address from the host and port and pass it to dotnet via the HTTP_PROXY and HTTPS_PROXY environment variables.
Store the certificate on disk, and pass the path to dotnet via SSL_CERT_FILE.
We also configure a suitable proxy for the feed reachability checks.

In testing so far, this works fine on Linux with fairly arbitrary versions of dotnet. It does not seem to work on macOS and likely also does not work on Windows.

LGTM, added some comments/questions.

What's not working on macOS? Is it the dotnet calls, or the httpclient that's not working?

Do we need this, because we need to support a certificate that's not signed by a well-known trusted root cert authority?

Correct. The Dependabot Proxy uses a self-signed certificate that is generated when we initialise it in the Default Setup workflow. That's then passed to us in an environment variable, and we should trust it.

This could probably implement System.Net.IWebProxy.

Maybe, but I am not sure I see any immediate benefit to it. Although I guess it would simplify the implementation for the feed reachability checking? Happy to do that in a follow-up PR if it makes sense, but it would be nice to get this functionality shipped for now.

What's not working on macOS? Is it the dotnet calls

dotnet on macOS (and Windows) does not seem to respect HTTP(S)_PROXY and SSL_CERT_FILE (likely because it only uses OpenSSL under the hood on Linux?)

dotnet on macOS (and Windows) does not seem to respect HTTP(S)_PROXY and SSL_CERT_FILE (likely because it only uses OpenSSL under the hood on Linux?)

If we don't expect this change to work on macos and windows, we should be explicit about it. Can we only create the dependabot proxy if we're on linux?

If we don't expect this change to work on macos and windows, we should be explicit about it. Can we only create the dependabot proxy if we're on linux?

@tamasvajk this is done as part of the Default Setup workflow with a dedicated action. Until recently, this only supported Linux. It does now support Windows and macOS as well, but the docs for the private registries feature will note the current limitations. In other words, it's not something we need to worry about in this C#-specific part.

If we don't expect this change to work on macos and windows, we should be explicit about it. Can we only create the dependabot proxy if we're on linux?

@tamasvajk this is done as part of the Default Setup workflow with a dedicated action. Until recently, this only supported Linux. It does now support Windows and macOS as well, but the docs for the private registries feature will note the current limitations. In other words, it's not something we need to worry about in this C#-specific part.

Does this mean that on Windows and MacOS, the CODEQL_PROXY_* environment variables are not going to be set?

…ment `IDisposable`

Does this mean that on Windows and MacOS, the CODEQL_PROXY_* environment variables are not going to be set?

@tamasvajk They do get set on Windows and macOS. The environment variables we set here just wouldn't affect dotnet on those platforms (though it would be desirable if they did).

However, the proxy we install for the feed reachability check would be affected. That would result in a weird situation on those platforms where we check whether the feeds that require the proxy (i.e. require authentication) are reachable, determine that they are, but then dotnet cannot connect them.

So I have now added a check to skip initialising the DependabotProxy instance on those platforms.

I have also addressed all of your other review comments.

LGTM

@tamasvajk I tested this a bit more after the changes in response to your review, and ran into some flakiness with the new X509Certificate2 call failing since the certificate hadn't been flushed to disk correctly yet in some cases. I have added an explicit Close call on the writer, but also am just loading the certificate from the existing string value obtained from the environment variable, so we can skip reading the certificate from disk entirely.

I'm somewhat surprised that this is needed. The writer is disposed at the end of this method, and we wouldn't access the file elsewhere before that. If you want to control the disposal a bit more fine grained, you could still use a using block, and you could get rid of the Close call.

using(var writer = certFile.CreateText()) 
{
  writer.Write(cert);
}

mbg self-assigned this Nov 19, 2024

github-actions Bot added the C# label Nov 19, 2024

mbg force-pushed the mbg/csharp/set-proxy-cert-file branch from c5c1260 to 9731006 Compare November 19, 2024 13:45

mbg force-pushed the mbg/csharp/set-proxy-cert-file branch from 9731006 to a107872 Compare November 29, 2024 13:19

github-advanced-security AI found potential problems Dec 2, 2024

View reviewed changes

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

mbg marked this pull request as ready for review December 2, 2024 14:25

mbg requested a review from a team as a code owner December 2, 2024 14:25

mbg commented Dec 2, 2024

View reviewed changes

Comment thread csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/NugetPackageRestorer.cs Outdated

tamasvajk reviewed Dec 2, 2024

View reviewed changes

mbg force-pushed the mbg/csharp/set-proxy-cert-file branch from fca0b6a to dcfcb88 Compare December 3, 2024 18:48

tamasvajk reviewed Dec 4, 2024

View reviewed changes

mbg added 11 commits December 5, 2024 12:14

C#: Add CODEQL_PROXY_* environment variable names

5376012

C#: Add DependabotProxy class

232caa7

C#: Initialise DependabotProxy in DotNetCliInvoker

8ca7560

C#: Set environment variables for proxy for calls to dotnet

6cd5711

C#: Add more logging to DependabotProxy

de415d6

C#: Use Add for environment variables

87bd21e

C# Expose CertificatePath from DependabotProxy

e999ec1

C#: Propagate DependabotProxy instance down from DependencyManager

984091d

C#: Set up proxy for IsFeedReachable, if configured

ca251fb

C#: Load Dependabot Proxy certificate in DependabotProxy, and imple…

ee7f0b0

…ment `IDisposable`

C#: Apply suggestions from code review for DependabotProxy

2e80e09

mbg force-pushed the mbg/csharp/set-proxy-cert-file branch from dcfcb88 to 2e80e09 Compare December 5, 2024 12:17

C#: Don't initialise DependabotProxy on Windows or macOS

7369d04

C#: Fix possible null dereference

952488c

tamasvajk previously approved these changes Dec 5, 2024

View reviewed changes

mbg added 2 commits December 6, 2024 13:13

C#: Explicitly close writer in DependabotProxy

653d68e

C#: Create certificate from string, rather than file

c8ccfe4

mbg dismissed tamasvajk’s stale review via c8ccfe4 December 6, 2024 13:13

mbg requested a review from tamasvajk December 6, 2024 13:15

tamasvajk reviewed Dec 6, 2024

View reviewed changes

tamasvajk approved these changes Dec 6, 2024

View reviewed changes

mbg merged commit e16adda into main Dec 6, 2024

mbg deleted the mbg/csharp/set-proxy-cert-file branch December 6, 2024 14:03

mbg restored the mbg/csharp/set-proxy-cert-file branch December 6, 2024 16:36

mbg mentioned this pull request Dec 6, 2024

Port "C#: Set proxy environment variables, if Dependabot proxy is detected" to 2.20.0 release branch #18238

Merged

mbg mentioned this pull request Mar 14, 2025

C#: Automatically use configured private registry feeds #18850

Merged

mbg mentioned this pull request Apr 9, 2025

Go: Support private registries via GOPROXY #19248

Merged

+                              httpClientHandler.Proxy = new WebProxy(this.dependabotProxy.Address);
+                              if (!String.IsNullOrEmpty(this.dependabotProxy.CertificatePath))
+                              {

+              namespace Semmle.Extraction.CSharp.DependencyFetching
+              {
+                  public class DependabotProxy

+                              using var writer = certFile.CreateText();
+                              writer.Write(cert);
+                              writer.Close();

C#: Set proxy environment variables, if Dependabot proxy is detected by mbg · Pull Request #18029 · github/codeql · GitHub

C#: Set proxy environment variables, if Dependabot proxy is detected - #18029

C#: Set proxy environment variables, if Dependabot proxy is detected#18029mbg merged 15 commits intomaingithub/codeql:mainfrom mbg/csharp/set-proxy-cert-filegithub/codeql:mbg/csharp/set-proxy-cert-fileCopy head branch name to clipboard

Conversation

mbg commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tamasvajk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tamasvajk Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

mbg Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

tamasvajk Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

mbg Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

mbg commented Dec 3, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tamasvajk commented Dec 4, 2024

Uh oh!

mbg commented Dec 4, 2024

Uh oh!

tamasvajk commented Dec 4, 2024

Uh oh!

mbg commented Dec 5, 2024

Uh oh!

tamasvajk left a comment

Choose a reason for hiding this comment

Uh oh!

mbg commented Dec 6, 2024

Uh oh!

tamasvajk Dec 6, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

C#: Set proxy environment variables, if Dependabot proxy is detected#18029
mbg merged 15 commits into
mainfrom
mbg/csharp/set-proxy-cert-file

mbg commented Nov 19, 2024 •
edited
Loading