SCAN4NET-227 Use system trusted certificate or JVM certificate store #2330

sebastien-marichal · 2025-02-19T16:58:16Z

martin-strecker-sonarsource

Looks good for me and should do the trick. I left some small comments but overall this looks promising.

martin-strecker-sonarsource · 2025-02-19T17:58:27Z

....PreProcessor.Test/AnalysisConfigProcessing/Processors/TruststorePropertiesProcessorTests.cs


        // Act
        processor.Update(config);

        // Assert
        config.LocalSettings.Should().ContainSingle(x => x.Id == SonarProperties.TruststorePassword && x.Value == input);
-        config.ScannerOptsSettings.Should().ContainSingle();
+        config.ScannerOptsSettings.Should().HaveCount(2);
+        AssertExpectedScannerOptsSettings("javax.net.ssl.trustStore", javaHomeCacerts.Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), config);
        AssertExpectedScannerOptsSettings("javax.net.ssl.trustStorePassword", expected, config);


You could additionally assert that fileWrapper.Exists(javaHomeCacerts) was called once.

martin-strecker-sonarsource · 2025-02-19T18:00:18Z

....PreProcessor.Test/AnalysisConfigProcessing/Processors/TruststorePropertiesProcessorTests.cs

+        var config = new AnalysisConfig { LocalSettings = [] };
+        using var envScope = new EnvironmentVariableScope();
+        envScope.SetVariable("JAVA_HOME", javaHome);
+        envScope.SetVariable("SONAR_SCANNER_OPTS", "-Djavax.net.ssl.trustStorePassword=itchange");


What happens if we also set another property here, e.g. -Xmx2048m?

If the value is still in the env variable during the end step, it will be kept.

I have added trustedSelfSignedCertificate_ExistingValueInScannerOpts in the ITs to test that behavior.

martin-strecker-sonarsource · 2025-02-19T18:04:35Z

...er.MSBuild.PreProcessor/AnalysisConfigProcessing/Processors/TruststorePropertiesProcessor.cs

-        MapProperty(config, "javax.net.ssl.trustStore", PropertyValueOrDefault(SonarProperties.TruststorePath, LocalSettings.TruststorePath), ConvertToJavaPath, EnsureSurroundedByQuotes);
-        var password = PropertyValueOrDefault(SonarProperties.TruststorePassword, LocalSettings.TruststorePassword);
-        MapProperty(config, "javax.net.ssl.trustStorePassword", password, EnsureSurroundedByQuotes);
+        if (new Uri(LocalSettings.ServerInfo.ServerUrl).Scheme != "https"


This constructor throws. Consider using TryCreate(url, UriKind.Absolute, out var uri) instead.

martin-strecker-sonarsource · 2025-02-19T18:06:23Z

...er.MSBuild.PreProcessor/AnalysisConfigProcessing/Processors/TruststorePropertiesProcessor.cs

-        MapProperty(config, "javax.net.ssl.trustStore", PropertyValueOrDefault(SonarProperties.TruststorePath, LocalSettings.TruststorePath), ConvertToJavaPath, EnsureSurroundedByQuotes);
-        var password = PropertyValueOrDefault(SonarProperties.TruststorePassword, LocalSettings.TruststorePassword);
-        MapProperty(config, "javax.net.ssl.trustStorePassword", password, EnsureSurroundedByQuotes);
+        if (new Uri(LocalSettings.ServerInfo.ServerUrl).Scheme != "https"


Suggested change

if (new Uri(LocalSettings.ServerInfo.ServerUrl).Scheme != "https"

if (new Uri(LocalSettings.ServerInfo.ServerUrl).Scheme != Uri.UriSchemeHttps

...er.MSBuild.PreProcessor/AnalysisConfigProcessing/Processors/TruststorePropertiesProcessor.cs

martin-strecker-sonarsource · 2025-02-19T18:10:35Z

...er.MSBuild.PreProcessor/AnalysisConfigProcessing/Processors/TruststorePropertiesProcessor.cs

+        var javaHome = Environment.GetEnvironmentVariable("JAVA_HOME");
+        if (javaHome is null)
+        {
+            // TODO: propagate the error?


I don't think so. A debug log should be enough.

It will not fail before the end step then.

If the JRE provisioning is enabled, JAVA_HOME does not have to be set. There might be cases where the provisioned JRE trusts the server anyhow. The JRE comes with a default set of CA roots. If we fail it here, it would break https servers with a proper purchased certificate or one from https://letsencrypt.org/
Or am I miss something here?

If this is from letsencrypt, it should be trusted without giving a store.
My understanding of purchased certificates is that you don't need to provide a truststore as they should be valid from some public CA.

I would be for failing the execution to follow the fail fast principle but I am not 100% sure that in this case there is no scenario where it would work.

The scenario is:

JRE provisioning is enabled and executed

JAVA_HOME is not set

The server is providing a certificate from a CA root trusted by the provisioned JRE (e.g. Let's Encrypt, Digi Cert, etc.)

The user doesn't specify anything (truststorePath is null)

We are on Linux

FindJavaTruststorePath is called

-> We would fail the begin step, even though everything would just work.

Can we safely assume that the cacerts of JAVA_HOME would be the same the one from the provisioned JRE?

If we take the same scenario with JAVA_HOME set, we will tell the provisioned JRE to use the local cacerts rather that the one download. I wonder if there is a possibility for the scenario to succeed with one and not the other where in theory it should succeed with both.

The root ca certs change on a regular basis (see e.g. here https://learn.microsoft.com/en-us/security/trusted-root/release-notes). It is likely that we will have different CA roots in both locations.

Note: I think this is a problem the scanner-cli may have as well.

Also: As a user, you can install custom root CAs in your JAVA_HOME, but you can not install root CAs in the provisioned JRE. Therefore, we should prefer JAVA_HOME over the provisioned JRE.

Sounds good to me, I will only do a log.

martin-strecker-sonarsource · 2025-02-19T18:11:14Z

...er.MSBuild.PreProcessor/AnalysisConfigProcessing/Processors/TruststorePropertiesProcessor.cs

+            return null;
+        }
+
+        var javaTruststorePath = Path.Combine(javaHome, "lib", "security", "cacerts");


Please add a link to some kind of documentation or ticket about the source of these path.

martin-strecker-sonarsource · 2025-02-19T18:17:09Z

src/SonarScanner.MSBuild.PreProcessor/WebClientDownloaderBuilder.cs

@@ -131,6 +131,7 @@ private static bool ServerCertificateCustomValidationCallback(X509Certificate2Co
    {
        if (errors is SslPolicyErrors.None)
        {
+            logger.LogWarning(Resources.WARN_CertificateTrustedBySystem);


The remote server certificate is trusted by the operating system. Provided truststore was not required.

If this is issued as a warning, it sounds like the command line argument can be removed from the begin step call. Is this always true? I know we set JAVAX_NET_SSL_TRUST_STORE_TYPE=Windows-ROOT but is this always working as intended. And what about Linux, here? I would turn this into a debug message. It does no harm if the user specified the file, right?
I would also remove this part: Provided truststore was not required.

I will update the message. I think it is important to keep in the message that a truststore has been provided to have context.
Something like: Provided truststore was not used.

martin-strecker-sonarsource

I think we should sh instead of bash

martin-strecker-sonarsource · 2025-02-25T10:00:14Z

src/SonarScanner.MSBuild.PreProcessor/JreResolution/LocalJreTruststoreResolver.cs

+
+        if (javaExePath is null || !fileWrapper.Exists(javaExePath))
+        {
+            var commandArgs = new ProcessRunnerArguments("/bin/bash", false) { CmdLineArgs = ["-c", "command -v java"], LogOutput = false };


Have you tried using sh instead? It is POSIX compatible.
Also we should not rely on bin/sh according to the standard.

Applications should note that the standard PATH to the shell cannot be assumed to be either /bin/sh or /usr/bin/sh, and should be determined by interrogation of the PATH returned by getconf PATH, ensuring that the returned pathname is an absolute pathname and not a shell built-in.

On some implementations this might return: /usr/xpg4/bin/sh

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sh.html

I think bin/sh is fine, but if you want to got the extra mile there is:
https://github.com/dotnet/runtime/blob/9a50493f9f1125fda5e2212b9d6718bc7cdbc5c0/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs#L686-L728

found here:
https://github.com/Tyrrrz/CliWrap/blob/7676d77ec3230c893511efc83b7a98efd6bb9d02/CliWrap/Command.Execution.cs#L43

martin-strecker-sonarsource · 2025-02-25T10:07:16Z

src/SonarScanner.MSBuild.PreProcessor/JreResolution/LocalJreTruststoreResolver.cs

+
+public class LocalJreTruststoreResolver(IFileWrapper fileWrapper, IDirectoryWrapper directoryWrapper, IProcessRunner processRunner, ILogger logger)
+{
+    public string TruststorePath(ProcessedArgs args)


Suggested change

public string TruststorePath(ProcessedArgs args)

public string UnixTruststorePath(ProcessedArgs args)

sonarqubecloud · 2025-02-25T13:57:49Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
95.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

martin-strecker-sonarsource

Approving, but I would recommend removing the stateful properties from the ProcessRunner in a follow up PR.

martin-strecker-sonarsource · 2025-02-25T14:11:37Z

....PreProcessor.Test/AnalysisConfigProcessing/Processors/TruststorePropertiesProcessorTests.cs

+        processRunner.Execute(Arg.Any<ProcessRunnerArguments>()).Returns(true);
+        processRunner.StandardOutput.Returns(new StringReader("/usr/bin/java"), new StringReader("/java/home/bin/java"));


Can you add be a bit more specific here?
Either

Use processRunner.Execute(Arg.Is<ProcessRunnerArguments>(x => x.Arguments is ["-c", "..."]).Returns(true); and processRunner.Execute(...).Returns(true);

Add comment about what the "/usr/bin/java" and "/java/home/bin/java" are supposed to represent.

martin-strecker-sonarsource · 2025-02-25T14:16:22Z

src/SonarScanner.MSBuild.Common/Interfaces/IProcessRunner.cs

+    public TextReader StandardOutput { get; }
+    public TextReader ErrorOutput { get; }
+


I'm not a big fan of making IProcessRunner statefull. Can we instead do

bool Execute(ProcessRunnerArguments runnerArgs); bool Execute(ProcessRunnerArguments runnerArgs, out TextReader standardOutput, out TextReader errorOutput);

This makes testing a bit harder, but it is possbile:
https://nsubstitute.github.io/help/setting-out-and-ref-arguments/

SCAN4NET-227 Use system trusted certificate or JVM certificate store

b702ab9

martin-strecker-sonarsource requested changes Feb 19, 2025

View reviewed changes

sebastien-marichal added 2 commits February 20, 2025 11:40

Addressing comment, add UTs & ITs

e987ee6

More ITs & code smells

ff0a12d

sebastien-marichal force-pushed the sma/cert-fallback branch from 1068943 to 70e2345 Compare February 21, 2025 15:20

Add JreTruststoreResolver

7bb1f16

sebastien-marichal force-pushed the sma/cert-fallback branch from 70e2345 to 7bb1f16 Compare February 21, 2025 16:18

Improve test coverage

b3a671c

sebastien-marichal requested a review from martin-strecker-sonarsource February 24, 2025 10:17

martin-strecker-sonarsource requested changes Feb 25, 2025

View reviewed changes

sebastien-marichal force-pushed the sma/cert-fallback branch from bd5dec8 to ab213f0 Compare February 25, 2025 13:11

sebastien-marichal marked this pull request as ready for review February 25, 2025 13:32

Use bourne shell

ae557b3

sebastien-marichal force-pushed the sma/cert-fallback branch from ab213f0 to ae557b3 Compare February 25, 2025 13:33

sebastien-marichal added 2 commits February 25, 2025 14:41

Trigger Build

4175399

Code smell

5676bd9

sebastien-marichal requested a review from martin-strecker-sonarsource February 25, 2025 13:59

martin-strecker-sonarsource approved these changes Feb 25, 2025

View reviewed changes

sebastien-marichal merged commit 2ffa6f0 into master Feb 25, 2025
17 checks passed

sebastien-marichal deleted the sma/cert-fallback branch February 25, 2025 14:29

	if (new Uri(LocalSettings.ServerInfo.ServerUrl).Scheme != "https"
	if (new Uri(LocalSettings.ServerInfo.ServerUrl).Scheme != Uri.UriSchemeHttps

	public string TruststorePath(ProcessedArgs args)
	public string UnixTruststorePath(ProcessedArgs args)

		processRunner.Execute(Arg.Any<ProcessRunnerArguments>()).Returns(true);
		processRunner.StandardOutput.Returns(new StringReader("/usr/bin/java"), new StringReader("/java/home/bin/java"));

		public TextReader StandardOutput { get; }
		public TextReader ErrorOutput { get; }

SCAN4NET-227 Use system trusted certificate or JVM certificate store #2330

SCAN4NET-227 Use system trusted certificate or JVM certificate store #2330

Uh oh!

Conversation

sebastien-marichal commented Feb 19, 2025 • edited by jira bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martin-strecker-sonarsource left a comment

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!

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!

martin-strecker-sonarsource Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

martin-strecker-sonarsource left a comment

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!

sonarqubecloud bot commented Feb 25, 2025

Quality Gate passed

Uh oh!

martin-strecker-sonarsource left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martin-strecker-sonarsource Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sebastien-marichal commented Feb 19, 2025 •

edited by jira bot

Loading

martin-strecker-sonarsource Feb 20, 2025 •

edited

Loading

martin-strecker-sonarsource Feb 25, 2025 •

edited

Loading