Siyao Meng created HDDS-17200:
---------------------------------
Summary: S3SecretManagerImpl.getSecretString ignores the deleted
marker of an S3 secret cache entry and returns its secret
Key: HDDS-17200
URL: https://issues.apache.org/jira/browse/HDDS-17200
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: CR-10-treat-deleted-s3-secret-cache-entry-as-absent.patch
h3. Mechanism
{{S3SecretManagerImpl.getSecretString}} does its own cache lookup: it calls
{{S3SecretCache.get}} for the access key and, for any entry it finds, returns
{{S3SecretValue.getAwsSecret}} of that entry. It never consults
{{S3SecretValue.isDeleted}}. The in memory cache keeps a revoked secret as an
entry that still carries the secret string and is marked as deleted, until the
double buffer flush cleans the entry up (HDDS-9444), so a marked entry is a
value that the cache holds precisely to record that the secret is gone.
{{S3SecretManagerImpl.getSecret}}, the other reader of the same cache on the
same class, treats an entry marked as deleted as absent and returns null for it
without consulting the store. {{getSecretString}} bypasses {{getSecret}}
entirely, so the two readers of one cache state disagree: {{getSecret}} reports
the secret as absent and {{getSecretString}} returns its secret string. On a
cache miss {{getSecretString}} also reads {{S3SecretStore.getSecret}} itself
and writes the result back into the cache with {{updateCache}}, instead of
going through {{getSecret}}.
h3. Trigger
# The S3 secret cache holds an entry for an access key that is marked as
deleted and is not yet flushed.
# {{S3SecretManagerImpl.getSecretString}} is called for that access key.
h3. Impact
* For an access key whose cache entry is marked as deleted, {{getSecretString}}
returns the secret string of that entry instead of throwing
{{OzoneSecurityException}} with {{S3_SECRET_NOT_FOUND}}, which is what the same
class reports through {{getSecret}} for the same state. Confirmed by the
regression test below.
* The disagreement lasts as long as the marked entry stays in the cache, that
is until the flush removes it; after that {{getSecretString}} reads the store
and the secret is reported as absent (code reading, not run).
* No secret value and no stored row is changed by the read path, and the result
for an entry that is not marked as deleted is the same before and after the
patch.
* By history, not run on older code: the cache entries marked as deleted
arrived with HDDS-9444, so a marked entry can only exist from that change
onwards.
h3. Reproduction
There is no standalone reproduction; the regression test is in the attached
patch. {{TestS3GetSecretRequest.testGetSecretStringOfRevokedSecret}}, added to
the existing suite, drives a get and then a revoke through
{{S3RevokeSecretRequest.validateAndUpdateCache}} without a flush, asserts that
the cache entry is marked as deleted and that {{getSecret}} returns null, and
then expects {{getSecretString}} to throw with {{S3_SECRET_NOT_FOUND}}. Without
the change it fails with "Expected
org.apache.hadoop.hdds.security.exception.OzoneSecurityException to be thrown,
but nothing was thrown."
h3. Patch
[^CR-10-treat-deleted-s3-secret-cache-entry-as-absent.patch], against
a6b7bdb937109ba2893688b41a89470bc95c88cf. It also applies to master at
1753288f333.
{{S3SecretManagerImpl.getSecretString}} no longer reads the cache and the store
itself. It calls {{getSecret}} for the access key, which is the reader that
treats an entry marked as deleted as absent without consulting the store, and
throws {{OzoneSecurityException}} with {{S3_SECRET_NOT_FOUND}} when the result
is null. The direct cache lookup is dropped; on a cache miss {{getSecret}}
reads the store and caches the result as {{getSecretString}} did before, so
there is one reader of the cache state for both methods. There is no wire or
protocol change and nothing stored changes.
With the patch 125 unit tests pass ({{TestS3GetSecretRequest}},
{{TestOzoneDelegationTokenSecretManager}}, {{TestOzoneManagerDoubleBuffer}},
{{TestS3DerivedKey}}, {{TestS3SecurityUtil}}, the other S3 secret request
tests, and the tenant request and manager tests),
{{TestSecureOzoneCluster#testGetSetRevokeS3Secret}} and
{{TestMultiTenantVolume}} pass on a mini cluster (6 tests), and checkstyle is
clean.
Found by code review of the OM multi-tenancy and Ranger sync paths, as part of
the TLA+ verification effort under HDDS-15926, on commit
a6b7bdb937109ba2893688b41a89470bc95c88cf. Checked against HDDS issues and
apache/ozone pull requests for duplicates before filing. Related but not a
duplicate: HDDS-9444 (introduced the in memory S3 secret cache entries that are
marked as deleted). 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]
