-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Java: Add integration test for autobuild truststore merging #21262
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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. | ||
|
|
||
| Tests that CodeQL can connect to HTTPS servers with custom CA certificates: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||
| 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.""" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not what this test tests. This test is about
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, I get now the background of |
||
| # 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): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test abuses
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
is exactly the behaviour one might want to avoid. However, I understand this assumption might be wrong in light of how our proxy works. |
||
| }, | ||
| ) | ||
There was a problem hiding this comment.
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_storetest is about this. The existing test is unrelated.