Close the cheap CodeQL findings: pinned actions, token scopes, TLS identity, private temp files

[vharseko]

The mechanical part of the CodeQL medium triage: findings whose fix is a few lines, grouped in one PR. Two of them change what an existing deployment sees — the SMTP and the setup host name checks below; each says so under Upgrade impact.

Workflows (actions/unpinned-tag ×11, actions/missing-workflow-permissions ×5)

• docker/metadata-action, docker/setup-qemu-action, docker/setup-buildx-action, docker/login-action, docker/build-push-action and softprops/action-gh-release are pinned to the commit their major tag resolves to today, with the precise release in a comment (v6.2.0, v4.4.0, v4.4.1, v4.6.0, v7.4.0, v3.0.3). actions/* stay on tags: first-party, and not what the query flags.
• .github/dependabot.yml (new) enables the github-actions ecosystem, weekly and grouped into one pull request: a SHA pin is frozen by definition, so without it the actions would never move again. Dependabot updates the SHA and the version comment together, and the grouped PR carries the ci/dependencies labels, matching Harden GitHub Actions workflows: token permissions, SHA pinning, latest actions OpenIG#170.
• Every job declares the GITHUB_TOKEN scopes it uses: build-maven contents: read; build-docker keeps the block Add Trivy vulnerability scanning for the Docker image #1141 gave it on master, contents: read + security-events: write (the image goes to a local registry; the Trivy scan is uploaded to code scanning); deploy-maven contents: write (docs are pushed to the wiki with github.token); release-maven contents: write (release:prepare pushes the tag, the GitHub release is created, the wiki is pushed); release-docker contents: read + packages: write (GHCR login with GITHUB_TOKEN). The Docker Hub and doc-repo pushes use their own secrets and need nothing from the token.

TLS identity (java/insecure-smtp-ssl ×2, java/unsafe-hostname-verification 1 of 3)

• AMSendMail.postMail(…, ssl=true) sets mail.smtp.ssl.checkserveridentity: the SMTP server's certificate has to be the host's, not merely one the JVM trusts. It is on by default and can be turned off with the new advanced server property org.openidentityplatform.openam.smtp.checkServerIdentity=false, registered in validserverconfig.properties (otherwise saving it is rejected, and the upgrade step would drop it from the server defaults) and described in the Configuration Reference. Only an explicit false turns the check off: the validator lets an empty value through, and an empty or unrecognised value keeps the check rather than parse as false. Pinned by shouldCheckTheServerIdentityOnAnSslConnection (default), shouldLeaveTheServerIdentityUncheckedWhenTheDeploymentTurnsItOff and shouldKeepTheServerIdentityCheckForAnEmptyValue.
  • Upgrade impact: jakarta.mail 2.0.2 leaves the check off by default, so until now smtps accepted any certificate the JVM trusts. A deployment whose mail server host is not named in its certificate — an IP address without an IP SAN, a short name, a relay alias — now fails with Can't verify identity of server, which stops password reset, lockout notification, the Email Service and the HOTP / OAuth2 email one-time passwords. A host given as an IP address matches an IP SAN on Java 11 as is, and on Java 16 and later only when the JVM runs with --add-exports java.base/sun.security.util=ALL-UNNAMED (the Docker image does): jakarta.mail reaches HostnameChecker by reflection, which Java 11's default --illegal-access=permit allows, and without it falls back to comparing DNS names and the CN. The fix is a host name the certificate carries, or the property above.
• AMSetupUtils.openConnection no longer installs a trust-all HostnameVerifier. getRemoteServerInfo posts the admin password to the remote server over that connection, so a certificate that is valid for some other host must not do. openConnection is package-private for shouldKeepTheDefaultHostNameVerifierForARemoteServerOverHttps.
  • Upgrade impact: adding a server to an existing site, where the existing OpenAM's certificate does not name the URL's host, now fails that setup step with the JDK's message instead of silently accepting it.
• Not changed, proposed for dismissal instead: ClusterStateService (a site health probe whose answer is a boolean; strict verification would break clusters whose certificates do not name the internal host names) and SoapSTSConsumer.handleSTSServerCertCNDNSMismatch (a test helper whose javadoc already says it must not be relied upon in production).

Private temporary files (java/local-temp-file-or-directory-information-disclosure ×4)

File.createTempFile creates the file with the process umask — rw-r--r-- on a typical server — in the shared temporary directory. java.nio.file.Files.createTempFile creates it rw-------.

• ApiDocsService: the generated .asciidoc and .html go through privateTempFile (owner-only, deleted on exit).
• NT: the file that carries the user name and password to the Samba helper goes through createCredentialsFile; its name no longer embeds the user name (which also failed for names shorter than three characters, the prefix minimum).
• Embedded Cassandra Server: the storage directory keeps its fixed default under java.io.tmpdir — a random name would not survive a restart of the development store — but is created, missing parents included, and if it already exists set, to rwx------ where the file system is POSIX; elsewhere it is created as before. A directory another account created first cannot be set, and the store refuses to start in it.

What the tests pin:

• NTCredentialsFileTest and ApiDocsServiceTest pin the helpers NT.createCredentialsFile and ApiDocsService.privateTempFile only, not their call sites in NT.process and ApiDocsService.getDocs / getAsciiDoc: the former runs the Samba helper, the latter needs the router and Asciidoctor. Both call sites are one line each, shown in the diff.
• For Cassandra the call site is pinned too: ServerTest checks the storage directory run() leaves behind, and points cassandra.storagedir at a directory under target that does not exist yet, so a directory an earlier run left in java.io.tmpdir cannot mask a revert. ServerStorageDirectoryTest pins the new directory, missing parents (which get their permissions from the creation attribute alone), an existing directory (kept data, permissions tightened) and a directory owned by another account (refused; skipped where the test account could write to /).
• Each was watched failing on the code it replaces: the helpers with [OWNER_READ, OWNER_WRITE, GROUP_READ, OTHERS_READ] (files) and 0755 / inherited (directory); ServerTest with path.mkdirs() back in run(), also with a stale embeddedCassandra left in java.io.tmpdir; the empty-value case with SystemProperties.getAsBoolean back in AMSendMail; the parent case without the creation attribute; the other-account case with the permission failure swallowed.

Verification

Rebased onto master at aba00acdad (#1141); the build-docker permissions are resolved in the first commit, and the merged build.yml carries one permissions block per job. On that base, under JDK 11 on x86_64: openam-cassandra-embedded 5 and AMSendMailTest 32. The Cassandra module's tests are skipped on JDK ≥ 15, on aarch64 and on Windows by the parent pom's profiles, so in CI they run on the ubuntu-latest × 11 leg only — including ServerTest, which starts the embedded Cassandra through the changed run(). On the earlier base 1ceea86, under JDK 26: openam-shared 1237, openam-core 2134, openam-core-rest 441, openam-auth-nt 7. 0 failures. NTCredentialsFileTest is a separate plain class because NTTest runs under PowerMock's class loader, which cannot instrument sun.nio.fs on JDK 26.

The CodeQL bot's SSRF flag on AMSetupUtils.openConnection (alert #511) is triaged and dismissed as won't fix — a re-fingerprint of #145.

[maximthomas]

praise: Each fix lands where its CodeQL alert points, and each comes with a test.

• AMSetupUtils.openConnection no longer installs a trust-all HostnameVerifier, so getRemoteServerInfo no longer posts the admin password to a server whose certificate names a different host.
• AMSendMail.postMail turns on the identity check that the pinned jakarta.mail 2.0.2 leaves off by default (SocketFetcher.configureSSLSocket reads mail.smtp.ssl.checkserveridentity with default false).

---

question (blocking): Is it intended that existing smtps deployments whose certificate does not name the configured SMTP host stop sending mail after the upgrade, with no way to opt out?

openam-core/src/main/java/com/iplanet/am/util/AMSendMail.java:232-236, :226

jakarta.mail 2.0.2 leaves mail.smtp.ssl.checkserveridentity off by default, so today smtps accepts any certificate the JVM trusts. postMail builds a fresh Properties with no defaults, so after this change no system property or SMS key can turn the check off again. Some deployments set the host to an IP literal without an IP SAN, a short name, or a relay alias the certificate does not list. Those now fail with "Can't verify identity of server", which breaks password reset, lockout notification, the REST email service and HOTP/OAuth2 email OTP (HOTP login over email fails). The description says these fixes carry no behaviour risk. If the break is deliberate, a release-note entry settles it and the toggle below becomes a suggestion. If it was not considered, an opt-out that defaults to on keeps the hardening and gives operators a way back:

        if (ssl) {
            moduleProps.put("mail.smtp.ssl.enable", "true");
            // The certificate has to be the SMTP host's, not merely one the JVM trusts.
            moduleProps.put("mail.smtp.ssl.checkserveridentity", String.valueOf(SystemProperties.getAsBoolean(
                    "org.openidentityplatform.openam.smtp.checkServerIdentity", true)));
        }

Or: keep the check unconditional and add the upgrade impact to the release notes. If the key becomes an advanced server property, it also needs registering in validserverconfig, or saving it is rejected as an unknown property.

---

issue (non-blocking): The three private-temp-file tests pin the extracted helpers but not the call sites that use them.

openam-authentication/openam-auth-nt/src/main/java/com/sun/identity/authentication/modules/nt/NT.java:241, openam-core-rest/src/main/java/org/forgerock/openam/core/rest/docs/api/ApiDocsService.java:159, :179, openam-cassandra/openam-cassandra-embedded/src/main/java/org/openidentityplatform/openam/cassandra/embedded/Server.java:80

NTCredentialsFileTest, ApiDocsServiceTest and ServerStorageDirectoryTest call NT.createCredentialsFile(), ApiDocsService.privateTempFile(...) and Server.privateDirectory(...) directly. No test runs NT.process or ApiDocsService.getDocs/getAsciiDoc, and ServerTest.start_test asserts nothing after run(). Put any of these call sites back to File.createTempFile(...) or path.mkdirs() and all three tests stay green, including for the file that carries the Samba password. The description says each test was watched failing on the previous code; that holds for the helper bodies only. ServerTest can pin the Cassandra call site:

	@Test
	public void start_test() throws IdRepoException, IOException {
		cassandra=new Server();
		cassandra.run();
		assumeTrue(FileSystems.getDefault().supportedFileAttributeViews().contains("posix"));
		assertEquals(PosixFilePermissions.fromString("rwx------"),
				Files.getPosixFilePermissions(Paths.get(System.getProperty("cassandra.storagedir"))));
	}

Pin: on a fresh runner, a path.mkdirs() revert leaves embeddedCassandra at rwxr-xr-x and this assertion fails. The imports it needs are java.io.IOException, java.nio.file.{FileSystems,Files,Paths}, java.nio.file.attribute.PosixFilePermissions, static org.junit.Assert.assertEquals and static org.junit.Assume.assumeTrue. For NT.process and ApiDocsService.getAsciiDoc, either drive the call site through a seam or say in the description that only the helper is pinned.

---

suggestion (non-blocking): Nothing pins the owner-only attribute that createDirectories applies to the directories it creates.

openam-cassandra/openam-cassandra-embedded/src/test/java/ServerStorageDirectoryTest.java:49, openam-cassandra/openam-cassandra-embedded/src/main/java/org/openidentityplatform/openam/cassandra/embedded/Server.java:65-67

privateDirectory always runs setPosixFilePermissions(path, ownerOnly) after createDirectories(path, asFileAttribute(ownerOnly)), and both tests read only the final permissions of the leaf. Remove the attribute and the directory is created at the umask default, then chmod'ed. Both tests stay green (measured by running both test bodies against the mutant in a scratch harness, not surefire). A missing parent gets its permissions only from the attribute and is never chmod'ed, so asserting on it pins the attribute:

    @Test
    public void missingParentsAreCreatedForTheOwnerOnly() throws IOException {
        Path parent = base.resolve("parent");

        Server.privateDirectory(parent.resolve("embeddedCassandra"));

        assertEquals(EnumSet.of(OWNER_READ, OWNER_WRITE, OWNER_EXECUTE), Files.getPosixFilePermissions(parent));
    }

Pin: drop asFileAttribute(ownerOnly) and parent is created rwxr-xr-x, so the case fails.

---

suggestion (non-blocking): Nothing pins that privateDirectory refuses a storage directory owned by another account.

openam-cassandra/openam-cassandra-embedded/src/test/java/ServerStorageDirectoryTest.java:59

The fix exists for a /tmp/embeddedCassandra that another account created first: setPosixFilePermissions on it fails with EPERM and run() stops. The existing-directory test uses a directory the test account owns, so wrapping the chmod in try { … } catch (IOException e) { } keeps both tests green (measured in a scratch harness). A root-owned directory, used from a non-root account, pins it:

    @Test(expected = FileSystemException.class)
    public void aDirectoryOwnedByAnotherAccountIsRefused() throws IOException {
        assumeFalse("root".equals(System.getProperty("user.name")));

        Server.privateDirectory(Paths.get("/"));
    }

Pin: with the chmod failure swallowed, no exception is thrown and the case fails. The imports it needs are java.nio.file.FileSystemException, java.nio.file.Paths and static org.junit.Assume.assumeFalse.

---

issue (non-blocking): ServerStorageDirectoryTest leaves an embedded-cassandra-test* directory in the shared temporary directory on every run.

openam-cassandra/openam-cassandra-embedded/src/test/java/ServerStorageDirectoryTest.java:42-46

@Before creates a new temporary directory for each test method and nothing deletes it; the second test also leaves its embeddedCassandra/data file inside. The PR's other new tests clean up after themselves (NTCredentialsFileTest:50, ApiDocsServiceTest:46).

    @Rule
    public TemporaryFolder tmp = new TemporaryFolder();

    @Before
    public void posixOnly() throws IOException {
        assumeTrue(FileSystems.getDefault().supportedFileAttributeViews().contains("posix"));
        base = tmp.newFolder("embedded-cassandra-test").toPath();
    }

---

suggestion (non-blocking): ServerStorageDirectoryTest does not need Cassandra, yet in CI it runs only on ubuntu × JDK 11.

openam-cassandra/openam-cassandra-embedded/src/test/java/ServerStorageDirectoryTest.java:38

The openam-cassandra parent pom skips surefire on JDK ≥ 15 (jdk-15-cassandra), on aarch64 (arm64, which includes macos-latest) and on Windows. Of the nine build-maven legs, only ubuntu-latest × 11 runs this pure filesystem test, and a local run on an arm64 Mac skips it too. The description mentions the JDK ≥ 15 skip.

Pin: add a dedicated surefire execution in openam-cassandra-embedded/pom.xml that includes only ServerStorageDirectoryTest and does not inherit the profiles' skip. Or move privateDirectory into a module that is tested on every leg.

[vharseko]

Addressed in 9b979dd; the branch is rebased onto current master.

SMTP server identity (blocking). Not considered, and the break is real — so the opt-out went in, default on, as you proposed: org.openidentityplatform.openam.smtp.checkServerIdentity (Constants.AM_SMTP_CHECK_SERVER_IDENTITY), read with SystemProperties.getAsBoolean(…, true). It is registered in validserverconfig.properties as true,false — besides the save-time check, UpgradeServerDefaultsStep drops server defaults that file does not list — and described in the Configuration Reference, next to the Mail Server Secure Connection setting. shouldLeaveTheServerIdentityUncheckedWhenTheDeploymentTurnsItOff pins the false path (it fails with the value hard-coded back to "true"); shouldCheckTheServerIdentityOnAnSslConnection still pins the default.

One more case for the same list: jakarta.mail matches through sun.security.util.HostnameChecker, reached by reflection. On JDK 17+ without --add-exports java.base/sun.security.util=ALL-UNNAMED the call fails with IllegalAccessException (checked locally) and it falls back to comparing DNS SANs and the CN only — so a host given as an IP address fails even with a correct IP SAN. The Docker image sets the export; a WAR on a stock Tomcat does not. The docs say so, and the description now has an Upgrade impact note for this and for the AMSetupUtils change, with "no behaviour risk" gone.

Call sites of the temp-file helpers. ServerTest.start_test now asserts rwx------ on the storage directory run() leaves, as you proposed; with path.mkdirs() back in run() and a fresh storage directory it fails with the rwxr-xr-x set. For NT.process and ApiDocsService.getDocs / getAsciiDoc I went with the description: it now says the tests pin the helpers only, and which ones.

Creation attribute of missing parents. Added missingParentsAreCreatedForTheOwnerOnly; without asFileAttribute(ownerOnly) it fails.

Directory owned by another account. Added aDirectoryOwnedByAnotherAccountIsRefused, with one change to the guard: besides user.name it also skips when Files.isWritable(Paths.get("/")). Under uid 0 with another name, the test would otherwise really set / to rwx------. With the chmod failure swallowed it fails.

Leftover temporary directories. ServerStorageDirectoryTest uses a TemporaryFolder rule now.

Only ubuntu × JDK 11 runs it. Recording this, not doing it here. What the test checks is POSIX mkdir/chmod behaviour, which does not change with the JDK; on Windows it is skipped by its own assume anyway, so what a separate execution would add is the macOS leg. That comes at the cost of a surefire execution working against the module's deliberate skip, and I have not checked that Server loads without Cassandra on JDK ≥ 15. The description spells out which leg runs it.

All three new cases, and the ServerTest assertion, were run against their mutant and seen failing; openam-cassandra-embedded 5/5 and AMSendMailTest 31/31 green under JDK 11.

[maximthomas]

praise: Round 1's blocker is fixed the right way. The new property is registered, documented and pinned.

• org.openidentityplatform.openam.smtp.checkServerIdentity defaults to on and is registered in validserverconfig.properties:96, so it survives a console save and UpgradeServerDefaultsStep. shouldLeaveTheServerIdentityUncheckedWhenTheDeploymentTurnsItOff pins the false path.
• ServerTest:59 now asserts on the directory Server.run() leaves, not only on privateDirectory.
• aDirectoryOwnedByAnotherAccountIsRefused skips when Files.isWritable("/"), so a build run as root never chmods /.

---

issue (blocking): Merged onto current master, the build-docker job has two permissions keys.

.github/workflows/build.yml:80-81

After this PR's last push, #1141 (aba00acdad, Trivy) added its own permissions: block to build-docker, after runs-on: contents: read and security-events: write. This PR adds permissions: contents: read before runs-on. The hunks do not overlap, so git merge-tree --write-tree origin/master 9b979dd is clean and GitHub offers the merge button. The merged file carries jobs.build-docker.permissions twice, at lines 80 and 83. GitHub rejects a workflow with a duplicate key ("'permissions' is already defined"), so after the merge the Build workflow would not run on master at all, and neither would deploy.yml, which is chained off it. The green checks on this PR ran on a test merge with the old base 1ceea86, so they cannot show this.

To fix it: rebase onto master and keep master's block, which already scopes the job:

  build-docker:
    needs: build-maven
    runs-on: 'ubuntu-latest'
    permissions:
      contents: read
      # upload the Trivy scan of the built image to code scanning
      security-events: write

The description's "build-maven / build-docker contents: read" line then needs security-events: write for build-docker.

---

issue (non-blocking): An empty value of the new property turns the SMTP identity check off, although the docs say "Default: true".

openam-core/src/main/java/com/iplanet/am/util/AMSendMail.java:236-237

SystemProperties.getAsBoolean(key, true) returns the default only for null and otherwise returns Boolean.parseBoolean(value). ServerPropertyValidator.validate checks a value against true,false only when value.length() > 0. An empty value saved on the Advanced tab is stored and read back as "", and parseBoolean("") is false. The same goes for a -D value such as yes, which never reaches the validator. In either case smtps quietly accepts any trusted certificate again, and nothing shows it, because mail keeps flowing. If only an explicit false turns the check off, the check fails closed:

            moduleProps.put("mail.smtp.ssl.checkserveridentity", String.valueOf(
                    !"false".equalsIgnoreCase(SystemProperties.get(Constants.AM_SMTP_CHECK_SERVER_IDENTITY, "true").trim())));

With this change, shouldLeaveTheServerIdentityUncheckedWhenTheDeploymentTurnsItOff has to stub SystemProperties.get(key, "true") instead of getAsBoolean.

---

issue (non-blocking): The Configuration Reference says an IP-address mail host needs --add-exports to match an IP SAN. On JDK 11 it matches without the flag.

openam-documentation/openam-doc-source/src/main/asciidoc/reference/chap-config-ref.adoc:6130

jakarta.mail 2.0.2 reaches sun.security.util.HostnameChecker by reflection. On JDK 9-15 the default --illegal-access=permit allows that call, so the IP match works with no flag. We ran jakarta.mail's own SocketFetcher.matchCert against a certificate with SAN=ip:127.0.0.1:

• JDK 11.0.25, no flag: 127.0.0.1 matches, 127.0.0.2 does not.
• JDK 17.0.11, no flag: neither matches.

The sentence is right only on JDK 16 and later.

A host given as an IP address matches an IP address subject alternative name on Java 11 as is, and on Java 16 and later only when the JVM is started with `--add-exports java.base/sun.security.util=ALL-UNNAMED`, as the OpenAM Docker image is; otherwise use a host name.

---

suggestion (non-blocking): ServerTest catches a revert of the run() call site only when java.io.tmpdir is clean.

openam-cassandra/openam-cassandra-embedded/src/test/java/ServerTest.java:59-60, Server.java:79-80

init() sets no cassandra.storagedir, so run() uses the fixed ${java.io.tmpdir}/embeddedCassandra, and nothing removes that directory afterwards. After one run it is left behind as rwx------. If path.mkdirs() is then put back at Server.java:80, the test still passes on that machine, because mkdirs() does not touch an existing directory. On GitHub's fresh VMs the assertion does fail. On a developer machine or a self-hosted runner it does not.

	@BeforeClass
	public static void init() throws IdRepoException, IOException {
		// A storage directory that does not exist yet, so the assertion sees what run() creates.
		System.setProperty("cassandra.storagedir",
				Files.createTempDirectory("server-test").resolve("embeddedCassandra").toString());
		...

Pin: with this init(), the path.mkdirs() mutant fails with rwxr-xr-x on every machine, not only on a fresh one.

[vharseko]

Addressed in 587492f; the branch is rebased onto current master (aba00acdad).

Duplicate permissions on build-docker (blocking). Confirmed: git merge-tree of the old head with master gave jobs.build-docker.permissions at lines 80 and 83. Resolved inside the rebase rather than in this round's commit: the first commit (now cb50e65) no longer adds a block to build-docker, which keeps the one #1141 gave it (contents: read, security-events: write). The rebased build.yml has one permissions block per job, and the description now lists security-events: write for build-docker.

Empty value turns the check off. Confirmed: ServerPropertyValidator.validate checks the possible values only when value.length() > 0, and getAsBoolean parses "" as false. Taken as proposed: only an explicit false turns the check off. One change to the tests: both stub the single-argument SystemProperties.get(key) rather than get(key, "true"). With the two-argument stub, the getAsBoolean code reads the unstubbed get(key), gets null and the default, so a test for the empty value would pass on it. The new shouldKeepTheServerIdentityCheckForAnEmptyValue fails with getAsBoolean put back (expected "true" but was "false"). The Configuration Reference says that only false turns it off.

IP SAN on Java 11. Confirmed with the same SocketFetcher.matchCert check (jakarta.mail 2.0.2, SAN=ip:127.0.0.1):

JVM	127.0.0.1	127.0.0.2
11.0.32, no flag	true	false
17.0.20, no flag	false	false
21.0.12, no flag	false	false
17.0.20, --add-exports	true	false

The Configuration Reference takes your sentence; the Upgrade impact note in the description is corrected the same way.

ServerTest and a stale storage directory. Taken, with one change: the fresh cassandra.storagedir is created under target, not under java.io.tmpdir. The test never stops the daemon, so its data cannot be removed in @AfterClass; under target it goes with mvn clean instead of piling up in the shared temporary directory. With path.mkdirs() back in run() and a rwx------ embeddedCassandra left in java.io.tmpdir from an earlier run, start_test now fails with the rwxr-xr-x set.

AMSendMailTest 32/32 and openam-cassandra-embedded 5/5 green under JDK 11 on the new base; both mutants seen failing.

[maximthomas]

praise: The rebase resolves the workflow conflict the right way round, and the SMTP opt-out now fails closed.

• .github/workflows/build.yml:81-84: build-docker keeps master's single permissions block (contents: read + security-events: write); merged onto current master (394a779) the workflow still has one block per job.
• openam-core/src/main/java/com/iplanet/am/util/AMSendMail.java:237-238 keeps the identity check for an empty value; only false turns it off.
• ServerTest.init points cassandra.storagedir at a directory under target that does not exist yet, so a stale embeddedCassandra in java.io.tmpdir can no longer mask a revert of run().