Siyao Meng created HDDS-17214:
---------------------------------
Summary: With an S3 secret store without batch support, a revoke
can delete a newer secret and a store failure at flush time terminates the OM
Key: HDDS-17214
URL: https://issues.apache.org/jira/browse/HDDS-17214
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments:
MC-5-update-non-batch-s3-secret-store-at-apply-time.patch,
TestBugMC5NonBatchRevokeThenGetSecret.java
h3. Mechanism
For an S3 secret store whose {{S3SecretStore.batcher}} returns null, such as
{{VaultS3SecretStore}}, the OM updates the store at two different points. Since
HDDS-10469, {{S3GetSecretRequest.validateAndUpdateCache}} writes a new secret
to the store when the entry is applied. Revoke, set and the tenant access ID
requests still update the store when the double buffer flushes the entry:
{{S3RevokeSecretResponse.addToDBBatch}} calls
{{S3SecretManagerImpl.revokeSecret}}, and {{OMSetSecretResponse}},
{{OMTenantAssignUserAccessIdResponse}} and
{{OMTenantRevokeUserAccessIdResponse}} do the same for their updates.
{{OzoneManagerStateMachine.applyTransaction}} answers the client after apply
and does not wait for the flush, and the store delete is unconditional.
This has two consequences.
* *A revoke deletes a newer secret.* A revoke at index n only marks the cache
entry as deleted at apply time. A getsecret for the same access ID at index
n+1, applied before n is flushed, sees the deleted marker, writes a new secret
to the store and returns it to the client. The flush of n then deletes that
secret from the store and marks the new cache entry as deleted. After the cache
entry is cleaned up, {{S3SecretManagerImpl.getSecretString}} throws
S3_SECRET_NOT_FOUND for the secret the client holds.
* *A store failure at flush time terminates the OM.* If the store call in
{{addToDBBatch}} fails, the request has already been answered as successful,
and {{OzoneManagerDoubleBuffer}} terminates the OM before the batch and the
transaction info are committed. The transaction info does not advance, so each
restart replays the entry, calls the store again and terminates again until the
call succeeds.
Affected versions: the lost secret since 2.0.0 (HDDS-10469 moved only the
getsecret write to apply time); the flush time termination since the external
store was added in 1.4.0 (HDDS-7814).
h3. Trigger
An external store without batch support (Vault, configured through
{{ozone.secret.s3.store.provider}}).
* Lost secret: a revoke and a getsecret for the same access ID applied by one
OM in the same flush window. On the leader that needs the two requests to
follow each other closely. In OM HA every OM applies the log and updates the
shared store itself, so a follower that applies both entries before flushing
the revoke (for example while catching up) deletes the secret the leader
issued, however far apart the client sent them. The follower case is argued
from the code; the attached reproduction runs one OM.
* OM termination: the store call fails (store unreachable, or rejecting writes)
while a revoke, set, tenant access ID assign or tenant access ID revoke is
flushed.
h3. Impact
* The secret returned by getsecret is not in the store, so S3 requests signed
with it fail with S3_SECRET_NOT_FOUND, also after an OM restart. Nothing
reports the loss; the user has to call getsecret again and reconfigure with the
new secret. Fail closed, no access is gained.
* A store failure at flush time stops the OM for all clients, not only S3,
after the request was answered as successful, and restarts fail until the store
call succeeds.
h3. Reproduction
[^TestBugMC5NonBatchRevokeThenGetSecret.java] ({{hadoop-ozone/ozone-manager}}):
real {{preExecute}}, {{validateAndUpdateCache}} and a running double buffer on
one OM, with an in memory store without batch support, not Vault. The only
timing aid is the existing {{OzoneManagerDoubleBuffer.pause}} / {{unpause}}
hook, so the run shows that the window exists, not how often real clients hit
it. After a revoke and a getsecret in one flush the store is empty: the
assertion on the store fails with {{Expecting actual not to be null}}. It
passes with the patch.
The flush time termination is argued from the code above and was not reproduced
in an attachment.
h3. Patch
[^MC-5-update-non-batch-s3-secret-store-at-apply-time.patch], against
df04bc0e309f625a7dcdcd60790a42f34e2743d9. It also applies to master at
2cb1b6dd4e9.
For a store without batch support, {{S3RevokeSecretRequest}},
{{OMSetSecretRequest}}, {{OMTenantAssignUserAccessIdRequest}} and
{{OMTenantRevokeUserAccessIdRequest}} now update the store in
{{validateAndUpdateCache}}, as {{S3GetSecretRequest}} already does, and their
responses no longer touch such a store. Revoke and set do this under the
existing per access ID lock, tenant revoke through
{{S3SecretLockedManager.revokeSecret}}. Apply is single threaded and in log
order, so on one OM every store update follows the log order and a revoke can
no longer delete a secret written by a later entry. With a shared store, a
lagging OM still removes such a secret until it applies the later entry; that
belongs to the design issue filed alongside. All four requests move together;
moving only the revoke would let a flush time set write a secret after a later
revoke deleted it. The default RocksDB store supports batches and is unchanged.
Because the store now holds the secret as soon as a tenant assign is applied,
the assign treats a store entry holding its own secret as already applied. The
secret is generated in {{preExecute}} and carried in the log entry, so it
identifies the entry. Without this, a replay after a restart, or a follower
applying the entry after the leader, would reject the assign with
TENANT_USER_ACCESS_ID_ALREADY_EXISTS and leave the access ID out of that OM's
tenant tables. This also fixes a case that exists today: a follower that
applies a tenant assign after the leader has flushed it finds the secret in the
shared store and rejects the assign, and a later tenant revoke of that access
ID fails on that follower with a NullPointerException ({{omDBAccessIdInfo ==
null}}), which terminates it. The assign still rejects an entry with a
different secret, so the case where the access ID was revoked and assigned
again before a lagging OM applies the first assign remains; it is part of the
design issue. For a single OM replaying after a restart that case was limited
to a crash within one flush before this patch, and with it any restart between
the apply and the flush of the first assign is enough.
With the patch, a store failure in these requests happens at apply time, as for
getsecret: the request fails instead of being answered as successful, and
nothing calls the store from the flush thread. The OM still terminates on that
apply time failure (INTERNAL_ERROR); that is part of the design issue filed
alongside.
This is a contained fix in line with HDDS-10469, and it adds apply time store
calls for these four requests. A leader side design, as discussed in the design
issue filed alongside, would replace them.
Tests added to the existing {{TestS3GetSecretRequest}}, all with a store
without batch support:
* {{testRevokeThenGetSecretBeforeFlushWithNonBatchStore}}: S3 revoke then
getsecret in one flush; the store keeps the issued secret.
* {{testRevokeFailsWhenNonBatchStoreFails}}: the store rejects the revoke; the
request fails with INTERNAL_ERROR and the secret is kept.
* {{testSetThenRevokeBeforeFlushWithNonBatchStore}}: set then revoke in one
flush; the store ends empty. It passes without the patch too, and fails if only
the revoke is moved.
* {{testTenantAccessIdReplayAndRevokeWithNonBatchStore}}: a tenant assign
writes the store when applied, applying it again after a restart without a
flush succeeds, and a tenant revoke clears the store.
Without the patch the first, second and fourth fail. Store failures of set and
of the tenant requests and the follower cases are not covered by unit tests.
The existing S3 secret, tenant, double buffer and state machine unit tests
(193), {{TestMultiTenantVolume}} and {{TestOzoneTenantShell}} pass, but they
use the default RocksDB store and do not run the changed branches. Checkstyle
is clean.
The S3 secret cache fix filed alongside also edits {{S3RevokeSecretRequest}},
{{OMTenantRevokeUserAccessIdRequest}} and {{TestS3GetSecretRequest}}, so
whichever lands second needs a rebase.
Found by TLA+ model checking and code review of the OM S3 secret cache and
external S3 secret store paths under HDDS-15926, on commit
df04bc0e309f625a7dcdcd60790a42f34e2743d9. Checked against HDDS issues and
apache/ozone pull requests for duplicates before filing. Related but not
duplicates: HDDS-10469 (moved only the getsecret store write to apply time),
HDDS-17200 ({{getSecretString}} ignores the deleted marker) and HDDS-9444
(cache and table consistency for the default store). 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]
