fix(#856): the identity, keys and certificates family of identifier draw sites - #1287
Merged
Merged
Conversation
Eight generators across seven services move onto IDMint: Cognito (both the user-pool and the identity-pool API), IAM Identity Center, KMS, ACM, Secrets Manager, WAFv2, and API Gateway's API keys. Every rendering is byte-for-byte what crypto/rand produced, so an identifier a previous substrate recorded is still the shape this one mints. Nothing in any of the seven API models distinguishes the two renderings of a UUID-shaped value, which is why #671 leaves them alone. Three of the minted values are not identifiers, and are in scope because a caller hands them back. A WAFv2 LockToken must be returned to UpdateWebACL, so a re-minted one answered a recorded update with WAFOptimisticLockException. A Secrets Manager version ID is the quietest of the family: GetSecretValue reports an unknown VersionId by omitting SecretString rather than refusing, so a replay answered a recorded read with a 200 that had lost a member. A KMS stub data key is observed twice in one response, as Plaintext and inside CiphertextBlob. An API Gateway API key's id and value were drawn from ACM's certificate-ID generator, so a change to ACM's rendering would have silently moved API Gateway's. They now have their own minter. Two fidelity gaps the family turned up are filed rather than folded in: a secret version ID is half the published minimum width and ignores the caller's ClientRequestToken (#1285), and CreateUserPool reports the pool id as UserPool.UserPoolId where UserPoolType publishes Id (#1286). 15 draw sites remain on crypto/rand. Refs #856.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
6 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tier 4 of #856. Eight generators across seven services move onto
IDMint: Cognito (both theuser-pool and the identity-pool API), IAM Identity Center, KMS, ACM, Secrets Manager, WAFv2, and
API Gateway's API keys.
rand.Readdraw sites go from 23 to 15.Every rendering is byte-for-byte what the
crypto/randversion produced — the width, the case andthe alphabet — so an identifier a previous substrate recorded is still the shape this one mints.
Where the choice was between
IDMint.UUID(RFC 4122 version and variant nibbles set) andIDMint.HexUUID(the same grouping without them), the API model was consulted each time and settlednothing:
API_ApiKeypublishes no length and no pattern on eitheridorvalue,API_KeyMetadata.KeyIdpublishes a length of 1–2048 and no pattern, andCertificateArn's patternadmits either form. So the existing rendering stays, per #671's "only what the API model states".
WAFv2 is the exception where the model does speak —
API_WebACLSummarypublishes^[0-9a-f]{8}-(?:[0-9a-f]{4}-){3}[0-9a-f]{12}$, which is indifferent to those nibbles — andUUID()preserves what was recorded.
Three of these are not identifiers
They are in scope because a caller hands them back, so a re-minted one turns a recorded success
into a refusal or an incomplete response:
LockTokenmust be returned toUpdateWebACL. A replay that re-minted it answered therecorded update with
WAFOptimisticLockExceptioninstead of the recorded success. One minterserves the web ACL ID, the IP set ID and both lock tokens, because the published constraint is
identical on each.
GetSecretValuereports anunknown
VersionIdby omittingSecretStringrather than by refusing, so a replay answered arecorded read with a 200 that had silently lost a member.
GenerateDataKeystub data key is observed twice in one response: the bytes are thePlaintext, and the same bytes are wrapped into theCiphertextBlob. A recordedDecryptwasnever affected — substrate's ciphertext carries its own plaintext, so a replayed decrypt answers
from the recorded blob regardless — so the break was in the create, not the read.
One service was minting another's identifiers
CreateApiKeyreached intogenerateACMCertIDfor both UUID-shaped strings it returns, so a changeto ACM's rendering would have silently moved API Gateway's. They now have their own minters, and the
idandvaluedraw in turn so they differ.generateACMCertIDno longer returns an error: themint cannot fail where
rand.Readcould, so the caller's dead error branch went with it.Tests
emulator/ids_test.gogains the ordinal test — oneCreateUserPoolClientwithGenerateSecretdraws three times, and the client ID, the first half of the secret and its second half are pairwise
distinct — and the wire assertion: a recorded stream across all seven services replays with zero
DifferencesandStateValidtrue underValidateState. Each recorded read is one a re-mintedvalue would break, and the assertion was shown non-vacuous by reverting
generateVersionIDtorandomHex, which produces 21 differences:VersionIdmajors on three operations, the lostSecretString, and a state-hash cascade through the rest of the stream.Gaps filed rather than folded in
Keeping the tier shape-preserving is what lets its tests rest on "the rendering is unchanged", so two
fidelity gaps it turned up became issues:
VersionIdpublishes a minimum of 32,and
CreateSecret/PutSecretValuemint their own rather than using the caller'sClientRequestToken, which AWS documents as becoming the version ID and builds an idempotencycontract on. (
RotateSecretalready echoes the token it was sent.)CreateUserPool,DescribeUserPoolandUpdateUserPoolreport the pool's identifieras
UserPool.UserPoolId, whereUserPoolTypepublishesIdand carries noUserPoolIdmember atall. Found because the replay stream had to read
UserPoolIdto learn the pool ID.ListUserPoolsalready renders
Id, so the two disagree inside one service.Both are recorded in
docs/services.mdbeside the existing member-name gap (#1136) rather than leftonly on the tracker.
Verification
make lint(0 issues),make test(race, green), andmake docs-reference-check docs-versions version-check discarded-unmarshal-check wire-bookkeeping-checkwith the ratchet still at 330.Patch coverage intersected against
git diff -U0 main: 43/43 changed statement lines covered.Refs #856.