Conversation
vharseko
added
java
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: The reload sits exactly where the stale certificate came from: the managers handed out, not the handler's SSL context.
FileStampcompares mtime, size and the file key with symbolic links followed, so a renewal agent's rename is caught (FileStamp.java:67-92).- A failed reload records its stamps and keeps the last good manager, while
getKeyManagers()/getTrustManagers()still throw on an unloadable file, so configuration validation is unchanged (FileBasedKeyManagerProvider.java:294-298). - A plain trust manager stays plain, so JSSE keeps its own checks around
ExpirationCheckTrustManager(FileBasedTrustManagerProvider.java:223-226).
issue (non-blocking): A reloaded key store that loads but has no usable key or alias replaces the last good key manager.
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileBasedKeyManagerProvider.java:284-313
The last good manager is kept only when loadKeyManagers() throws. A store with no private-key entry is only logged (:313), and the ssl-cert-nickname alias is checked only when a handler builds its SSL context (LDAPConnectionHandler.java:1335). Suppose a renewal is written under another alias than the configured one (config.ldif default server-cert). It is loaded at :291, SelectableCertificateKeyManager.findServerAlias (:184) finds no alias, and every LDAPS/HTTPS handshake fails until the file changes again. Only NOTE_FILE_KEYMANAGER_RELOADED is logged. Before this change the old certificate kept serving. The guard keeps the old manager for one unusable file (wrong PIN) but drops it for another (wrong alias).
final KeyManager[] keyManagers = loadKeyManagers();
if (keyManagers.length != 1 || !(keyManagers[0] instanceof X509ExtendedKeyManager))
{
throw new DirectoryException(DirectoryServer.getCoreConfigManager().getServerErrorResultCode(),
ERR_FILE_KEYMANAGER_CANNOT_CREATE_FACTORY.get(keyStoreFile, Arrays.toString(keyManagers)));
}
if (!containsAtLeastOneKey())
{
throw new DirectoryException(DirectoryServer.getCoreConfigManager().getServerErrorResultCode(),
ERR_NO_KEY_ENTRY_IN_KEYSTORE.get(keyStoreFile));
}Or: also keep the last good manager when the new store has none of the key aliases the last good store had.
issue (non-blocking): The only test of the key store reload does not exercise getServerAliases, the method LDAPS calls when ssl-cert-nickname is set.
opendj-server-legacy/src/test/java/org/opends/server/extensions/FileBasedKeyManagerProviderTestCase.java:358-363
serverCertificateOf chooses the alias with chooseEngineServerAlias. With a nickname set, LDAPConnectionHandler.java:1346 wraps the managers in SelectableCertificateKeyManager, which asks ReloadingKeyManager.getServerAliases instead. The mutant return loaded.keyManager.getServerAliases(keyType, issuers); at FileBasedKeyManagerProvider.java:124 passes testKeyStoreLoadedAgainWhenChanged, yet with it LDAPS keeps serving the old certificate. This test is the only one on #1095.
/** The road LDAPS takes with ssl-cert-nickname set: SelectableCertificateKeyManager asks getServerAliases. */
private static String serverCertificateByAliasesOf(X509ExtendedKeyManager keyManager)
{
final String[] aliases = keyManager.getServerAliases("RSA", null);
assertThat(aliases).isNotEmpty();
return keyManager.getCertificateChain(aliases[0])[0].getSubjectX500Principal().getName();
}Pin: use it for the first read after replace(keyStore, client.keystore): final String clientCertificate = serverCertificateByAliasesOf(keyManager);. The :124 mutant then fails isNotEqualTo(serverCertificate).
issue (non-blocking): The trust reload rejects a new manager whose class differs from the last one, and isFips() can change that class while the server runs.
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileBasedTrustManagerProvider.java:265, :222, :305-311
loadTrustManagers wraps in ExpirationCheckTrustManager only when !isFips(), and isFips() matches any provider whose name contains "fips". Creating a BCFKS key or trust provider at runtime calls FipsStaticUtils.registerBcProvider(true), which inserts BCFIPS (FipsStaticUtils.java:53). From then on every renewal of a JKS trust store is rejected with ERR_FILE_TRUSTMANAGER_CANNOT_RELOAD, until the next getTrustManagers() stores the raw manager at :222. After that, the plain wrappers handed out earlier run without ExpirationCheckTrustManager. The cause argument of that message is Arrays.toString(trustManagers), and the key side does the same at :289.
if (trustManagers.length != 1 || !(trustManagers[0] instanceof X509TrustManager)
|| current.trustManager instanceof X509ExtendedTrustManager
&& !(trustManagers[0] instanceof X509ExtendedTrustManager))issue (non-blocking): A failed reload leaves the new PIN in keyStorePIN / trustStorePIN.
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileBasedKeyManagerProvider.java:284, :294-298; FileBasedTrustManagerProvider.java:263
The PIN field is assigned before the load, and the catch block does not restore it. Suppose the PIN file is renewed before the store, or alone. One handshake stores the new PIN and fails on the old store. Until the store is renewed, containsAtLeastOneKey() / containsKeyWithAlias() return false and getKeyManagers() throws. Enabling or reconfiguring an SSL handler on this provider is then rejected (LDAPConnectionHandler.java:1318, :1335, HTTPConnectionHandler.java:893, :911). Without such a handshake in the window, the same change is accepted.
final char[] previousPIN = keyStorePIN;
try
{
// ... unchanged, including keyStorePIN = pin;
}
catch (DirectoryException e)
{
keyStorePIN = previousPIN;
logger.traceException(e);
loaded = new LoadedKeyManager(stamps, current.keyManager);
logger.error(ERR_FILE_KEYMANAGER_CANNOT_RELOAD, keyStoreFile, currentConfig.dn(), e.getMessageObject());
}Or: the next suggestion, which removes the cached PIN from the road altogether.
suggestion (non-blocking): getKeyManagers() / getTrustManagers() could read the PIN the configuration names now, as the reload does.
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileBasedTrustManagerProvider.java:214-217; FileBasedKeyManagerProvider.java:237-240
Only currentKeyManager / currentTrustManager re-read the PIN (:279 / :258). After a store and its PIN are renewed, a fresh getTrustManagers() still loads with the cached PIN and throws ERR_FILE_TRUSTMANAGER_CANNOT_LOAD. That call comes from pass-through authentication (LDAPPassThroughAuthenticationPolicyFactory.java:1159) or from a reconfigured handler. It keeps throwing until a handshake on a manager handed out earlier refreshes the field, and never if no such manager shares the provider. This already happened before the change, so it is a gap in the renewal story, not a regression.
/** Reads the PIN the configuration names now, as currentTrustManager does. */
private char[] currentPIN() throws DirectoryException
{
final ConfigChangeResult ccr = new ConfigChangeResult();
final char[] pin = getTrustStorePIN(currentConfig, ccr);
if (ccr.getResultCode() != ResultCode.SUCCESS)
{
throw new DirectoryException(ccr.getResultCode(), ccr.getMessages().get(0));
}
return pin;
}Then trustStorePIN = currentPIN(); before loadTrustManagers() in getTrustManagers() and in currentTrustManager(), and the same for the key store.
suggestion (non-blocking): Take the stamps again inside the lock.
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileBasedTrustManagerProvider.java:242-251; FileBasedKeyManagerProvider.java:263-272
The double-check under the lock compares loaded.stamps with the caller's pre-lock stamps and records those. Suppose a thread stamped an intermediate state of a non-atomic write. It reloads what another thread has just loaded, and the next caller reloads once more. The NOTE/ERR message is then logged two or three times and the file re-read, although the description says the error is logged once. The manager chosen stays correct.
List<FileStamp> stamps = stampFiles();
LoadedTrustManager current = loaded;
if (current.stamps.equals(stamps))
{
return current.trustManager;
}
synchronized (this)
{
stamps = stampFiles();
current = loaded;
// ... unchangedsuggestion (non-blocking): No test covers the reset in applyConfigurationChange that makes the managers already handed out load again.
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileBasedKeyManagerProvider.java:397-400; FileBasedTrustManagerProvider.java:365-368
Neither test class calls applyConfigurationChange. With the block deleted, every test stays green. The block only matters when the PIN or type changes and the files do not, since a new file path changes the stamp anyway.
@Test
public void testKeyStoreLoadedAgainWhenPinChangesInConfiguration() throws Exception
{
final File configDir = new File(DirectoryServer.getInstanceRoot(), "config");
final File keyStore = new File(configDir, "reconfigured-test.keystore");
replace(keyStore, Files.readAllBytes(new File(configDir, "server.keystore").toPath()));
FileBasedKeyManagerProvider provider = initializeKeyManagerProvider(providerEntry("password"));
try
{
final X509ExtendedKeyManager keyManager = (X509ExtendedKeyManager) provider.getKeyManagers()[0];
final String serverCertificate = serverCertificateOf(keyManager);
// a store under a PIN the configuration does not have yet: the failed reload records its stamps
replace(keyStore, withPassword(new File(configDir, "client.keystore"), "password", "changed"));
assertThat(serverCertificateOf(keyManager)).isEqualTo(serverCertificate);
provider.applyConfigurationChange(InitializationUtils.getConfiguration(
FileBasedKeyManagerProviderCfgDefn.getInstance(), providerEntry("changed")));
assertThat(serverCertificateOf(keyManager)).isNotEqualTo(serverCertificate);
}
finally
{
provider.finalizeKeyManagerProvider();
Files.deleteIfExists(keyStore.toPath());
}
}
private static Entry providerEntry(String pin) throws Exception
{
return TestCaseUtils.makeEntry(
"dn: cn=Reconfigured Key Manager Provider,cn=SSL,cn=config",
"objectClass: top",
"objectClass: ds-cfg-key-manager-provider",
"objectClass: ds-cfg-file-based-key-manager-provider",
"cn: Reconfigured Key Manager Provider",
"ds-cfg-java-class: org.opends.server.extensions.FileBasedKeyManagerProvider",
"ds-cfg-enabled: true",
"ds-cfg-key-store-file: config/reconfigured-test.keystore",
"ds-cfg-key-store-pin: " + pin);
}Pin: the last assertion fails with the block at :397-400 deleted.
suggestion (non-blocking): No test pins "not retried until the file changes again".
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileBasedKeyManagerProvider.java:297; FileBasedTrustManagerProvider.java:276
The mutant that leaves loaded unchanged in the catch block passes both tests. Retrying an unchanged bad file fails again and returns the same manager, and neither test checks logs or load counts. A PIN source that is not stamped makes a retry visible:
// provider entry with ds-cfg-key-store-pin-property: reload.test.pin instead of the PIN file
System.setProperty("reload.test.pin", "password");
// ... provider, keyManager, serverCertificate as in testKeyStoreLoadedAgainWhenChanged
replace(keyStore, withPassword(new File(configDir, "client.keystore"), "password", "changed"));
assertThat(serverCertificateOf(keyManager)).isEqualTo(serverCertificate);
System.setProperty("reload.test.pin", "changed");
assertThat(serverCertificateOf(keyManager)).isEqualTo(serverCertificate);Pin: the mutant retries on the last call and returns the client certificate. The same shape with ds-cfg-trust-store-pin-property covers :276.
suggestion (non-blocking): The trust test does not cover the PIN-file stamp, the PIN re-read or the choice between the plain and extended wrapper.
opendj-server-legacy/src/test/java/org/opends/server/extensions/FileBasedTrustManagerProviderTestCase.java:315
The test uses a literal ds-cfg-trust-store-pin: password and no PIN file, and it only calls getAcceptedIssuers(). These mutants all pass: dropping the PIN file from stampFiles() (:231), dropping trustStorePIN = pin (:263), and always handing out ReloadingTrustManager (:225).
// provider entry with ds-cfg-trust-store-pin-file: config/reload-test.truststore.pin, holding "password"
final X509TrustManager trustManager = (X509TrustManager) provider.getTrustManagers()[0];
assertThat(trustManager).isNotInstanceOf(X509ExtendedTrustManager.class);
assertThat(trustManager.getAcceptedIssuers()).hasSize(3);
replace(trustStore, FileBasedKeyManagerProviderTestCase.withPassword(
new File(configDir, "client.truststore"), "password", "changed"));
assertThat(trustManager.getAcceptedIssuers()).hasSize(3);
replace(pinFile, ("changed" + EOL).getBytes(StandardCharsets.UTF_8));
assertThat(trustManager.getAcceptedIssuers()).hasSize(2);Pin: withPassword becomes package-private. Dropping the PIN-file stamp or the PIN re-read makes the last assertion fail. The isNotInstanceOf fails if an extended wrapper is always handed out.
suggestion (non-blocking): No test pins that the file key catches a rename that keeps size and mtime.
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileStamp.java:92
Every replace() in both tests also changes the size, so the mutant that drops Objects.equals(fileKey, other.fileKey) passes. That is the Kubernetes Secret case the description names.
public class FileStampTestCase extends ExtensionsTestCase
{
@Test
public void testRenameWithSameSizeAndTimeChangesStamp() throws Exception
{
final Path file = Files.createTempFile("stamp", ".bin");
Files.write(file, new byte[] { 1 });
if (Files.readAttributes(file, BasicFileAttributes.class).fileKey() == null)
{
throw new SkipException("no file key on this file system");
}
final FileTime time = Files.getLastModifiedTime(file);
final List<FileStamp> before = FileStamp.of(file.toFile());
final Path tmp = Files.createTempFile(file.getParent(), "stamp", ".tmp");
Files.write(tmp, new byte[] { 2 });
Files.setLastModifiedTime(tmp, time);
Files.move(tmp, file, StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE);
assertThat(FileStamp.of(file.toFile())).isNotEqualTo(before);
}
}Pin: the assertion fails with the file-key term dropped. The skip covers Windows, where fileKey() is null and build-maven also runs
|
A note carried over from the review of #1100, recorded here so it is not lost. It is not a change request. In #1100, a keystore that comes with a new password is copied into
The fix would be for Tracked as #1105. |
…e again when the file changes A connection handler builds its SSL context once, so a renewed key store file was not used until the server restarted or the handler's configuration changed. The key and trust managers handed out by the file based providers now check the file, and its PIN file, when a handshake starts (a certificate is checked, for a trust store) and load it again when it has changed. A file that cannot be loaded leaves the last good content in use and is logged once, until it changes again. Fixes OpenIdentityPlatform#1095
…al holds no key a handler can present, and read the PIN on every load Review round 2 of OpenIdentityPlatform#1101: - a reloaded key store with no private key, or with none under the aliases the last good one held keys under, leaves the last good key manager in use and logs why (ERR_FILE_KEYMANAGER_NO_KNOWN_KEY_ALIAS_656); a configuration change clears the aliases remembered; - the PIN is no longer cached: every load reads the PIN the configuration names at that moment, so a failed reload leaves nothing behind, and getKeyManagers() / getTrustManagers() follow a renewed PIN file; - a plain trust manager handed out takes a new trust manager of either kind, so a server that has turned to FIPS mode keeps loading a renewed trust store; - the stamps are taken again under the lock, and those are recorded; - tests for getServerAliases, the reset on a configuration change, no retry of an unchanged bad file, the trust store PIN file and wrapper kind, and the file key of FileStamp.
a1e78af to
735b9aa
Compare
|
Thanks for the review. All ten points are taken, in 735b9aa. I rebased the branch onto the current 1. A reloaded store with no usable key replaced the last good one. Taken, both halves. The reload now keeps the last good key manager in two cases: when the new store holds no private key (
Before this round, the test's
The second step is also caught by the alias guard, since an empty alias set shares no alias with the last store. So the no-key guard is pinned where no aliases are remembered: after a configuration change, in 2. 3. The class comparison rejected every trust reload once FIPS came up. Taken as suggested. A plain trust manager that was handed out accepts a new manager of either kind; an extended one requires an extended one. 4 + 5. The cached PIN. Taken, by removing it. This also removes the gap recorded above from the review of #1100 and tracked as #1105: 6. Stamps taken before the lock. Taken: the stamps are taken again under the lock, and those are the ones recorded. No test pins this; it would take a controlled interleaving of two handshakes. 7. The 8. "Not retried until the file changes" was untested. Taken on both sides: 9. The trust test lacked the PIN file, the PIN re-read and the plain/extended check. Taken. The test now uses 10. A rename with the same size and mtime was untested. Taken: new I also added the missing Verification (
|
…le loads without a handshake in between The PIN is read on every load since the previous commit, so getKeyManagers() / getTrustManagers() open a key store or trust store renewed together with its PIN file with the new PIN, where they used the PIN held since the provider was configured. Pin it on both sides: replace the store and its PIN file, then ask the provider itself, as a connection handler rebuilding its SSL context does. Fixes OpenIdentityPlatform#1105
|
A follow-up to the round above: 6705997 adds the test that #1105 proposes, so this PR now also fixes #1105 (
Verification: the two classes are green (17/17, 15/15). A mutant that restores the round-1 behaviour turns exactly these two tests red, with the failure #1105 describes: |
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: Round 2 closes all ten points of round 1 where the bug is, and the reload path is now careful about what it records.
FileStampadds the file key to mtime and size, so a file renamed over another with the same size and mtime is caught, as renewal agents and Kubernetes Secret volumes replace it (FileStampTestCase).- The stamps are taken again under the lock, and those are the ones recorded (
FileBasedKeyManagerProvider.java:293,FileBasedTrustManagerProvider.java:263). - The PIN is no longer cached: every load reads
currentPIN()(FileBasedKeyManagerProvider.java:210), so a failed reload leaves no stale PIN behind.
issue (blocking): getKeyManagers() replaces the key manager of every running handler and skips the reload guards.
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileBasedKeyManagerProvider.java:248-265, :304-313
getKeyManagers() writes the provider-wide loaded, which every ReloadingKeyManager handed out earlier reads (:111-153, :284). It only logs a store with no private key (:254-258) and never runs the alias check. A handler's configuration dry run calls it before the change can be refused: LDAPConnectionHandler2.isConfigurationChangeAcceptable :629 → :584 createSSLContext → getKeyManagers() :965/:978/:987 (LDAPConnectionHandler :776 → :720 → :1328/:1346). Take a renewal that moves the key to alias 1 (cert-manager's alias), which the handshake path refused at :309. Any later dsconfig set-connection-handler-prop on a handler that uses the same provider, refused or not, then installs the refused store with current stamps. With ssl-cert-nickname: server-cert, every LDAPS handshake fails until the file changes again. At BASE the dry run's key managers were private and thrown away. 6705997 changes only tests, so this is still there at the current head.
final KeyManager[] keyManagers = loadKeyManagers(keyStore, pin);
if (keyManagers.length != 1 || !(keyManagers[0] instanceof X509ExtendedKeyManager))
{
return keyManagers;
}
synchronized (this)
{
// a store already handed out is replaced only by the guarded reload in currentKeyManager()
if (loaded == null)
{
loaded = new LoadedKeyManager(stamps, keyAliases, (X509ExtendedKeyManager) keyManagers[0]);
}
}
return new KeyManager[] { new ReloadingKeyManager() };Pin: in testKeyStoreLoadedAgainWhenChanged, after the other-cert step is refused, call provider.getKeyManagers() once more and assert that the manager handed out first still serves the client certificate. This goes red at 735b9aa.
suggestion (non-blocking): No test pins that a failed reload keeps the remembered aliases.
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileBasedKeyManagerProvider.java:326
The catch keeps current.keyAliases. A mutant that records Collections.emptySet() there stays green. In testKeyStoreLoadedAgainWhenChanged, every store after the first failed load is either refused by the no-key check (:304, which runs before the alias check) or holds server-cert. With the mutant, one half-written file turns the alias guard off for the next renewal. I traced this by reading and did not run it.
Pin: after the "not a key store" step, write the server key under other-cert again and assert that the certificate stays the same. That kills the mutant.
suggestion (non-blocking): No test pins the relaxed trust kind check, where a plain manager accepts an extended reload.
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileBasedTrustManagerProvider.java:276
Both trust tests reload ExpirationCheckTrustManager → ExpirationCheckTrustManager with isFips() false. So round 1's trustManagers[0].getClass() != current.trustManager.getClass(), restored as a mutant, stays green. The fix for a server that turns FIPS mode on while running could be reverted and no test would notice.
Pin: load the store once, make isFips() true in this class's JVM (FipsStaticUtils.registerBcProvider(true) inserts BCFIPS at runtime), renew the file, and assert that getAcceptedIssuers() follows the new store.
suggestion (non-blocking): No test calls four of ReloadingKeyManager's six alias methods.
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileBasedKeyManagerProvider.java:109-136, opendj-server-legacy/src/test/java/org/opends/server/extensions/FileBasedKeyManagerProviderTestCase.java:459-469
The tests reach the reload only through chooseEngineServerAlias and getServerAliases. If getClientAliases, chooseClientAlias, chooseEngineClientAlias or chooseServerAlias(Socket) delegated to loaded.keyManager instead of currentKeyManager(), every test would stay green. Production uses two of those roads. JMX without a nickname hands the unwrapped manager to a server SSLSocket (RmiConnector.java:346), and OAuth2 client TLS (HttpOAuth2AuthorizationMechanism.java:123) calls the client methods. A mutant on either road keeps presenting the stale certificate after a renewal.
Pin: after one renewal, read keyManager.getCertificateChain(keyManager.chooseClientAlias(new String[] { "RSA" }, null, null))[0] before any other call. After a second renewal, read keyManager.getCertificateChain(keyManager.chooseServerAlias("RSA", null, null))[0] first. Assert the renewed certificate each time.
suggestion (non-blocking): No test pins the stamp reset in the trust provider's applyConfigurationChange.
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileBasedTrustManagerProvider.java:375-379
No test calls the trust provider's applyConfigurationChange, so deleting the reset stays green. Without the reset, a trust store renewed before its trust-store-pin change keeps the old CA set in live contexts until the file changes again. This is the trust half of round-1 point 7.
Pin: add the counterpart of testKeyStoreLoadedAgainWhenPinChangesInConfiguration. Write client.truststore under a new password while the configuration still names the old PIN; getAcceptedIssuers() stays at 3. Then call applyConfigurationChange with the new PIN and assert 2.
question (non-blocking): Which trust-wrapper mutant did you measure red: "an always plain trust wrapper" (the PR body) or "always the extended trust wrapper" (your round-1 reply)?
opendj-server-legacy/src/test/java/org/opends/server/extensions/FileBasedTrustManagerProviderTestCase.java:324
The test JVM is not in FIPS mode, and the loaded manager is ExpirationCheckTrustManager, a plain X509TrustManager. Under an always-plain getTrustManagers(), assertThat(trustManager instanceof X509ExtendedTrustManager).isEqualTo(isFips()) is false == false, so only "always extended" fails. If the body means the extended (FIPS) branch of getTrustManagers() is pinned, it is not: no CI cell runs failsafe in FIPS mode, and that makes this a Major test gap. If it is a slip, correct the body.
Pin: make isFips() true in this class's JVM (FipsStaticUtils.registerBcProvider(true); I did not run this) and assert that getTrustManagers()[0] is an X509ExtendedTrustManager.
nitpick (non-blocking): The Javadoc of currentKeyManager() says the alias guard protects the key named by a handler's ssl-cert-nickname, but the rule it implements does not.
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileBasedKeyManagerProvider.java:274-281, :309
Take a last good store with {A, B}, a handler with nickname A, and a renewal with only {B}. :309 passes because B is shared, the renewal is taken, and the handler has no key to present. The any-alias rule is your stated trade-off. The Javadoc and the commit subject ("holds no key a handler can present") should say that the guard catches only a renewal that shares no alias with the last good store.
…nded out load the file on its own getKeyManagers() stored what it loaded in a field shared by every key manager handed out before, without the checks a reload runs. A handler's configuration check calls it, so it could install a key store that a reload had refused, for every running handler. Each key manager and trust manager handed out now keeps what it last loaded. A configuration change reaches them through a counter of changes applied. Tests pin the managers handed out before a new getKeyManagers() / getTrustManagers() call, the aliases kept by a failed load, the four other alias choices, the reset on the trust side, and both FIPS paths, through a package-private isFipsMode() that a test overrides.
|
Thanks for the review. All seven points are taken, in 6fd0db1. The blocking one is fixed differently from the snippet; the reason is below. The branch base is unchanged: it is still the current 1 (blocking). I did not take the
Pin: 2. A failed reload keeping the aliases was unpinned. Taken, with one change to the pin. After the "not a key store" step, the manager presents the server certificate, so writing the server key under 3. The relaxed trust kind check was unpinned. Taken, but not through 4. Four of the six alias methods were not called. Taken: 5. The reset in the trust 6. Question: which trust wrapper mutant was measured? "Always the extended trust wrapper", as in my round-1 reply. "Always plain" in the PR body was a slip, and you are right that it could not have failed outside FIPS mode. The FIPS branch is now pinned: 7. Nitpick: the Javadoc overclaimed. Taken. The Javadoc of Verification:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| /** The trust manager handed out by {@link #getTrustManagers()} over a plain trust manager. */ | ||
| private final class ReloadingTrustManager implements X509TrustManager |
| } | ||
|
|
||
| /** The trust manager handed out by {@link #getTrustManagers()} over an extended trust manager. */ | ||
| private final class ReloadingExtendedTrustManager extends X509ExtendedTrustManager |
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: 6fd0db1 fixes the round-2 blocker at its root: each manager handed out now keeps its own state, so nothing the provider is asked later can reach it.
getKeyManagers()andgetTrustManagers()wrap a freshLoadedKeyManager/TrustStoreFolloweron each call and write no shared state (FileBasedKeyManagerProvider.java:335-354). A handler's configuration dry run can no longer swap a live handler's key.testKeyManagerHandedOutAgainLeavesEarlierOnesAloneandtestTrustManagerHandedOutAgainLeavesEarlierOnesAlonepin it.- The
ReloadingKeyManagerJavadoc now states the trade-off: a renewal that keeps any remembered alias is taken, even if it drops the alias a handler names (FileBasedKeyManagerProvider.java:130-139). - #1105 is closed with a test on each side (
testKeyStoreAndPinRenewedTogetherLoadedWithoutHandshake,testTrustStoreAndPinRenewedTogetherLoadedWithoutCheck). Both provider classes ran 19/19 green on the Linux JDK 17 cell.
issue (non-blocking): A handshake's reload no longer excludes applyConfigurationChange, so it can load a mix of the old and the new configuration.
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileBasedKeyManagerProvider.java:140-190, :449-457; FileBasedTrustManagerProvider.java:135, :427
currentKeyManager() is now a method of the inner ReloadingKeyManager, so synchronized (this) at :147 takes the manager's monitor. applyConfigurationChange still sets currentConfig, keyStoreFile and keyStoreType under the provider's monitor (:449-457). A reload that reads count N while a new key-store-file or PIN is being applied can pair the new file with the old PIN, or check the new aliases against N's remembered ones. That load fails: ERR_FILE_KEYMANAGER_CANNOT_RELOAD is logged for a store that is fine, and the handshake keeps the old key. The attempt is recorded under N, so the next handshake loads again and nothing stays wrong. The volatile count covers only a reader that sees the new count. The trust side has the same window. At 735b9aa both methods shared the provider's monitor.
synchronized (this)
{
// applyConfigurationChange sets the configuration a load reads under the provider's
// monitor: take it too, so a load sees the configuration before a change or after it
synchronized (FileBasedKeyManagerProvider.this)
{
final int changes = configurationChanges;
final List<FileStamp> stamps = stampFiles();
// ... the rest of the current block, unchanged
}
}The lock order is always manager → provider. applyConfigurationChange takes only the provider's monitor and getKeyManagers() takes none, so there is no cycle. The same change applies in TrustStoreFollower.currentTrustManager() with FileBasedTrustManagerProvider.this.
suggestion (non-blocking): No test pins the extended half of the trust kind check, in either direction.
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileBasedTrustManagerProvider.java:118, :152, :212
The two FIPS tests reload only plain → extended (:503-521) and extended → extended (:533-550). Two mutants survive the class (traced, not run):
- Delete
|| extended && !(trustManagers[0] instanceof X509ExtendedTrustManager). An extended manager handed out then takes a plainExpirationCheckTrustManager, andcurrent()(:212) throwsClassCastExceptionon every certificate check. - Restore 735b9aa's per-load
current.trustManager instanceof X509ExtendedTrustManager. Then plain handed out → extended load → plain load is refused.
This state is reachable outside tests. On a non-FIPS server, Platform.generateSelfSignedCertificate (Platform.java:251-296) registers BCFIPS and removes it in a finally, and isFips() is not cached, so FIPS mode can turn off again at runtime.
// testPlainTrustManagerTakesExtendedOneWhenFipsModeTurnsOn, after the last assert
provider.fips = false;
replace(trustStore, Files.readAllBytes(new File(configDir, "server.truststore").toPath()));
assertThat(trustManager.getAcceptedIssuers()).hasSize(3);
// testTrustManagerHandedOutInFipsModeIsExtended, after the last assert
provider.fips = false;
replace(trustStore, Files.readAllBytes(new File(configDir, "server.truststore").toPath()));
assertThat(trustManager.getAcceptedIssuers()).hasSize(2);Pin: the first block turns the per-load mutant red (it stays at 2). The second turns the deleted-clause mutant red (ClassCastException instead of the kept two issuers).
suggestion (non-blocking): No test calls a check method of a handed-out file-based trust manager; every reload test reads only getAcceptedIssuers().
opendj-server-legacy/src/main/java/org/opends/server/extensions/FileBasedTrustManagerProvider.java:182-191, :216-253
A search for checkClientTrusted|checkServerTrusted under */src/test/* finds only TrustManagersTestCase:212 (opendj-core) and BlindTrustManagerProviderTestCase:70-71. Three mutants survive the class. One check method skips the reload and delegates to follower.loaded.trustManager. One has an empty body and trusts every chain. One SSLEngine/Socket overload falls back to the 2-arg one. The description says the stamp check "happens before each certificate check", and this is the path that decides trust. The reload itself is pinned, because getAcceptedIssuers() goes through the same currentTrustManager().
final java.security.cert.X509Certificate[] before = trustManager.getAcceptedIssuers();
replace(trustStore, Files.readAllBytes(new File(configDir, "client.truststore").toPath()));
// checked before any getAcceptedIssuers() call loads the renewal for it
final List<java.security.cert.X509Certificate> refused = new java.util.ArrayList<>();
for (java.security.cert.X509Certificate cert : before)
{
try
{
trustManager.checkClientTrusted(new java.security.cert.X509Certificate[] { cert }, "RSA");
}
catch (java.security.cert.CertificateException e)
{
refused.add(cert);
}
}
final List<java.security.cert.X509Certificate> kept = java.util.Arrays.asList(trustManager.getAcceptedIssuers());
for (java.security.cert.X509Certificate cert : before)
{
if (!kept.contains(cert))
{
assertThat(refused).contains(cert);
}
}Pin: in a copy of testTrustManagerHandedOutAgainLeavesEarlierOnesAlone's setup, a certificate the renewal drops is refused by the first check after the renewal. Unmeasured. It assumes client.truststore does not keep an issuer of the certificate it drops.
3 participants
Problem
A connection handler builds its
SSLContextonce, when it is initialized or when its own configuration changes (LDAPConnectionHandler#configureSSL). The key managers in that context hold the key store as it was read at that moment. So a renewed key store file was not used until the server restarted, or until some property of the handler was changed. Measured onopenidentityplatform/opendj:latest: aftercpof a newconfig/keystore, LDAPS keeps serving the old certificate, and adsconfig set-connection-handler-propon the handler makes it serve the new one. Certificates renewed by an agent (cert-manager, certbot, Vault agent) need the new file used without a restart. The Docker side is #1087 / #1100.Change
FileBasedKeyManagerProviderandFileBasedTrustManagerProviderhand out a key manager / trust manager that delegates to the one it last loaded from the file. Each manager handed out keeps its own state and loads the file on its own. So a latergetKeyManagers()/getTrustManagers()call, which a connection handler makes when it checks or applies a configuration change, does not change the managers already in use:Before a handshake chooses its alias, the manager compares the stamps of the store file and of the PIN file (if one is configured) with the stamps taken when they were loaded. For a trust manager the check happens before each certificate check and before
getAcceptedIssuers. A stamp is the modification time, size and file key (inode) fromBasicFileAttributes, with symbolic links followed (new package-privateFileStamp). The file key catches a file replaced by rename, the way renewal agents and Kubernetes Secret volumes replace it.When a stamp has changed, the manager loads the store again under its own lock and logs
NOTE_FILE_{KEY,TRUST}MANAGER_RELOADED. The stamps are taken again inside the lock, and those are the ones recorded. Each manager handed out logs its own reload, so a renewal is logged once per manager that sees it.The PIN is not cached. Every load, including
getKeyManagers()/getTrustManagers()andcontainsAtLeastOneKey()/containsKeyWithAlias(), reads the PIN that the configuration names at that moment. So a failed load leaves no stale PIN behind, and a renewed PIN file is followed everywhere. This closes A key store and PIN renewed together fail to load in getKeyManagers() / getTrustManagers() until a handshake reloads the PIN #1105: a store renewed together with its PIN file loads throughgetKeyManagers()/getTrustManagers()without a handshake in between.The last good manager stays in use, and
ERR_FILE_{KEY,TRUST}MANAGER_CANNOT_RELOADis logged once per manager, when the new file:ERR_FILE_KEYMANAGER_NO_KNOWN_KEY_ALIAS).The provider does not see a handler's
ssl-cert-nickname. A renewal that moves the key to another alias would leave the handler nothing to present, so the aliases of the last good store stand in for the nickname. The guard catches only a renewal that shares no alias with the last good store: a store with keys{A, B}renewed to{B}is taken, even if a handler namesA. The stamps of the failed attempt are recorded, so the file is not tried again until it changes again. This covers a store and its PIN renewed as two separate writes. After such a refusal, a handler reconfigured to the new alias gets a new key manager that loads the file as it is.getCertificateChain/getPrivateKeyuse the manager the alias was chosen from and do not look at the file. The one remaining window is noted in a comment: another handshake loading the file again between those calls.A plain trust manager (
ExpirationCheckTrustManager, the non-FIPS path) is handed out as a plainX509TrustManager, and an extended one (FIPS) as an extended one, so JSSE keeps adding the same checks around it as before. A plain one handed out accepts a reloaded manager of either kind, sinceisFips()can turn true while the server runs. An extended one requires an extended one. The provider asks a package-privateisFipsMode(), which returnsisFips()and which a test overrides.applyConfigurationChangecounts the configuration changes applied. A manager handed out that sees a new count loads the file again on its next use, and the alias guard is off for that load. So a changedkey-store-file/ PIN reaches the existing SSL contexts too.Messages 652-656 are added to
extension.properties. No open PR uses these numbers.getKeyManagers()/getTrustManagers()still load the file on the call and throw as before when it cannot be loaded. A key store without a private key is still only logged there, so configuration validation is unchanged.Tests
FileBasedKeyManagerProviderTestCase#testKeyStoreLoadedAgainWhenChanged: a key manager handed out once follows the file through these steps:server.keystore;client.keystorekey underserver-cert, read throughgetServerAliases(the path LDAPS takes withssl-cert-nickname);other-cert, and thenserver.truststore(no private key): the client certificate stays;server.keystoreunder a new password, with the new PIN file written after the key store: the certificate stays the previous one until the PIN file arrives;other-cert: still refused, because the failed load in step 5 kept the aliases.#testKeyManagerHandedOutAgainLeavesEarlierOnesAloneandFileBasedTrustManagerProviderTestCase#testTrustManagerHandedOutAgainLeavesEarlierOnesAlone: after a manager in use refuses the file, or fails to load it, the provider is asked again. The new manager loads the file as it is (on the key side, the key underother-cert, the handler-reconfigured-to-the-new-alias case), and the manager handed out before keeps what it had.#testEveryAliasChoiceLoadsAgain:getClientAliases,chooseClientAlias,chooseEngineClientAliasandchooseServerAlias(Socket)(the OAuth2 client and JMX paths) each see a renewal first.#testKeyStoreLoadedAgainWhenPinChangesInConfigurationandFileBasedTrustManagerProviderTestCase#testTrustStoreLoadedAgainWhenPinChangesInConfiguration: a PIN changed only in the configuration makes the manager load the unchanged file again. On the key side, after a further configuration change, a store with no private key is still not taken.#testKeyStoreNotLoadedAgainUntilChangedandFileBasedTrustManagerProviderTestCase#testTrustStoreNotLoadedAgainUntilChanged: with the PIN in a system property, which is not stamped, a file that failed to load is not tried again when only the PIN changes.FileBasedTrustManagerProviderTestCase#testTrustStoreLoadedAgainWhenChanged: with a PIN file, the test goesserver.truststore(3 issuers) →client.truststoreunder a new password (still 3 until the PIN file changes, then 2) → not a trust store (still 2).FileBasedTrustManagerProviderTestCase#testPlainTrustManagerTakesExtendedOneWhenFipsModeTurnsOnand#testTrustManagerHandedOutInFipsModeIsExtended: withisFipsMode()overridden by a test subclass, rather than a FIPS security provider inserted into the shared test JVM. A plain manager handed out outside FIPS mode follows a renewal loaded after FIPS mode turns on. In FIPS mode, the manager handed out is anX509ExtendedTrustManagerand follows a renewal.#testKeyStoreAndPinRenewedTogetherLoadedWithoutHandshakeandFileBasedTrustManagerProviderTestCase#testTrustStoreAndPinRenewedTogetherLoadedWithoutCheck(A key store and PIN renewed together fail to load in getKeyManagers() / getTrustManagers() until a handshake reloads the PIN #1105): the store and its PIN file are replaced together, and the provider itself is asked with no handshake or certificate check in between, as a connection handler rebuilding its SSL context does. On the key sidecontainsAtLeastOneKey()/containsKeyWithAlias()see the new store too.FileStampTestCase: a file renamed over another with the same size and modification time changes the stamp. The test is skipped where the file system has no file key.getKeyManagers()/getTrustManagers()loading with the PIN read when the provider was initialized (the state before this PR's second commit);getKeyManagers()/getTrustManagers()call replaces it or only the first one sets it;getServerAliases,getClientAliases,chooseClientAlias,chooseEngineClientAliasorchooseServerAliasasking the manager last loaded;FileStampwithout the file key.Fixes #1095
Fixes #1105