Siyao Meng created HDDS-16955:
---------------------------------
Summary: SCM secret key rotation proposed after a Ratis submit
timeout drops the key of the timed out rotation on all SCMs
Key: HDDS-16955
URL: https://issues.apache.org/jira/browse/HDDS-16955
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments:
MC-1-propose-pending-secret-key-again-after-failed-update.patch,
TestBugMC1SubmitTimeoutDropsKey.java
h3. Mechanism
{{SecretKeyManager.checkAndRotate}} proposes the whole key list: the keys of
the applied state plus one newly generated key. {{checkAndInitialize}} proposes
the keys of the local key file, or one newly generated key if the file has no
valid key. {{SecretKeyStateImpl.updateKeys}} replaces the list when the request
is applied. {{SCMRatisServerImpl.submitRequest}} waits
{{ozone.scm.ha.ratis.request.timeout}} (default 30s) for the reply and then
gives up without cancelling the request, so the request stays in the Raft log
and is applied later if the leader keeps its term. The callers treat the
resulting {{SCMException(TIMEOUT)}} as a failure:
{{SecretKeyManagerService.run}} logs it and tries again at the next check,
{{SCMSecurityProtocolServer.checkAndRotate}} returns it to the caller. The next
attempt still reads the old applied state, generates another key and proposes a
list without the first new key. Both requests are then applied in log order:
the first new key is the current key for a moment and is then removed from
every SCM and from its key file, although it has not expired.
HDDS-16142 fixed the same assumption (a submit that timed out did not happen)
for the deleted block transaction summary.
h3. Trigger
# The SCM leader keeps its term but cannot commit or apply a request within the
request timeout.
# A rotation is proposed and its submit times out. The request stays in the
Raft log.
# A second rotation is proposed before the first one is applied. Either {{ozone
admin scm rotate --force}} is repeated after the timeout error, or the rotation
checker reaches its next check. The check interval
{{hdds.secret.key.rotate.check.duration}} is 10m by default, so on the checker
path the stall has to last that long.
# The stall ends and both requests are applied.
A stall in which the leader loses its term does not trigger this. With the
default 5s leader election timeout a plain follower outage ends in
{{NotLeaderException}} first, and a new leader accepts requests only after it
has applied every earlier entry.
Run with default timeouts: every follower the quorum depends on (both followers
of a three SCM group, or the only live one) keeps answering heartbeats but does
not acknowledge log entries (see Reproduction). The leader kept its term and
the forced rotation timed out after 30s. With only one of two followers
affected nothing stalls. Argued, not run: this is how a follower whose Raft log
write hangs looks to the leader (in Ratis 3.3.1 the reply to a heartbeat does
not wait for the log write), and a leader whose {{StateMachineUpdater}} thread
is blocked for 30s gives the same timeout.
The first key of a cluster has the same problem ({{checkAndInitialize}} with no
saved key). That case is covered by a unit test only.
h3. Impact
* A token signed with the removed key cannot be verified. SCM answers
{{getSecretKey}} with null and the verifier throws "Can't find the signing
secret key". Nothing restores the key. No invalid token is accepted.
* The key is the current key only between the two applies, 28 ms in the
reproduction run (longer if other requests sit between the two in the log).
Signers in that window: SCM itself signs container tokens with the applied
current key ({{ContainerTokenSecretManager}}, with
{{hdds.container.token.enabled}}), for example for close container commands,
which are sent again later. OM and datanodes fetch the current key at start and
then once per check interval after their key has passed the rotation period
({{DefaultSecretKeySignerClient}}). If such a fetch falls into the window, that
process signs with the removed key until its key passes the rotation period
(default 1d), a restart or {{ozone admin om fetch-key}}.
* No real signer was seen to pick up the removed key. In the reproduction the
rejected token is signed by the test with the key taken from the Raft log.
h3. Reproduction
PASS on unmodified source with default Ratis timeouts.
[^TestBugMC1SubmitTimeoutDropsKey.java] ({{hadoop-ozone/integration-test}})
starts a secure mini cluster with three SCMs. The Ratis test hook on
{{RaftServerImpl.appendEntries}} makes both followers hold every call that
carries entries while heartbeats pass. A forced rotation over the SCM secret
key protocol fails with {{TimeoutException}} after 30s, with the leader still
leader in the same term and the key list unchanged. The call is repeated and
the followers are released once the second request is in the leader's log. The
test then reads both {{updateKeys}} requests from the Raft log and shows that
the new key of the first one is missing on all three SCMs and from
{{getSecretKey}} while unexpired, and that the datanode block token verifier
rejects a token signed with it and accepts one signed with the surviving key.
About 70s including cluster start. A passing test means the defect is present.
The rotation checker calls the same method without the force flag. That path is
covered by the unit test in the patch, not run on a cluster.
h3. Suggested fix
The patch changes only the proposing side. The alternative is to change what
{{updateKeys}} does on apply (merge into the current list, or carry the key the
proposal was based on and reject a stale one). That changes how a replicated
request is applied, so SCMs of two versions applying the same log would end
with different key lists unless it is gated by a layout feature. It is not
needed for this defect.
h3. Patch
[^MC-1-propose-pending-secret-key-again-after-failed-update.patch], against
025266b52a6dcaf39508607cad736799c40907e8. It also applies to master at
a65a32e7c4b.
{{SecretKeyManager}} remembers the new key of an update that failed. While that
key is not in the applied key list, the next rotation proposes it again, on top
of the then applied state, instead of generating another key. If the first
request is applied after the second one was proposed, the second one carries
the same key and removes nothing. If the first request was lost, the second one
does the rotation. Once the first request has been applied, its key is in the
applied list and the next rotation generates a new key, as it does today. This
is decided by key id, not by creation time: an applied key has been through the
Raft log, which stores its times in milliseconds, while the proposer's copy has
the precision of the JVM clock. A rotation proposal never contains a key id
twice, also when the first request is applied while the next proposal is being
built ({{SecretKeyStateImpl.updateKeys}} cannot apply such a list). At
initialization ({{checkAndInitialize}} with no saved key) the remembered key is
proposed again unless it has expired: the applied list is empty there, so there
is nothing to compare it with, and proposing the same single key again is
harmless if the first request is applied in the meantime. The remembered key is
forgotten when an update succeeds. It is replaced by a new key when it has
expired, or when it would no longer become the current key because a newer key
has been applied. This also holds across a leader change: only this SCM can
have an unapplied proposal of its own in the log, because Ratis accepts
requests only from a leader that has applied all earlier entries.
Nothing changes as long as no update fails. After a failed update that has not
been applied, the next rotation on that SCM uses the key that was generated at
the failed attempt, so a forced rotation that timed out and is repeated before
the first request is applied rotates once, not twice. The log then says that
the key of the last failed update is proposed again. After a failed update that
has been applied, the next rotation behaves as on unmodified code.
Tests are in the existing {{TestSecretKeyManager}}. Proposals are applied as a
state machine gets them, after {{toProtobuf}} and {{fromProtobuf}}.
{{testRotateAgainAfterTimeout}} (scheduled and forced) and
{{testInitializeAgainAfterTimeout}} fail without the change because the key of
the first update is missing from the final list.
{{testRotateAfterTimedOutUpdateIsApplied}} (timed out, applied after all,
rotated again), {{testRotateWhileTimedOutUpdateIsApplied}} (applied between the
two reads of the applied state), {{testRotateAfterTimeoutAndNewerKeyApplied}}
and {{testExpiredKeyIsNotProposedAgain}} pass with and without the change. They
guard the cases in which the key must not be proposed again.
With the change the secret key unit suites of {{hdds-server-framework}}
({{TestSecretKeyManager}}, {{TestLocalKeyStore}}, {{TestManagedSecretKey}}, 24
tests) and the mini cluster suites {{TestSecretKeysApi}},
{{TestSecretKeySnapshot}} and {{TestBlockTokensCLI}} (8 tests) pass, checkstyle
is clean. Also run with the change on a three SCM mini cluster: a forced
rotation times out, the stall ends and the request is applied, then the
rotation is forced again. That rotation succeeded with a new key, and all three
SCMs stayed up with the same three keys. With the change the reproduction fails
at the assertion that the key is gone: the forced rotation still times out, the
repeated call proposes the same key, and the key list ends with that key and
the old one.
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. 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]
