[jira] [Created] (HDDS-16956) SCM secret key file is rewritten in place without sync: a crash can leave it empty or cut, and a new leader then drops the keys or cannot initialize

["Siyao Meng (Jira)" <ji...@apache.org>]

Siyao Meng created HDDS-16956:
---------------------------------

             Summary: SCM secret key file is rewritten in place without sync: a crash can leave it empty or cut, and a new leader then drops the keys or cannot initialize
                 Key: HDDS-16956
                 URL: https://issues.apache.org/jira/browse/HDDS-16956
             Project: Apache Ozone
          Issue Type: Bug
            Reporter: Siyao Meng
         Attachments: MC-2-write-secret-key-file-atomically-and-durably.patch, TestBugMC2SecretKeyFileNotCrashSafe.java

h3. Mechanism
{{LocalSecretKeyStore.save}} truncates the secret key file and rewrites it in place, with one write per key and no sync. SCM keeps its symmetric secret keys only in memory and in this file, while the applied index is persisted separately in scm.db. A crash can therefore leave the file empty or cut while scm.db already records an index at or past the key update.

After a restart Ratis replays only the entries above the index in scm.db. {{LocalSecretKeyStore.load}} has a single caller, {{SecretKeyManager.checkAndInitialize}}, which only runs on a leader. So the restarted SCM is caught up but has no keys in memory until the next key update is applied to it ({{hdds.secret.key.rotate.duration}}, default 1d). If it becomes leader before that, {{checkAndInitialize}} trusts the file:
* *Empty file.* {{load}} returns an empty list, as for a missing file. The leader generates one new key and replicates it through {{SecretKeyState.updateKeys}} as the complete key set. Every SCM replaces its keys with it and rewrites its own file, so the committed, unexpired keys are gone from the whole cluster.
* *Cut file.* {{load}} throws. Every initialization attempt fails, {{SecretKeyManagerService}} logs "Error occurred when updating SecretKeys." at each rotation check, and the leader has no current key for as long as it leads.

h3. Trigger
The key file must end up empty or cut on an SCM whose scm.db is at or past the key update, and that SCM must become leader (election or {{ozone admin scm transfer}}) before the next key update reaches it. Two ways to get such a file:
# *Power loss or kernel crash after the unsynced save.* Shown with a device image probe (see Reproduction), and only for the first save of the key file on XFS: the image held the index and a 0 byte key file at some point within about a minute after the save. It was not seen for later saves (rotations) or on ext4, and no image held a cut file. By the code, the first save on an SCM is its first applied key update or its first snapshot install (new cluster, newly added SCM).
# *Process kill or write error inside the save, on a path that is not replayed.* A SIGKILL during {{save}} leaves empty and cut files (attached test). On the apply path this heals, because the entry is applied again after the restart. It does not heal in a snapshot install, where {{SCMStateMachine.reinitialize}} replaces scm.db before it saves the keys (HDDS-16923 describes that window with a missing or stale file). The kill inside a snapshot install was not run here.

h3. Impact
Run on a secure mini cluster with 3 SCMs, 1 OM and 1 datanode, default rotation and expiry:
* *Empty file.* After the leadership transfer all three SCMs hold a single new key and {{SecretKeyProtocol.getSecretKey}} returns null for the committed key, which would not expire for 9 more days. OM still signs with it, and the datanode side check ({{BlockTokenVerifier}}) rejects a block token that OM issued after the event: "Can't find the signing secret key". How long that lasts was not measured (same code paths as in HDDS-16923).
* *Cut file.* The new leader logs one ERROR per rotation check and {{SCMSecurityProtocolServer.getCurrentSecretKey}} throws "Secret key initialization is not finished yet." (called on the server object, not over RPC). The other SCMs still hold the key in memory, and after leadership was transferred to one of them the secret key RPC returned it again.

Both fail closed: no token is accepted that should be rejected.

h3. Reproduction
[^TestBugMC2SecretKeyFileNotCrashSafe.java] (path and Maven command in the class comment) passes on unmodified source, which means the defect is present. Its first test kills a child JVM that runs {{save}} in a loop with 10 keys: of 40 kills, 8 left an empty file and 4 a cut one (this shows that both states exist, not how often they occur). The two cluster tests replace the key file of a stopped follower by an empty or a cut one, restart it, assert that it catches up with no keys in memory (this part is not injected), transfer leadership to it and check the outcomes above.

The power loss route was checked with a device image probe (not attached) in a Linux VM: kernel 7.0.14, XFS and ext4 on a loop device, default mount options, {{dirty_writeback_centisecs}} 1500 (the kernel default is 500, so the window is longer here). It runs {{save}}, then an unsynced RocksDB batch that stands for the applied index, and copies the device image while the writer is alive. On XFS, for the first save, the image held the index and a 0 byte key file from right after a WAL sync (as in a DB checkpoint) to the last image at 46 seconds, and without a sync (WAL dirty for 25 seconds before the save) at about 51 and 65 seconds. Each scenario ran once and no image was fed to a cluster.

h3. Patch
[^MC-2-write-secret-key-file-atomically-and-durably.patch], against 025266b52a6dcaf39508607cad736799c40907e8. It also applies to master at a65a32e7c4b.

{{LocalSecretKeyStore.save}} now writes the keys to a new temporary file next to the key file, syncs it, renames it over the key file and syncs the directory, using the Ratis {{AtomicFileOutputStream}} as {{YamlUtils.dump}} does. A failed write aborts the stream, so an incomplete file is never renamed. The temporary file is created owner only, as in {{KeyStorage.storeKey}}, and a leftover one is deleted, not reused. The content is byte for byte what the old code wrote, so the format is unchanged for existing key files and for a downgrade. The file is replaced on each save: a symlink at the key path would become a regular file, and hard links, ACLs or a non default owner are not carried over.

The cost is two syncs per key update on the Ratis apply thread, inside the key state write lock (once per scheduled or forced rotation and per snapshot install). A save that fails, now including a failed sync of the file, stops the SCM as a failed write did before, and leaves the previous file in place.

{{TestLocalKeyStore}} gets three cases that fail without the change: the file is replaced and not rewritten in place, a failed write keeps the previous keys, and a leftover temporary file is not reused. With it the symmetric key unit suites, {{TestSecretKeySnapshot}} and {{TestSecretKeysApi}} pass and checkstyle is clean. On the test machine {{TestSecretKeysApi}} often failed in cluster startup with the race in HDDS-12174, with and without the patch. With the patch no kill in the reproduction leaves a damaged file.

The patch only stops new damage. What a leader does with a missing, empty or unreadable key file is unchanged (the two cluster tests still pass with it): a file that is already empty or cut from an earlier crash is not repaired, a file lost or damaged by other means still leads to the outcomes above, and the snapshot install ordering of HDDS-16923 still leaves a stale or missing file, with the first outcome above. Whether a leader without keys should ask its peers, as a snapshot install does with {{getSecretKeysFromLeader}}, or load its file at startup so that a problem shows before it leads, is a larger change and is left open here. This patch and the one attached to HDDS-16923 touch different files and apply together.

Found by TLA+ model checking and code review of the SCM secret key rotation and distribution paths under HDDS-15926, on commit 025266b52a6dcaf39508607cad736799c40907e8. Checked against HDDS issues and apache/ozone pull requests for duplicates before filing. Related but not a duplicate: HDDS-16923 (open) covers the snapshot install ordering and names the in place, unsynced save as the gap its patch leaves open. The attached patch is a proposal for review. Generated with Specula (Claude Opus 5.5).



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org