[
https://issues.apache.org/jira/browse/HDDS-17406?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Fabian Morgan reassigned HDDS-17406:
------------------------------------
Assignee: Fabian Morgan
> Revoked STS token purge deletes cutoffs without checking their current value
> and leaves purged entries in the table cache
> -------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17406
> URL: https://issues.apache.org/jira/browse/HDDS-17406
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 2.3.0
> Reporter: Siyao Meng
> Assignee: Fabian Morgan
> Priority: Major
> Attachments: MC-1-CR-4-purge-only-unchanged-revocation-cutoffs.patch
>
>
> h3. Mechanism
> {{RevokedSTSTokenCleanupService}} picks the {{s3RevokedStsTokenTable}}
> entries to purge from {{revokedStsTokenTable.iterator()}}, which reads
> RocksDB only ({{TypedTable.iterator}} does not consult the table cache), and
> submits {{DeleteRevokedSTSTokens}} with the original access key IDs whose
> cutoff is older than 12h. The request carries the key IDs only.
> {{S3DeleteRevokedSTSTokensRequest.validateAndUpdateCache}} does not read the
> current cutoff and writes nothing to the cache, and
> {{S3DeleteRevokedSTSTokensResponse.addToDBBatch}} deletes every listed key
> from the DB.
> This has two effects on the table, which is a full cache table:
> # A purge entry deletes whatever cutoff the key has in the DB when the entry
> is applied, not the one the scan read. When a {{RevokeSTSToken}} for the same
> key that writes a newer cutoff is ordered before the purge entry, the purge
> removes that newer cutoff from the DB. The table cache keeps it, because the
> purge writes no cache entry, so an OM that has not rebuilt its cache still
> reads it, while an OM that rebuilds the cache from the DB (restart,
> checkpoint install) has no cutoff for that key.
> # Every purged entry stays in the table cache, since {{FullTableCache}} only
> evicts entries whose cache value is null. Cache first reads ({{getIfExist}})
> and the cache size keep reporting purged cutoffs on every OM until it
> rebuilds the cache, while the DB has none.
> {{S3SecurityUtil.isRevokedStsToken}} reads this table through the cache
> ({{getIfExist}}).
> h3. Trigger
> For the first effect, the leader's cleanup scan reads a cutoff older than 12h
> for a key, and a {{RevokeSTSToken}} for the same key that writes a newer
> cutoff is ordered before the purge entry. Two states lead there, both
> reproduced with the real scan and apply code:
> # The newer cutoff is applied on the leader but not yet flushed to RocksDB
> when the scan creates its iterator.
> # The newer {{RevokeSTSToken}} is applied after the scan, but is ordered
> before the purge entry in the log.
> The older cutoff must still be in the table (the first cleanup run after it
> turns 12h old removes it; the default interval is 3h), so this is a race with
> one cleanup run.
> The second effect follows from every purge.
> h3. Impact
> * First effect: after the purge is flushed, the DB of every OM has no row for
> the key, although the last applied {{RevokeSTSToken}} for it wrote a newer
> cutoff. OMs that rebuild the table cache from the DB have no cutoff for the
> key; OMs that did not still have the newer one in the cache. Revocation
> checks for STS tokens of that key created before the newer cutoff read that
> state and differ between the two groups until those tokens expire (at most
> 12h). Argued from {{S3SecurityUtil.isRevokedStsToken}}, not run on a cluster.
> * Second effect: purged cutoffs stay readable through the cache and counted
> in the cache size until a cache rebuild, so OMs at the same applied index
> hold different cache contents. A purged cutoff is at least 12h old and STS
> tokens live at most 12h, so this does not change any revocation check.
> h3. Reproduction
> The regression tests in the attached patch cover both effects
> ({{hadoop-ozone/ozone-manager}}, real {{OmMetadataManagerImpl}} with a flush
> as the double buffer does it). They need the new field to compile, so they
> were checked with only the apply change in
> {{S3DeleteRevokedSTSTokensRequest}} reverted, where the first two and the
> mismatch test fail:
> * {{TestS3RevokeSTSTokenRequest.testPurgeKeepsNewerCutoffOfSameKey}}: older
> cutoffs of three keys are flushed and one purge with those values is applied.
> For the first key a newer cutoff is applied before the purge, for the second
> one right after it (before the flush), and the third has none. The newer
> cutoffs must remain in the cache and in the DB, and only the third key is
> removed. Without the change the DB has no row for the first key.
> *
> {{TestS3RevokeSTSTokenRequest.testPurgeRemovesUnchangedCutoffFromCacheAndDB}}:
> after a purge of an unchanged cutoff is flushed, neither the cache nor the
> DB may return it and the cache must become empty. Without the change
> {{getIfExist}} still returns the purged cutoff.
> *
> {{TestS3RevokeSTSTokenRequest.testPurgeWithoutCutoffsDeletesListedEntries}}:
> a request without the new list still deletes the listed entries.
> *
> {{TestS3RevokeSTSTokenRequest.testPurgeWithMismatchedCutoffsDeletesNothing}}:
> a cutoff list whose length differs from the key list is rejected and nothing
> is deleted.
> *
> {{TestRevokedSTSTokenCleanupService.submitsCleanupRequestForOnlyExpiredTokens}}
> now also checks that the scan sends the cutoff it read.
> No standalone reproduction is attached.
> h3. Patch
> [^MC-1-CR-4-purge-only-unchanged-revocation-cutoffs.patch], against
> 5f7111a27ea07ce6b7bf5284a4213ebacb65f2ae. It also applies to master at
> 65320362775.
> {{DeleteRevokedSTSTokensRequest}} gets {{repeated uint64 revocationTimeMillis
> = 2}}, the cutoff the scan read for each key, in key order.
> {{RevokedSTSTokenCleanupService}} fills it and includes it in the request
> size check. On apply, {{S3DeleteRevokedSTSTokensRequest}} reads each key's
> current cutoff with {{getIfExist}}, which every OM answers the same way for
> the same log prefix, and deletes only the keys whose cutoff still equals the
> one the scan read. For each deleted key it writes a delete marker into the
> table cache, as other full cache table deletes do, so the entry leaves the
> cache after the flush. A newer cutoff written between the scan and the purge
> stays in the cache and in the DB. A cutoff list whose length differs from the
> key list is rejected with {{INTERNAL_ERROR}} and deletes nothing.
> A request with no cutoff list, as written before this change, is applied as
> before (all listed keys deleted, no cache entry), so log entries written
> without the field apply the same way on patched OMs. An OM without the patch
> ignores the field and deletes unconditionally, and there is no layout feature
> for it, so the patch should land before the first release that ships STS;
> {{DeleteRevokedSTSTokens}} is not in any release yet. Three byte limit tests
> in {{TestRevokedSTSTokenCleanupService}} have new numbers because each entry
> is now larger.
> A narrower alternative without a wire change is to let the scan read each
> cutoff through the table cache; that covers only the first of the two states
> and leaves purged entries in the cache.
> With this patch the purge also removes the cutoff from the table cache on
> every OM, so the change relies on the purge threshold covering clock
> differences between OMs, which a separate issue addresses.
> With the patch {{TestS3RevokeSTSTokenRequest}},
> {{TestRevokedSTSTokenCleanupService}}, {{TestOzoneManagerRatisRequest}},
> {{TestS3SecurityUtil}}, {{TestS3AssumeRoleRequest}},
> {{TestOzoneManagerDoubleBuffer}} and {{TestOmMetadataManager}} pass, and
> checkstyle on {{hadoop-ozone/interface-client}} and
> {{hadoop-ozone/ozone-manager}} is clean.
> Found by TLA+ model checking and code review of the OM STS token revocation
> and cleanup paths under HDDS-15926, on commit
> 5f7111a27ea07ce6b7bf5284a4213ebacb65f2ae. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. Related but not
> duplicates: HDDS-14094 (introduced the cleanup service and the purge
> request), HDDS-14373 (revocation writes the table cache at once), HDDS-16879
> (cleanup service after a checkpoint install). The attached patch is a
> proposal for review. Generated with Specula (Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
