[
https://issues.apache.org/jira/browse/HDDS-16900?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16900:
------------------------------
Attachment: TestBugMC2CertificateIdDivergence.java
> Primary SCM restart can lower the replicated CertificateId row, so
> certificate IDs are returned twice
> -----------------------------------------------------------------------------------------------------
>
> Key: HDDS-16900
> URL: https://issues.apache.org/jira/browse/HDDS-16900
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Major
> Attachments:
> MC-1-do-not-lower-certificate-id-on-primary-scm-start.patch,
> TestBugMC1CertificateIdReseed.java, TestBugMC2CertificateIdDivergence.java
>
>
> h3. Mechanism
> On every start of the primary SCM with security enabled,
> {{StorageContainerManager.persistPrimarySCMCerts}} calls
> {{SequenceIdGenerator.upgradeToCertificateSequenceId(scmMetadataStore,
> true)}}. With {{force}} set, that method writes the largest serial found in
> {{validSCMCertsTable}} and {{validCertsTable}} (at least 2) into the
> {{CertificateId}} row of {{sequenceIdTable}}. It does not compare the new
> value with the existing row (the row is only tested for absence, and with
> {{force}} the result of that test is ignored), and it writes to the local
> RocksDB directly, although once the row exists it is otherwise changed only
> through Ratis ({{StateManagerImpl.allocateBatch}}).
> The row can be above the largest stored serial.
> {{SCMSecurityProtocolServer.getEncodedCertToString}} allocates the ID in one
> Ratis transaction before {{DefaultCAServer.signAndStoreCertificate}} signs
> the certificate and stores it in a second one. If signing or storing fails,
> the ID stays allocated and no certificate carries it. The row is also above
> the largest stored serial after the certificate with that serial has expired
> and been removed. On the next start of the primary the row is then set back,
> the generator loads the lower value and returns IDs it has already returned.
> The other SCMs keep the replicated value, so the SCMs now disagree on a
> replicated row. {{allocateBatch}} is a compare and set on the previous value,
> so from then on an allocation changes the row on some SCMs and is a no-op on
> the others, and every SCM that installs a checkpoint from the primary takes
> over the lowered row.
> The forced call came with HDDS-9138. It is present in every release tag from
> ozone-1.4.0 to ozone-2.2.1 (read from the tagged source, not run on a
> release).
> h3. Trigger
> # The most recently allocated IDs have no stored certificate when the primary
> stops, so the row is above the largest stored serial: signing or the store
> transaction failed for the last request, or the certificate with the largest
> serial expired and was removed.
> # The row is durable on the primary. A clean stop is enough: Ratis takes a
> snapshot when the server stops, and {{SCMStateMachine.takeSnapshot}} flushes
> the SCM transaction buffer.
> # The primary SCM is restarted.
> No setting has to be changed for this. If the SCM is killed before the
> allocation was flushed, the allocation is replayed from the Raft log after
> the forced write and the row is not lowered. The buffer is also flushed every
> 60 seconds on a leader that is out of safe mode, when an SCM becomes leader
> and at every Ratis snapshot, so a kill after one of those leaves the same
> state as a clean stop (both read from the code, not run).
> h3. Impact
> Reproduced on unmodified source unless marked otherwise.
> * *An ID is returned twice (one SCM).* With IDs allocated up to 4 and 3 as
> the largest stored serial, the row is 3 after a clean restart and the next
> certificate gets serial 4. [^TestBugMC1CertificateIdReseed.java]
> * *Two certificates with the same serial (one SCM).* After the certificate
> with serial 3 expired and was removed, the row is 2 after the restart and the
> same CA issues a second certificate with serial 3 for another subject and
> key. This needs the certificate with the largest serial to be expired and
> removed. With the default lifetime of 365 days, renewal 28 days before
> expiry, and removal only by {{ozone admin cert clean}} or by the root CA
> rotation manager (off by default), that is not expected on a live cluster.
> The test uses a lifetime of 3 seconds. [^TestBugMC1CertificateIdReseed.java]
> * *SCMs diverge and valid requests are refused (three SCMs, default
> settings).* IDs were allocated up to 9 with 7 as the largest stored serial.
> After its restart the primary has row 7 and the other two keep 9. Another SCM
> leads and issues serials 10 and 11. On the primary the two allocations log
> "Failed to allocate a batch for CertificateId, expected lastId is 9, actual
> lastId is 7." and "expected lastId is 10, actual lastId is 7." and leave the
> row at 7. When the primary leads next, five valid certificate requests in a
> row end as: issued 8, issued 9, refused with "Conflicting certificate ID10",
> refused with "Conflicting certificate ID11", issued 12. One request is
> refused for every certificate the other leader issued in the meantime, so the
> number of refusals to come grows for as long as an SCM with the higher value
> leads. The allocation of 12 is the first that all three SCMs apply again
> (read from the log of the run), so from there the rows are equal. A restart
> of the lowered primary also raises its row, to the largest serial stored by
> then, because the forced write runs again, on unpatched code too (read from
> the code, not run). [^TestBugMC2CertificateIdDivergence.java]
> * *A new SCM starts with the lowered row (default settings).* A third SCM
> that joins while the restarted primary leads installs the primary's
> checkpoint. Its row is 7 while the remaining SCM has 9, and when it leads it
> issues serial 8. [^TestBugMC2CertificateIdDivergence.java]
> * *An existing follower is lowered by a checkpoint (log purge enabled).* With
> {{ozone.scm.ha.ratis.log.purge.enabled}} set and a small purge gap and
> snapshot threshold, a follower that held row 9 durably and was down while the
> primary restarted installs the primary's checkpoint. All three SCMs then
> report the same applied index with rows 7, 7 and 9, and the follower issues
> serial 8 when it leads. With log purge off (the default) an existing follower
> catches up from the log and keeps its row (read from the Ratis code, not
> run). [^TestBugMC2CertificateIdDivergence.java]
> In the multi SCM runs the IDs that were returned a second time had no stored
> certificate, so no two stored certificates shared a serial.
> {{SCMCertStore.checkValidCertID}} prevents that, and the same check is what
> refuses the requests.
> h3. Reproduction
> The two attached classes are self contained and pass on unmodified source
> when the bug is present (target path and Maven command are in each class
> comment). Both go into {{hadoop-ozone/integration-test}} and use MiniKdc. No
> state is injected and no source is changed.
> {{TestBugMC1CertificateIdReseed}} runs one secure SCM with default flush and
> snapshot settings, requests certificates from the handler of
> {{SCMSecurityProtocolServer}} (called in process) and stops and starts the
> SCM. The second test shortens the certificate lifetime to 3 seconds.
> {{TestBugMC2CertificateIdDivergence}} runs a secure SCM HA mini cluster with
> three SCMs per test and uses certificate requests, SCM stop and start, the
> leadership transfer admin call and the checkpoint install that Ratis requests
> by itself. The first and the last test use default Ratis settings. The second
> enables log purge with purge gap 5 and snapshot threshold 5.
> In both classes the ID without a certificate is produced by the generator
> call that {{getEncodedCertToString}} makes
> ({{getNextId(SequenceIdType.CertificateId)}}), standing in for a request that
> fails after the ID was taken. A failed request itself is not part of the
> attached tests.
> h3. Patch
> [^MC-1-do-not-lower-certificate-id-on-primary-scm-start.patch], against
> b0aa6475b7856d2c7f467dd3db12b9a1dd116f63. It also applies to master at
> f650a22f3e3.
> {{upgradeToCertificateSequenceId}} now reads the existing row once and writes
> only when there is no row or the largest stored serial is above it. The
> forced call from {{persistPrimarySCMCerts}} stays and can still raise the row
> over a certificate that was just stored, it can no longer lower it. On a
> cluster initialised with the current code the forced call was already a no-op
> at first start (the log shows "upgrade CertificateId to 2" twice), so nothing
> changes for clusters the bug does not affect. Checkpoint install needs no
> change: once the primary keeps its row, the checkpoint it serves carries the
> replicated value.
> The patch keeps the local write to a replicated row, now raise only. Removing
> the forced call (the first seeding of an absent row would stay local) or
> sending it through Ratis is the stricter alternative. The patch also does not
> repair rows that already differ. They converge as described under Impact.
> Covered by {{testForcedCertificateIdUpgradeKeepsAllocatedIds}} in the
> existing {{TestSequenceIDGenerator}}, which fails without the change
> ("expected: 4L but was: 3L"). With the patch {{TestSequenceIDGenerator}} and
> seven surrounding SCM unit suites (68 tests),
> {{TestSCMInstallSnapshotWithHA}} (3 tests) and five cases of
> {{TestSecureOzoneCluster}} pass, and checkstyle is clean. The other four
> cases of {{TestSecureOzoneCluster}} and {{TestSecretKeySnapshot}} time out in
> this environment with and without the patch (Kerberos RPC connect timeouts)
> and were left to CI. With the patch the scenarios of the attached classes
> keep the row (4 on the single SCM, 9 on every SCM) and no request is refused,
> so the attached classes, which assert the faulty values, fail at their first
> assertion on the row.
> Found by TLA+ model checking and code review of the SCM sequence ID generator
> and its startup and checkpoint paths under HDDS-15926, on commit
> b0aa6475b7856d2c7f467dd3db12b9a1dd116f63. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. Related but not
> duplicates: HDDS-9138 (introduced the forced call), HDDS-16035 (a secure SCM
> HA cluster cannot start because the primary refuses to sign certificates for
> the other SCMs without a leader) and HDDS-14337 (seeding of the local ID in
> the same class). 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]
