Repository navigation
[#1188] Serialize and replace atomically the writes of the ads-truststore, and report a certificate it cannot take or drop - #1189
Conversation
…es of the ads-truststore, and report a certificate it cannot take or drop
Every change to the ads-truststore reads the file, changes that copy and writes the whole file
back over the old one, truncating it first, and nothing kept two of them apart. Replication
replays the instance keys of cn=admin data on parallel threads (16 at least), each of which
copies its key into the trust store, and an SSL context is built from the file for every
replication connection. Two keys replayed together lost one of them, and a reader could find the
file half written ("Keystore was tampered with", "signed overrun"). A directory server which
lost the certificate of a replication server that way failed every handshake with it.
TrustStoreBackend now holds one lock over every access to the file and to its certificate
manager: reads, adds, deletes, the instance certificate it generates, and the change of its
configuration. The key managers and the keys it hands out are opened under that lock too, with
the PIN of the file they were read from rather than that of a configuration changed meanwhile.
The lock keeps apart the readers and writers of this server only. Platform now writes every key
store to a file of its own next to it and moves that over the old one at once, so a reader
outside the lock never finds the file half written either, and a server stopped part way
through a write keeps the old file whole instead of an empty one it cannot start with. The file
replaced keeps its permissions, a key store path which is a link goes on pointing where it did,
and a new key store, or one which cannot be replaced that way (a directory this process cannot
write to, a file mounted on its own into a container), is written in place as before.
CryptoManagerSync reported an instance key it could not copy into the trust store, and one it
could not drop from it, at debug level only: a server which did not trust another one said
nothing about why, and one which kept trusting a compromised key said nothing either. Both are
errors now (INFO_TRUSTSTORESYNC_ADD_FAILED_655 and INFO_TRUSTSTORESYNC_DELETE_FAILED_656 become
ERR_, same ordinals), except where nothing failed: a certificate the trust store holds already
(dsreplication seeds the trust store of the servers it enables) and one it holds no longer (it
leaves when its key is marked compromised). TrustStoreBackend now keeps the result code of
those two refusals, an alias in use (entry already exists) and an alias not in use (no such
entry), instead of turning them into a server error, which CryptoManagerSync would have reported.
Not fixed here: OpenIdentityPlatform#1188 also found an instance key stored in cn=admin data without its
certificate, which no trust store can take. The scenario of the issue, run three times on this
change, did not reproduce it; its cause is still open.
Tests:
- CryptoManagerTestCase: sixteen instance keys added together all reach the trust store while a
reader builds trust managers from the file the whole time; an instance key the trust store
refuses is in the error log, and one it holds already is not; a compromised key it cannot
drop is in the error log, and the delete of a key whose certificate has left already is not;
the trust store refuses an alias in use and one not in use with their own result codes.
- CertificateManagerTestCase: a reader of a key store file never finds it half written while
it is rewritten; the rewrite keeps the permissions of the file and the link to it; a key
store in a directory this process cannot write to is rewritten in place.
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: The two halves of #1188 on the trust store side are fixed where they break, and each fix has a pin that fails without it.
TrustStoreBackend.trustStoreLockserializes every file andCertificateManageraccess: withTrustStoreBackend.javareverted to the base,CryptoManagerTestCase.testInstanceKeysAddedTogetherAllReachTheTrustStorekeeps 9 of 16 instance keys andtestTrustStoreRefusesAnAliasByWhatIsWrongWithItgetsOtherinstead ofEntry Already Exists(48 run, exactly these 2 red); both are green at the head.Platform.PlatformIMPL.storewrites to a temp file, copies the mode and replaces throughFiles.move(ATOMIC_MOVE)overtoRealPath(), so a link keeps pointing where it did;CertificateManagerTestCase.testKeyStoreFileIsNeverSeenHalfWrittengoes red against in-place writes (5/5) and stays green at the head.
issue (non-blocking): A key store file this process cannot write to, in a directory it can write to, is now replaced instead of refused.
opendj-server-legacy/src/main/java/org/opends/server/util/Platform.java:158-181
store() takes the rename road whenever the target exists and createTempFile succeeds in its parent; nothing checks Files.isWritable(target). rename(2) needs write permission on the directory only, and the old r--r--r-- mode is copied onto the new content. At the base, new FileOutputStream(ksPath) failed with Permission denied and the add/delete was refused; at the head the file is rewritten and still looks read-only (measured with a single-file probe). Not a security boundary — the directory was writable — but the javadoc lists only a new file, an unwritable directory and a single-file mount as in-place cases.
target = target.toRealPath();
if (!Files.isWritable(target))
{
storeInPlace(ks, target, pwd);
return;
}Or: state the change in the store() javadoc.
issue (non-blocking): The atomic replace keeps the mode of the old key store, but not its owner, group, ACLs or hard links.
opendj-server-legacy/src/main/java/org/opends/server/util/Platform.java:162, :177, :181
The temp file gets the writer's euid/egid from createTempFile, only setPosixFilePermissions copies the old mode, then Files.move replaces the inode. The base wrote in place, so owner, group, ACLs and hard links stayed. An ads-truststore owned opendj:sidecar with mode 0640 (config/ not setgid) becomes opendj:opendj 0640 at the first replayed instance key, and the sidecar loses read; a tool run once as root over an opendj-owned 0600 key store leaves it root-owned and the server can no longer read it. The description promises the file "keeps its permissions", which holds for the mode only.
if (Files.getFileStore(target).supportsFileAttributeView(PosixFileAttributeView.class))
{
final PosixFileAttributes old = Files.readAttributes(target, PosixFileAttributes.class);
final PosixFileAttributeView view = Files.getFileAttributeView(temp, PosixFileAttributeView.class);
view.setPermissions(old.permissions());
try
{
view.setGroup(old.group());
}
catch (IOException e)
{
// Not a member of that group: the check below writes in place.
}
final PosixFileAttributes now = view.readAttributes();
if (!now.owner().equals(old.owner()) || !now.group().equals(old.group()))
{
storeInPlace(ks, target, pwd);
return;
}
}(import java.nio.file.attribute.PosixFileAttributes; — the finally still deletes the temp file.) ACLs and hard links are still lost on the move road; say so in the javadoc.
question (non-blocking): Is the "never found half written" guarantee meant to hold on Windows?
opendj-server-legacy/src/test/java/org/opends/server/util/CertificateManagerTestCase.java:1437, opendj-server-legacy/src/main/java/org/opends/server/util/Platform.java:183-186
On Windows java.io.FileInputStream opens with FILE_SHARE_READ | FILE_SHARE_WRITE, without FILE_SHARE_DELETE; MoveFileEx(MOVEFILE_REPLACE_EXISTING) over a file held open that way fails, and store() falls back to storeInPlace, the truncating write the reader's FILE_SHARE_WRITE permits. testKeyStoreFileIsNeverSeenHalfWritten holds exactly such a stream open in a loop and, unlike its two siblings, does not call skipUnlessPosix(); a reader outside trustStoreLock (FileBasedTrustManagerProvider, another process) can still see a truncated file there — never worse than the base. Not run: no Windows host here, and no CI cell runs the opendj-server-legacy failsafe suite on Windows. Minor either way: if the guarantee is POSIX-only, skip the test and say so in the javadoc; if it is meant for Windows too, the reader needs another open mode.
public void testKeyStoreFileIsNeverSeenHalfWritten()
throws Exception
{
skipUnlessPosix();suggestion (non-blocking): The fallback after a failed Files.move is reached by no test.
opendj-server-legacy/src/main/java/org/opends/server/util/Platform.java:183-186, opendj-server-legacy/src/test/java/org/opends/server/util/CertificateManagerTestCase.java:1492, :1527
testKeyStoreRewriteKeepsPermissionsAndLink and testKeyStoreInADirectoryNotWritableIsRewrittenInPlace kill the mutants that drop toRealPath(), the mode copy and the createTempFile catch, but both are green at the base too (the in-place write keeps link, inode and mode). A mutant that removes the catch (IOException e) { storeInPlace(...) } around Files.move survives; without it, a key store mounted on its own into a container (rename fails with EBUSY/EXDEV) would fail every trust store add and delete.
Pin: a package-private seam for the move (or one that makes it throw AtomicMoveNotSupportedException), and an assertion that the key store still takes the certificate when the move fails — red when the catch is removed.
suggestion (non-blocking): No concurrent test covers the delete path or the configuration change under trustStoreLock.
opendj-server-legacy/src/main/java/org/opends/server/backends/TrustStoreBackend.java:504-520, :838, opendj-server-legacy/src/test/java/org/opends/server/crypto/CryptoManagerTestCase.java:348
The only concurrent trust store test is testInstanceKeysAddedTogetherAllReachTheTrustStore (16 processAdd tasks and a getTrustManagers loop); its processDelete calls run sequentially in finally. Mutants that drop the lock from deleteEntry or from the configuration change stay green, so a delete lost to a concurrent add during replay — a compromised key left trusted — would turn nothing red.
Pin: a sibling of the add race — N threads processAdd new instance keys while M threads processDelete ones added before; assert every added alias is in the trust store and no deleted alias is. Red when the lock is dropped from deleteEntry.
suggestion (non-blocking): The read lock in TrustStoreBackend.loadKeyStore has no pin while the atomic move works.
opendj-server-legacy/src/test/java/org/opends/server/crypto/CryptoManagerTestCase.java:374
The getTrustManagers reader in testInstanceKeysAddedTogetherAllReachTheTrustStore is protected twice — by the read lock and by the write-then-move. On the test file system the move always succeeds, so a mutant that drops the read lock survives; the lock matters only on the in-place roads (new file, unwritable directory, single-file mount, the Windows fallback), which the test never takes.
Pin: run the same add/read race with the trust store file in a directory of its own made r-x, so store() takes the in-place road and only the read lock keeps the reader from a half-written file.
suggestion (non-blocking): testInstanceKeyTheTrustStoreRefusesIsReported is refused by the schema check, never by the trust store.
opendj-server-legacy/src/test/java/org/opends/server/crypto/CryptoManagerTestCase.java:427, opendj-server-legacy/src/main/java/org/opends/server/backends/TrustStoreBackend.java:1093
The instance key is added without ds-cfg-public-key-certificate; CryptoManagerSync copies it with an internal processAdd that carries no Relax control, so the schema check refuses it with OBJECTCLASS_VIOLATION (ds-cfg-instance-key MUST ds-cfg-public-key-certificate) before TrustStoreBackend.addEntry runs. The logger.error branch in CryptoManagerSync is pinned; addCertificate's own refusal and the new catch (DirectoryException) rethrow at :1093 are not reached, so a mutant on that rethrow stays green.
Pin: give the instance key a ds-cfg-public-key-certificate;binary of bytes that are not a certificate — it passes the schema check and fails CertificateFactory in addCertificate, so the refusal comes from the trust store and the case reaches :1093.
suggestion (non-blocking): testKeyStoreFileIsNeverSeenHalfWritten never checks that its reader overlapped the writes.
opendj-server-legacy/src/test/java/org/opends/server/util/CertificateManagerTestCase.java:1451, :1470
The main thread submits the reader and goes straight into 200 addTrustedCertificate calls; nothing waits for the reader to start and no count of loads is asserted. A reader first scheduled after writing.set(false) returns null without loading, and assertNull passes — at the base as well as at the head. It is the one lock-free pin on the atomic replace, so a vacuous pass on a loaded runner hides a regression.
final CountDownLatch started = new CountDownLatch(1);
final AtomicInteger loads = new AtomicInteger();
final Future<String> reads = executor.submit(() -> {
final KeyStore keyStore = KeyStore.getInstance("JKS");
started.countDown();
while (writing.get())
{
try (FileInputStream in = new FileInputStream(destination))
{
keyStore.load(in, "password".toCharArray());
loads.incrementAndGet();
}
catch (Exception e)
{
return e.toString();
}
}
return null;
});
assertTrue(started.await(60, TimeUnit.SECONDS), "the reader started");
for (int i = 1; i <= 200; i++)
{
manager.addTrustedCertificate("cert-" + i, certificate);
}
writing.set(false);
assertNull(reads.get(60, TimeUnit.SECONDS), "a read of the key store while it was rewritten");
assertTrue(loads.get() > 0, "the reader loaded the key store while it was rewritten");(java.util.concurrent.CountDownLatch, java.util.concurrent.atomic.AtomicInteger.)
suggestion (non-blocking): The finally of testKeyStoreInADirectoryNotWritableIsRewrittenInPlace masks an early failure.
opendj-server-legacy/src/test/java/org/opends/server/util/CertificateManagerTestCase.java:1532, :1552
store is created inside the try, after the CertificateFixture is built; the finally calls Files.setPosixFilePermissions(store, ...) unconditionally. If the fixture (RSA key generation) or the directory creation throws, the finally throws NoSuchFileException, which replaces the real cause in the report and skips TestCaseUtils.deleteDirectory(tmpDir). The case still goes red; only the diagnosis suffers.
finally
{
if (Files.exists(store))
{
Files.setPosixFilePermissions(store, PosixFilePermissions.fromString("rwxr-xr-x"));
}
TestCaseUtils.deleteDirectory(tmpDir);
}…aced key store, refuse one not writable, and pin the in-place roads Review round 1 of OpenIdentityPlatform#1189. Platform replaced a key store file whenever its directory could be written to: a file this process may not write to was replaced instead of refused, as it was before, and the new file had the mode of the old one but not its group, its owner or its other hard links. The rewrite now writes in place - and so refuses a file it may not write to, as before - where replacing the file would change what it is: one not writable, one with other hard links, and one whose owner or group the new file cannot take (a process may give a file only its own user and the groups it is a member of). The new file takes the group of the old one. ACLs are not kept; the javadoc says so, and that the guarantee against a half-written file holds on POSIX only (Windows does not replace a file a reader holds open, and the key store is then written in place). The replace goes through a package-private seam, which a test makes fail, so that the roads where the key store is written in place are reached by tests. Tests: - CertificateManagerTestCase: the rewrite keeps the group and still replaces the file; a key store with another hard link is rewritten in place; one not writable is refused; one which cannot be replaced is rewritten in place. The half-written test is skipped off POSIX, waits for its reader and asserts that it loaded the file; the in-place test no longer hides an early failure in its finally. - CryptoManagerTestCase: the add/read race again with the trust store written in place, where only the lock keeps the reader whole; instance keys added and deleted together; the javadoc of the error-log test says the key without a certificate is refused by the schema check.
|
Thanks. Round 2 is in a40a4a8. Unwritable file replaced instead of refused — fixed as you suggest: after Owner, group, ACLs, hard links — the new file takes the group of the old one ( Windows — POSIX only. The fallback after a failed move — the move now goes through a package-private The read lock in Delete under the lock —
The reader of the half-written test — taken as you wrote it: a latch before the writes and The Mutants of this round, each reverted on its own:
|
vharseko
added
tests
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: Round 1's points are all closed, and the new delete lock has a pin that really works.
Platform.storenow refuses a read-only key store (!Files.isWritable(target)→storeInPlace,Platform.java:186) and writes in place when the file has other hard links (hasOtherLinks) or when the new file cannot get its owner or group (takeOwnerAndMode).testInstanceKeysAddedAndDeletedTogetherAllReachTheTrustStorefails when onlysynchronized (trustStoreLock)is removed arounddeleteCertificateinTrustStoreBackend.deleteEntry. Measured at a40a4a8, red on the first run (1/1): a deleted instance key stays in the trust store.- The
keyStoreReplacerseam withKeyStoreReplaceFailurereaches the in-place fallback without needing a container.
issue (non-blocking): takeOwnerAndMode gives the temp file the old mode before the old group, so for a while the new key store is open to the process's group.
opendj-server-legacy/src/main/java/org/opends/server/util/Platform.java:247, :250, :209, :223
createTempFile creates the file at 0600, and ks.store writes the whole new key store into it. Then setPermissions(old.permissions()) runs before setGroup(old.group()). Take a key store at 0640 (or 0660) whose group differs from the group a new file gets: the process's group on Linux, or the directory's group on macOS or under a setgid directory. Between those two calls, members of a group the old file never trusted can read the new copy (and write it at 0660). If setGroup fails, the window lasts through the whole in-place rewrite, until deleteIfExists. A descriptor opened in that window still works after the rename. The PIN protects the content, which limits the damage but does not close the window. This order is the one my round-1 suggestion spelled out; it was wrong.
final PosixFileAttributes old = Files.readAttributes(target, PosixFileAttributes.class);
final PosixFileAttributeView view = Files.getFileAttributeView(temp, PosixFileAttributeView.class);
try
{
view.setGroup(old.group());
}
catch (IOException e)
{
// not a member of that group: told by the check below
}
final PosixFileAttributes now = view.readAttributes();
if (!now.owner().equals(old.owner()) || !now.group().equals(old.group()))
{
return false;
}
view.setPermissions(old.permissions());
return true;issue (non-blocking): The attribute probes that store() now runs are outside the in-place fallbacks, so a probe that fails refuses a write that BASE made.
opendj-server-legacy/src/main/java/org/opends/server/util/Platform.java:186, :207, :228-232, :239-258
hasOtherLinks and takeOwnerAndMode call Files.getFileStore, Files.getAttribute("unix:nlink"), readAttributes and setPermissions, and nothing catches what they throw. The IOException fallbacks cover only createTempFile and keyStoreReplacer.replace. On Linux, Files.getFileStore throws IOException("Mount point not found") when the path's mount is not listed in /proc/mounts. That happens to a server in a chroot whose root is not a mount point, or one with no /proc. There, an add or delete on the trust store backend fails with ERR_TRUSTSTORE_CANNOT_ADD_CERT / ERR_TRUSTSTORE_CANNOT_DELETE_CERT, where BASE succeeded. DiskSpaceMonitor.registerMonitoredDirectory already catches the same failure and keeps running. Read from the JDK source, not run: there is no Linux chroot here.
boolean otherLinks;
try
{
otherLinks = hasOtherLinks(target);
}
catch (IOException e)
{
otherLinks = true; // cannot tell: write in place, as before
}
if (!Files.isWritable(target) || otherLinks)
{
storeInPlace(ks, target, pwd);
return;
}
// ... and likewise around the owner and mode:
boolean taken;
try
{
taken = takeOwnerAndMode(temp, target);
}
catch (IOException e)
{
taken = false;
}
if (!taken)
{
storeInPlace(ks, target, pwd);
return;
}issue (non-blocking): On a file store without a POSIX view (Windows), store() replaces the file without the owner and hard-link checks that the javadoc promises.
opendj-server-legacy/src/main/java/org/opends/server/util/Platform.java:230, :241-244, :167-172
On Windows, takeOwnerAndMode returns true at once and hasOtherLinks is always false, so every write moves the temp file over the key store. Take a key store owned by another account that the service can write: after the first trust store change it belongs to the service account and has the directory's inherited DACL. A key store with an NTFS hard link loses that link, and the other name keeps the old content. The javadoc and the PR description promise that the new file keeps the owner, and that a file with other hard links, or whose owner the new file cannot take, is written in place. Neither promise says "POSIX only"; only the ACL loss and the half-written reader do. Not run on Windows. Files.getOwner works there; NIO cannot read the NTFS link count.
if (!Files.getFileStore(target).supportsFileAttributeView(PosixFileAttributeView.class))
{
return Files.getOwner(temp).equals(Files.getOwner(target));
}Or: limit the hard-link clause in the javadoc and the description to POSIX ("one with other hard links on a POSIX file system").
suggestion (non-blocking): No test reaches the in-place road taken when the new file cannot get the old owner or group.
opendj-server-legacy/src/main/java/org/opends/server/util/Platform.java:207-210, :257; opendj-server-legacy/src/test/java/org/opends/server/util/CertificateManagerTestCase.java:1593-1615
testKeyStoreRewriteKeepsTheGroup moves the file to a group the process belongs to, so setGroup succeeds and :257 returns true. KeyStoreReplaceFailure swaps only the move, which runs after takeOwnerAndMode. Two mutants survive every test: :257 changed to return true, and the setGroup catch rethrowing. Your reply already lists the first as unpinned. A non-root Linux test cannot build that target, but a seam like keyStoreReplacer can.
/** Gives the new key store file the owner, group and mode of the old one; a test makes it refuse. */
static volatile KeyStoreAttributeCopier keyStoreAttributeCopier = PlatformIMPL::takeOwnerAndMode;Pin: with the copier returning false, add an alias and assert that the alias is in the file, that Files.getAttribute(path, "unix:ino") is unchanged, and that no .ads-truststore.*.tmp is left behind, as testKeyStoreWithAnotherHardLinkIsRewrittenInPlace does for the hard-link road. The test fails with return true at :257.
suggestion (non-blocking): testKeyStoreRewriteKeepsPermissionsAndLink can leave its temp directory behind, depending on the order in which the directory is listed.
opendj-server-legacy/src/test/java/org/opends/server/util/CertificateManagerTestCase.java:1528
TestCaseUtils.deleteDirectory returns early when !dir.exists(), and that check follows links. If store is listed before the link ads-truststore (creation order on tmpfs, hash order on ext4), the link is left dangling and never deleted, and tmpDir.delete() fails without a word. Each run then leaves a rewriteKeeps* directory in java.io.tmpdir.
finally
{
Files.deleteIfExists(tmpDir.toPath().resolve("ads-truststore"));
TestCaseUtils.deleteDirectory(tmpDir);
}…e its mode, write in place where its attributes cannot be told, and on Windows Review round 2 of OpenIdentityPlatform#1189. Platform gave the new key store file the mode of the old one before its group, so between the two calls - and through the whole write in place when the group could not be taken - the new copy was open to a group the old file never trusted. The group now comes first, then the check of the owner and the group, and the mode last, only when the file is to be replaced. The probes of the file's links, owner and group asked Files.getFileStore, which on Linux looks the mount point up in /proc/mounts and throws "Mount point not found" where it is not listed (a chroot whose root is not a mount point, no /proc), and nothing caught what they threw: an add or delete on the trust store failed where it succeeded before. The POSIX attributes are now asked of the file system, and a file whose links, owner or group cannot be told is written in place. On a file system without POSIX attributes (Windows) the key store is written in place, as before: neither the owner, the ACLs nor the hard links of the old file would reach a new one there, and a file a reader holds open cannot be replaced anyway. The group goes through a package-private seam, which a test makes ignore or refuse it. Tests: - CertificateManagerTestCase: a key store whose group the new file cannot take, ignored or refused, is rewritten in place and keeps its group, and no file is left next to it; the link test deletes its link first, so that it leaves no directory behind.
|
Thanks. Round 3 is in c7b6d5e. Mode before group — fixed: Probes outside the fallbacks — fixed, and at the cause: Windows — neither of the two: on a file system without POSIX attributes the key store is now always written in place, as at the base. A check of the owner alone would still have dropped the DACL, which is the whole of the permissions there, and the NTFS link count cannot be read through NIO; a file a reader holds open could not be replaced anyway. The javadoc and the description say so; the clause about a reader on Windows is gone with it. Not run on Windows. The in-place road when the owner or group cannot be taken — pinned, with a narrower seam than the one you suggest: with the whole Leftover directory of the link test — taken as you wrote it: the link is deleted first. Mutants of this round, each reverted on its own:
The mutants of round 2 ( |
2 participants
Fixes #1188 in part: the trust store side. The instance key stored in
cn=admin datawithout its certificate is not explained by this change; see "Not fixed here" below.The defect
Every change to the ads-truststore reads the file, changes that copy and writes the whole file back over the old one, truncating it first, and nothing kept two of them apart. Replication replays the instance keys of
cn=admin dataon parallel threads (16 at least), each of which copies its key into the trust store inCryptoManagerSync, and an SSL context is built from the file for every replication connection. So:Keystore was tampered with,signed overrun,EOFException;bad_certificate).A failed copy into the trust store was logged at debug level only, so the server said nothing about why it did not trust the other one.
The change
TrustStoreBackendholds one lock over every access to the file and to its certificate manager: reads, adds, deletes, the instance certificate it generates and the change of its configuration. The key managers and keys it hands out are opened under that lock as well, with the PIN of the file they were read from rather than that of a configuration changed meanwhile.Platformwrites every key store to a file of its own next to it and moves that over the old one at once (ATOMIC_MOVE): a reader outside the lock never finds the file half written, and a server stopped part way through a write keeps the old file whole instead of an empty one it cannot start with. The new file takes the owner, the group and then the mode of the old one (not its ACLs), so that it is never open to a group the old one was not; a key store path which is a link goes on pointing where it did. The key store is written in place, as before, where replacing the file would change what it is or cannot be done: a new key store, one this process may not write to (refused, as before), one with other hard links, one whose owner or group the new file cannot take, one whose links, owner or group cannot be told, one in a directory this process cannot write to, and one which cannot be replaced (a file mounted on its own into a container). On a file system without POSIX attributes (Windows) the key store is always written in place, as before: neither the owner, the ACLs nor the hard links of the old file would reach a new one there.CryptoManagerSyncreports a certificate it cannot copy into the trust store, and one it cannot drop from it (a compromised key the server would go on trusting), at error level:INFO_TRUSTSTORESYNC_ADD_FAILED_655andINFO_TRUSTSTORESYNC_DELETE_FAILED_656becomeERR_, same ordinals. Nothing is reported where nothing failed: a certificate the trust store holds already (dsreplication seeds the trust store of the servers it enables) and one it holds no longer (it leaves when its key is marked compromised).TrustStoreBackendkeeps the result code of those two refusals, an alias in use (entry already exists) and an alias not in use (no such entry), instead of turning them into a server error, whichCryptoManagerSyncwould have reported.Not fixed here
#1188 also found an instance key stored in
cn=admin dataon the directory server withoutds-cfg-public-key-certificate, which no trust store can take. I ran the scenario of the issue (the #534 topology of #1184, a replication server and a directory server set up again on empty volumes while the first directory server is down) three times on an image built from this branch, with temporary logging of everyAddMsgof an instance key: built, decoded on the replication and the directory servers, replayed, and what each add left incn=admin data. It did not reproduce: every message carried the certificate (745 bytes), the encoder threw nothing, all four servers held every key with its certificate incn=admin dataand in the trust store, before and after a restart of the directory server. The cause of the entry without a certificate is still open.One thing the runs did show: the directory server that was down fails three handshakes with the new replication server, one per domain, in the second it starts, and none after. That is before it has learned the new certificate through the other replication server; I have not verified that further.
Tests
CryptoManagerTestCase:CertificateManagerTestCase:RED on master for the concurrency, error-log and result-code tests, and for the half-written file. Each of the changes reverted on its own turns its test red (22 mutants over the four rounds).
CryptoManagerTestCase,CertificateManagerTestCase,AdsTrustStoreProvisionerTestandAdsTrustStoreInstallTestCase: 138 tests, all green. Without a test: the PIN taken under the lock and the configuration change under it (the race with a change of the configuration cannot be made to happen reliably in one), the check of the owner the new file took (a process which is not root cannot make a file whose owner it cannot take), the order of the group and the mode (the window between them cannot be seen from a test), the in-place road where the links of the file cannot be told, and the in-place road on Windows (no Windows run).