[
https://issues.apache.org/jira/browse/HDDS-16914?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16914:
------------------------------
Attachment: TestBugMC1DuplicatePrepareAck.java
> SCM leader counts root CA rotation prepare acks per certificate, so one SCM
> acking twice lets it commit before every SCM prepared
> ---------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-16914
> URL: https://issues.apache.org/jira/browse/HDDS-16914
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Major
> Attachments: MC-1-count-root-ca-rotation-prepare-acks-per-scm.patch,
> TestBugMC1DuplicatePrepareAck.java
>
>
> h3. Mechanism
> {{RootCARotationHandlerImpl.rotationPrepareAck}} receives the new root
> certificate ID, the sender's new sub-CA certificate ID and the sender's SCM
> ID. It adds the certificate ID to a set and uses the SCM ID only in a log
> line. {{RootCARotationManager.WaitSubCARotationPrepareAckTask}} submits
> {{rotationCommit}} as soon as the size of that set equals the number of peers
> in the Raft configuration.
> An SCM that applies {{rotationPrepare}} a second time runs
> {{SubCARotationPrepareTask}} again, which deletes its {{scm/sub-ca-next}}
> directory, has a new sub-CA certificate signed with a new serial and sends
> another ack. {{shouldSkipRootCert}} does not stop the second apply, because
> the root certificate in memory only changes at {{rotationCommitted}}. An SCM
> records a request as applied only when its transaction buffer is flushed
> ({{ozone.scm.ha.dbtransactionbuffer.flush.interval}}, default 60s), so an SCM
> that is killed before the flush that follows {{rotationPrepare}} applies the
> request again when it starts. An SCM that is killed in that interval after
> its ack and is started again within {{hdds.x509.ca.rotation.ack.timeout}}
> (default 15 minutes) therefore acks twice. The leader of a three SCM ring
> then holds three certificate IDs from two SCMs and commits although the third
> SCM has not prepared.
> h3. Trigger
> Secure SCM HA with {{hdds.x509.ca.rotation.enabled=true}} (off by default),
> three SCMs.
> # One follower is down when the leader starts the rotation, so it does not
> ack.
> # The other follower acks, is killed before its next transaction buffer flush
> (default interval 60s) and is started again before the ack timeout (default
> 15 minutes). It applies {{rotationPrepare}} again and acks with a new
> certificate.
> # The leader counts three acks and submits {{rotationCommit}} and
> {{rotationCommitted}}.
> Two overlapping faults are needed. Without the kill the same cluster behaves
> as designed: the leader gets two acks, logs "Failed to receive all acks of
> rotation prepare" at the timeout and does not commit.
> h3. Impact
> The rotation is committed while one SCM has no prepared sub-CA. On an
> {{ozonesecure-ha}} compose cluster built from unmodified source (two runs)
> that SCM, once started, applied {{rotationPrepare}} and {{rotationCommit}}
> within a few milliseconds, before its prepare task had created
> {{scm/sub-ca-next}}: it moved {{scm/sub-ca}} to {{scm/sub-ca-previous}},
> failed the second move with {{NoSuchFileException}}, and after a restart
> failed to start because {{scm/sub-ca/certs}} was missing. The other two SCMs
> kept quorum and the leader finished the rotation.
> h3. Reproduction
> PASS, deterministic, unmodified source, about 4 seconds.
> [^TestBugMC1DuplicatePrepareAck.java] ({{hadoop-hdds/server-scm}}) runs one
> SCM's rotation code in the test JVM: the real {{RootCARotationManager}} with
> its tasks, {{RootCARotationHandlerImpl}}, {{SCMCertificateClient}} and the
> certificate directories on disk. {{StorageContainerManager}} is a mock, the
> Raft configuration is a mock that reports three peers, and a replicated
> request is applied by calling the handler method the generated invoker calls.
> The first test rebuilds the SCM objects from disk as after a restart, applies
> {{rotationPrepare}} again and shows a second ack with the same SCM ID and a
> different certificate ID. The second test lets the leader ack once, delivers
> two acks from one follower and none from the third SCM, and shows the leader
> logging "rotation is finished successfully" with the new root certificate in
> memory. A passing test means the defect is present.
> The kill and the Raft log replay are not part of this test. They were run on
> the compose cluster described under Impact.
> h3. Patch
> [^MC-1-count-root-ca-rotation-prepare-acks-per-scm.patch], against
> b9b33851b20fa11baf4b07982de9afb8ea44f85a. It also applies to master at
> 5108acff480.
> The handler counts the SCM IDs of the senders instead of the certificate IDs.
> The ack already carries the SCM ID, the tally only lives in the leader's
> memory, and nothing changes in the replicated request, the generated invoker
> or what is persisted. Counting per SCM means that every SCM has acked once,
> not that every SCM is prepared at the moment of the commit: an SCM that is
> killed after its ack and prepares again when it starts is still counted by
> its first ack, which is a separate defect that this patch does not change
> (HDDS-16913).
> Covered by {{testPrepareAckCountedOncePerScm}} in the existing
> {{TestRootCARotationManager}}, which fails without the change (expected 2
> acks, was 3). With it {{TestRootCARotationManager}},
> {{TestScmInvokerCodeGenerator}} and {{TestScmCodecFactoryReplicateCoverage}}
> (16 tests) pass and checkstyle is clean. With the patch the first test of the
> reproduction still passes, because a second certificate is still signed, and
> the second fails because the leader ends the rotation with "Failed to receive
> all acks of rotation prepare ... received 2 acks". The patched build was not
> run on the compose cluster.
> A stricter condition would be that the IDs of the acking SCMs equal the peer
> IDs of the current Raft configuration (an SCM's Raft peer ID is its SCM ID).
> It only differs when an SCM outside the configuration acks, and it needs a
> change of the handler interface and its generated invoker, so the patch does
> not do it.
> Found by TLA+ model checking and code review of the SCM root CA rotation
> paths under HDDS-15926, on commit b9b33851b20fa11baf4b07982de9afb8ea44f85a.
> 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]
