[
https://issues.apache.org/jira/browse/HDDS-16956?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16956:
------------------------------
Attachment: TestBugMC2SecretKeyFileNotCrashSafe.java
> 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
> Priority: Major
> 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: [email protected]
For additional commands, e-mail: [email protected]
