Siyao Meng created HDDS-17177:
---------------------------------
Summary: OM renew delegation token apply can write a canceled
token back and can lower a token's stored expiry
Key: HDDS-17177
URL: https://issues.apache.org/jira/browse/HDDS-17177
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments:
MC-1-reject-renew-of-removed-token-and-keep-later-expiry.patch,
TestBugMC1RenewAfterCancel.java, TestBugMC2RenewLowersExpiry.java
h3. Mechanism
{{OMRenewDelegationTokenRequest.preExecute}} validates the token on the leader
({{OzoneDelegationTokenSecretManager.renewToken}}: the token is in the in
memory token map, is not expired, and the caller is its renewer) and carries
the computed expiry in the request.
{{OMRenewDelegationTokenRequest.validateAndUpdateCache}} then, on every OM,
calls {{OzoneDelegationTokenSecretManager.updateRenewToken}}, which puts the
token into the in memory map unconditionally, then adds a {{dTokenTable}} cache
entry with that expiry, and {{OMRenewDelegationTokenResponse}} writes the row
to RocksDB. The apply neither checks that the token is still stored nor
compares the new expiry with the stored one. Two consequences:
* If a cancel of the token was applied before the renew, the renew apply writes
the token back into the map, the cache and RocksDB.
{{OMCancelDelegationTokenRequest}} had removed it from all three and returned
success.
* If a renew carrying an earlier expiry is applied after a renew carrying a
later one, the stored expiry goes down to the earlier value, below what the
first applied renew returned.
h3. Trigger
A renew whose validation on the leader passed is applied when the stored state
is no longer the one it was validated against: the token's cancel has been
applied since, or a renew with a later expiry has been applied since.
Validation in preExecute and the apply are separate steps, and nothing at apply
time checks the validated state again. The expiry in the request is computed by
the leader, clients cannot set it.
h3. Impact
* *Canceled token written back.* {{retrievePassword}}, which the OM RPC server
uses to authenticate token connections, accepts the canceled token again. The
row is in RocksDB, and a secret manager started on the same DB, as after an OM
restart, loads the token and accepts it. Every OM applies the same entries
through the same code, so all of them hold it (argued, one OM was run). The
token stays valid until its stored expiry and its renewer can renew it again up
to its max date (7 days by default). The client that canceled it was told that
the cancel succeeded. [^TestBugMC1RenewAfterCancel.java]
* *Stored expiry lowered.* Once the earlier expiry passes, {{retrievePassword}}
rejects the token and {{renewToken}} refuses to renew it, although a later
expiry was returned for it, and {{ExpiredTokenRemover}} deletes it on its next
scan. This fails closed. The shortfall equals the gap between the leader's
validations of the two renews. [^TestBugMC2RenewLowersExpiry.java]
h3. Reproduction
The two attached classes run in {{hadoop-ozone/ozone-manager}} (target path and
Maven command are in each class comment) and pass on unmodified source while
the bug is present. They use the real {{OzoneDelegationTokenSecretManager}},
the real {{validateAndUpdateCache}} of the cancel and renew requests and a real
{{OmMetadataManagerImpl}} with RocksDB, and call them in sequence: the leader
side validation ({{renewToken}}, {{cancelToken}}), then the applies in the
stated sequence, each response committed to RocksDB as the double buffer does.
No Ratis server and no second OM are involved.
h3. Patch
[^MC-1-reject-renew-of-removed-token-and-keep-later-expiry.patch], against
b94df213f20ec0d675f079b57a4566e4e64e4060. It also applies to master at
df42f7f60a2.
* {{OzoneDelegationTokenSecretManager}} records the identifiers removed by
applied cancels ({{removeToken}}, whose only caller is the cancel apply) and
drops each one once the token's max date has passed, since no renew can extend
a token beyond it. The renew apply rejects a token found there with
{{TOKEN_ERROR_OTHER}}, so no row is written and the map and cache are left
alone. The check deliberately does not look at {{dTokenTable}} or the in memory
map: {{ExpiredTokenRemover}} deletes from both outside Ratis on each OM's own
clock (HDDS-13457), on a restarted OM even before it replays its log, so a
check there could accept a renew on one OM and reject it on another, and lose a
valid token on the OM that catches up.
* The renew apply stores {{min(maxDate, max(new expiry, stored expiry))}} when
a row exists, otherwise the request's expiry as before. The cap keeps today's
result when the stored expiry is beyond max date (renew interval longer than
the max lifetime).
The record of applied cancels is kept in memory only. An OM that restarts, or
installs a checkpoint, between applying a cancel and applying a later renew of
the same token still writes that token back, as every OM does today. A stricter
alternative is a persisted cancel tombstone in {{dTokenTable}} that is rebuilt
into the record when tokens are loaded; it changes what a cancel writes, may
need a layout feature, and still meets rows that {{ExpiredTokenRemover}}
deletes outside Ratis.
Covered by four cases in the existing
{{TestOzoneDelegationTokenSecretManager}}:
{{testRenewAppliedAfterCancelDoesNotRestoreToken}},
{{testRenewAppliedWithEarlierExpiryKeepsStoredExpiry}},
{{testRenewAppliedAfterRemovalOutsideRatisIsApplied}} (a renew applied after
the remover deleted the token must still be applied, as on the other OMs) and
{{testRenewAppliedCapsExpiryAtMaxDate}}. Without the change the first two fail
("expected: TOKEN_ERROR_OTHER but was: OK", and the earlier expiry stored
instead of the later one). With it, 146 tests in the delegation token, request
handler, state machine and double buffer suites pass, checkstyle is clean, and
the two attached classes no longer show the bug. The Kerberos integration
suites were not run with the change.
Found by TLA+ model checking and code review of the OM delegation token secret
manager and request paths under HDDS-15926, on commit
b94df213f20ec0d675f079b57a4566e4e64e4060. Checked against HDDS issues and
apache/ozone pull requests for duplicates before filing. Related but not
duplicates: HDDS-13457 (the expired token remover runs on every OM outside
Ratis), HDDS-1973 and HDDS-1974 (moved renew and cancel to the cache and double
buffer). 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]
