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: [email protected]
For additional commands, e-mail: [email protected]
