[
https://issues.apache.org/jira/browse/HDDS-17177?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng reassigned HDDS-17177:
---------------------------------
Assignee: Siyao Meng
> 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
> Assignee: Siyao Meng
> Priority: Major
> 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]
