Siyao Meng created HDDS-16900:
---------------------------------
Summary: 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
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]
