[
https://issues.apache.org/jira/browse/HDDS-16955?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16955:
------------------------------
Attachment: TestBugMC1SubmitTimeoutDropsKey.java
> 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
> Priority: Minor
> 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]
