[
https://issues.apache.org/jira/browse/HDDS-17200?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17200:
------------------------------
Attachment: CR-10-treat-deleted-s3-secret-cache-entry-as-absent.patch
> 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
> Priority: Major
> 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]
