Skip to content
Closed
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
diagnostics
| file://:0:0:0:0 | Appending source destination directory target/generated-sources/annotations to sourcepath |
#select
| DepClass |
Original file line number Diff line number Diff line change
@@ -1,27 +1,85 @@
"""
Integration tests for truststore inheritance and merging.
Copy link
Member

Choose a reason for hiding this comment

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

Only the new test_autobuild_merge_trust_store test is about this. The existing test is unrelated.

Tests that CodeQL can connect to HTTPS servers with custom CA certificates:
Copy link
Member

Choose a reason for hiding this comment

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

In the case of the first (existing) test, that's mostly true for particular versions of the JRE and build-mode: none.

It does not really test this in the second (new) test where it puts the autobuilder into a situation where it can only trust one certificate, unless trust store merging is supported.

1. test_buildless: Buildless mode inherits truststore from MAVEN_OPTS
2. test_autobuild_merge_trust_store: Autobuild merges system truststore with
CODEQL_PROXY_CA_CERTIFICATE (fixes github/codeql-team#4482)
Copy link
Member

Choose a reason for hiding this comment

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

This should not be here.

"""
import subprocess
import os
import pytest
import runs_on
from contextlib import contextmanager


def test(codeql, java, cwd):
# This serves the "repo" directory on https://locahost:4443
@contextmanager
def _https_server(cwd):
"""Start an HTTPS server serving the repo/ directory on https://localhost:4443."""
Comment on lines +16 to +18
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

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

_https_server takes a cwd parameter but never uses it (the server is started with cwd="repo" relative to the current working directory). Either drop the parameter, or use it to build the repo path (e.g., based on cwd / "repo") so the helper is self-contained and less dependent on ambient CWD.

Copilot uses AI. Check for mistakes.
command = ["python3", "../server.py"]
if runs_on.github_actions and runs_on.posix:
# On GitHub Actions, we saw the server timing out while running in parallel with other tests
# we work around that by running it with higher permissions
command = ["sudo"] + command
repo_server_process = subprocess.Popen(command, cwd="repo")
certspath = cwd / "jdk8_shipped_cacerts_plus_cert_pem"
# If we override MAVEN_OPTS, we'll break cross-test maven isolation, so we need to append to it instead
maven_opts = os.environ["MAVEN_OPTS"] + f" -Djavax.net.ssl.trustStore={certspath}"

try:
yield
finally:
repo_server_process.kill()


@pytest.mark.ql_test(expected=".buildless.expected")
def test_buildless(codeql, java, cwd, check_diagnostics, check_buildless_fetches):
"""Test that buildless mode inherits truststore from MAVEN_OPTS."""
Copy link
Member

Choose a reason for hiding this comment

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

That's not what this test tests. This test is about CODEQL_JAVA_EXTRACTOR_TRUST_STORE_PATH. Both maven_opts and CODEQL_JAVA_EXTRACTOR_TRUST_STORE_PATH are based on certspath. Both are used independently.

Copy link
Contributor Author

@redsun82 redsun82 Feb 4, 2026

Choose a reason for hiding this comment

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

I see, I get now the background of CODEQL_JAVA_EXTRACTOR_TRUST_STORE_PATH. I would have probably opted for another approach rather than an in-production variable override just for testing: setting up a deep copy of JAVA_HOME as the new JAVA_HOME for an integration test where we inject our test certificates should be able to test the fix we did internally in that area I think? An overlay FS would be even less costly, but probably unfeasible on all platforms.

# Use buildless-specific expected file suffixes
check_diagnostics.expected_suffix = ".buildless.expected"
check_buildless_fetches.expected_suffix = ".buildless.expected"

with _https_server(cwd):
certspath = cwd / "jdk8_shipped_cacerts_plus_cert_pem"
# If we override MAVEN_OPTS, we'll break cross-test maven isolation, so we need to append to it instead
maven_opts = os.environ["MAVEN_OPTS"] + f" -Djavax.net.ssl.trustStore={certspath}"

codeql.database.create(
extractor_option="buildless=true",
_env={
"MAVEN_OPTS": maven_opts,
"CODEQL_JAVA_EXTRACTOR_TRUST_STORE_PATH": str(certspath),
},
)
finally:
repo_server_process.kill()


@pytest.mark.ql_test(expected=".autobuild.expected")
def test_autobuild_merge_trust_store(codeql, java, cwd, check_diagnostics):
Copy link
Member

Choose a reason for hiding this comment

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

The existing test is about exercising a particular fallback mechanism in the BMN extractor. Why is there now a test for an unrelated change in the autobuilder here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was to reuse resources without duplicating them. If we keep this test (which we may or may not do), we could just rename the directory to something more generic regarding trust stores, and either keep the two tests in the same file or even have two separate test files (sharing the resource directory).

With the new testing framework we don't need to have a 1:1 correspondence between directories and tests, and we should use that to avoid duplicating test resources.

"""
Test that autobuild merges system truststore with CODEQL_PROXY_CA_CERTIFICATE.
This tests the fix for a bug where autobuild was overriding JAVA_TOOL_OPTIONS
truststore with a new one containing only the proxy CA, causing PKIX failures
when connecting to internal HTTPS servers.
"""
# Use autobuild-specific expected file suffix
check_diagnostics.expected_suffix = ".autobuild.expected"

with _https_server(cwd):
certspath = cwd / "jdk8_shipped_cacerts_plus_cert_pem"

# Read the certificate to use as CODEQL_PROXY_CA_CERTIFICATE
with open(cwd / "cert.pem", "r") as f:
proxy_ca_cert = f.read()

# Set JAVA_TOOL_OPTIONS to use our custom truststore
# This is the key setting that was being overridden before the fix
java_tool_options = f"-Djavax.net.ssl.trustStore={certspath}"

# Run autobuild with the truststore configured
# Before the fix: autobuild would create a new truststore with ONLY the proxy CA,
# losing the custom CA from JAVA_TOOL_OPTIONS, causing PKIX failures
# After the fix: autobuild merges the system truststore + proxy CA
codeql.database.create(
build_mode="autobuild",
_env={
"JAVA_TOOL_OPTIONS": java_tool_options,
"CODEQL_PROXY_CA_CERTIFICATE": proxy_ca_cert,
Copy link
Member

Choose a reason for hiding this comment

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

This test abuses CODEQL_PROXY_CA_CERTIFICATE to put the Maven autobuilder into a situation where (without trust store merging) it only has access to this one certificate, and therefore fails when Maven tries to perform any networking that requires it to validate other certificates.

Copy link
Contributor Author

@redsun82 redsun82 Feb 4, 2026

Choose a reason for hiding this comment

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

My point here was that users seem to expect that trust stores passed via java tool options or maven options should still work in a default setup setting when CODEQL_PROXY_CA_CERTIFICATE, and, I thought, independently of all the proxy setup (including the other env variables for the proxy setup). Under this lens

and therefore fails when Maven tries to perform any networking that requires it to validate other certificates

is exactly the behaviour one might want to avoid.

However, I understand this assumption might be wrong in light of how our proxy works.

},
)