Repository navigation
fix(aws-kms): InvalidMarkerException on list operations, key ARN in DisabledException - #1682
Conversation
NitinKumar004
left a comment
NitinKumar004
left a comment
There was a problem hiding this comment.
What I checked
- Issue #1668 coverage. Both items are addressed. The unknown-Marker exception is now set per operation through a
markerFaultargument, andrequireUsablenames the key ARN in its message. - Architecture. The Marker is an opaque wire token that only the server issues, and the driver list methods return the full set. So validating the Marker in
server/aws/kms/paginate.gois the right layer, and the provider stays free of paging.ExInvalidMarkergoes back intoservices/kms/driver/errors.go. No driver interface changed, andgo generate ./...leaves no diff. - Parity against the KMS API reference and the SDK model:
- InvalidMarkerException is modeled on ListKeys, ListAliases, ListGrants, ListRetirableGrants, ListKeyRotations, ListResourceTags and DescribeCustomKeyStores. It is not modeled on ListKeyPolicies, and the PR maps all of them correctly.
- MarkerType is min 1, max 1024, with pattern
[ -ÿ]*. The length check counts runes, which is correct. Invalid UTF-8 bytes decode to U+FFFD and get ValidationException. - The DisabledException text
<key ARN> is disabled.matches the captured real response.errors.Is(err, driver.ErrKeyDisabled)still matches, so Secrets Manager'sisKeyStateErris unaffected.
- Races. The new message reads
kd.meta.ARN, which never changes after create. Every caller ofrequireUsableholdskd.mu(an RLock in the crypto paths,mutateKeyfor Enable/DisableKeyRotation and RotateKeyOnDemand). The PR adds no check-then-act sequence. - Gates on the head:
go build ./...,go veton the kms packages and gofmt are clean.go test -racepasses on providers/aws/kms, server/aws/kms, providers/aws/secretsmanager and server/aws/secretsmanager.go mod tidyandgo generate ./...leave no diff.- golangci-lint, run without
--new-from-revon providers/aws/kms, server/aws/kms and services/kms, reports 0 issues on the head and 0 on the e7c262c baseline.
- AWS CLI against
cloudemu serve:- The unknown Marker
bm90LWEtbWFya2Vygives InvalidMarkerException on all 7 modeled operations and ValidationException on ListKeyPolicies. --page-size 1auto-paging returns every item on ListKeys, ListAliases, ListGrants, ListRetirableGrants and ListResourceTags.- A 1025-character Marker gives ValidationException.
- Encrypt, EnableKeyRotation and Sign on a disabled key return DisabledException
arn:aws:kms:...:key/... is disabled.with HTTP 400. Encrypt works again after EnableKey.
- The unknown Marker
- Terraform hashicorp/aws 5.x with aws_kms_key, aws_kms_alias, aws_kms_grant and the aws_kms_alias data source: apply, plan clean, update, plan clean and destroy all succeed.
- The PR body and commits contain no attribution lines.
High
None.
Medium
None.
Low
-
Limit bounds are not enforced on any list operation. This predates the PR.
pageWindowtreatsLimit <= 0as the default and applies no upper bound. Against serve, ListAliases acceptsLimitvalues of 0, -1, 101 and 5000 without error, and ListResourceTags acceptsLimit: 51. The documented ranges are:Operation Limit range ListKeys, ListKeyPolicies, ListKeyRotations, DescribeCustomKeyStores 1-1000 ListAliases, ListGrants, ListRetirableGrants 1-100 ListResourceTags 1-50 Real KMS rejects a value outside its range with ValidationException. Since this PR already threads a per-operation argument through
pageWindow, a per-operation(min, max, default)would fit next tomarkerFault. -
The default page size is wrong for four operations. This also predates the PR.
defaultPageLimit = 100is used everywhere, but ListAliases, ListGrants, ListRetirableGrants and ListResourceTags default to 50. With 60 aliases,ListAliases {}returns all 60 withTruncated: false, where real KMS returns 50 withTruncated: true. SDK paginators still work. A client that reads only the first page sees a different count from real KMS. -
A forged Marker that is in the right format is still accepted. The PR body says "a Marker the server never issued now gives InvalidMarkerException", but
base64("999")(OTk5) on ListKeys returns an empty page withTruncated: falseinstead of an error, becausepageWindowclampsstarttototal. Rejectingstart > totalwhen a Marker was sent would make the claim true. One caveat: a Marker that pointed exactly at the end of a list that later shrank would then also error. -
The order of validation differs from real KMS. The MarkerType constraint is checked after the provider lookup. So a 1025-character Marker with a KeyId that doesn't exist gives NotFoundException, where real KMS fails request validation first with ValidationException. The constraint check needs no state, so it could run before
h.kms.ListGrants(...), and likewise in the other handlers that look up a key.
| ) | ||
|
|
||
| // defaultPageLimit is the page size KMS list operations use when Limit is unset. | ||
| const defaultPageLimit = 100 |
There was a problem hiding this comment.
This default is right for ListKeys, ListKeyPolicies, ListKeyRotations and DescribeCustomKeyStores. ListAliases, ListGrants, ListRetirableGrants and ListResourceTags default to 50 in the API reference. With 60 aliases, ListAliases {} here returns 60 with Truncated false, where real KMS returns 50 with Truncated true. This predates the PR, but a per-operation default could go next to markerFault.
| start, err = decodeMarker(marker) | ||
| // fault picks the exception for a Marker that decodes to no position. | ||
| func pageWindow( | ||
| marker string, limit int32, total int, fault markerFault, |
There was a problem hiding this comment.
Limit isn't range-checked. <= 0 falls back to the default and there's no upper bound, so ListAliases accepts Limit 0, -1, 101 and 5000, and ListResourceTags accepts 51. Real KMS returns ValidationException outside 1-1000 (ListKeys, ListKeyPolicies, ListKeyRotations, DescribeCustomKeyStores), 1-100 (ListAliases, ListGrants, ListRetirableGrants) or 1-50 (ListResourceTags). Since this signature already takes a per-operation argument, a small per-operation limits struct would cover both the bounds and the defaults. This predates the PR, so it's a follow-up and doesn't block.
| // Marker that passes them but was not issued by this server is | ||
| // InvalidMarkerException where the operation models it. | ||
| func decodeMarker(marker string, fault markerFault) (int, error) { | ||
| if err := validateMarkerConstraint(marker); err != nil { |
There was a problem hiding this comment.
A Marker in the right format that this server never issued still passes. OTk5 (base64 of 999) on ListKeys returns an empty page with Truncated false, because pageWindow clamps start to total. If the goal is that an unissued Marker gives InvalidMarkerException, rejecting start > total when a Marker was sent would cover it.
| sort.Slice(grants, func(i, j int) bool { return grants[i].GrantID < grants[j].GrantID }) | ||
|
|
||
| start, end, next, truncated, perr := pageWindow(req.Marker, req.Limit, len(grants)) | ||
| start, end, next, truncated, perr := pageWindow(req.Marker, req.Limit, len(grants), markerInvalid) |
There was a problem hiding this comment.
The MarkerType constraint check runs after h.kms.ListGrants, so a 1025-character Marker with a KeyId that doesn't exist gives NotFoundException. Real KMS validates the request shape first and returns ValidationException. The constraint check needs no state, so it could run before the provider call here and in the other handlers that look up a key.
…list operations A Marker the server never issued now gives InvalidMarkerException on ListKeys, ListAliases, ListGrants, ListRetirableGrants, ListKeyRotations, ListResourceTags and DescribeCustomKeyStores, which all model it. ListKeyPolicies does not model it, so it keeps ValidationException. A Marker longer than 1024 characters or with a character outside U+0020-U+00FF fails the modeled MarkerType constraint and gives ValidationException on every operation.
Real KMS answers an operation on a Disabled key with "<key ARN> is disabled." instead of a generic "key is disabled". The error is built with driver.WithMessage, so errors.Is still matches driver.ErrKeyDisabled for Secrets Manager and other callers. Every operation that returns DisabledException goes through requireUsable, so they all get the same message.
3db0606 to
f52de43
Compare
Builds on #1647 (merged); rebased onto
development, so the diff is now only this PR's changes.Summary
InvalidMarkerExceptionon the KMS list operations that model it.ListKeyPoliciesdoesn't model it, so it keepsValidationException.DisabledExceptionnow carries the message real KMS sends:<key ARN> is disabled.Changes
Item 1: InvalidMarkerException. Status: FIXED (commit
69ebfe0e)KMS pagination lives in
server/aws/kms/paginate.go. The driver list methods return the full set, and the server cuts pages and issues the opaque Marker. The Marker is a wire token that only the server creates, so checking it stays in that file. The provider has no Marker concept.pageWindowtakes amarkerFaultthat each operation sets from its modeled error list.MarkerTypeconstraint (longer than 1024 characters, or a character outside[ -ÿ]*) givesValidationExceptionon every operation, with the AWS constraint message.InvalidMarkerException, orValidationExceptiononListKeyPolicies.ExInvalidMarkeris back inservices/kms/driver/errors.go. fix(aws-kms): DeriveSharedSecret, custom key stores, reserved aws: tags, grant principal validation #1647 removed it because nothing used it, and now it is used.I checked each operation's error list in the botocore model (
kms/2014-11-01/service-2.json) and in the API reference:InvalidMarkerExceptionmodeledThese are the only KMS operations with a
Markerinput in the model. All of them useMarkerType(min 1, max 1024, pattern[ -ÿ]*). TheInvalidMarkerExceptionmessage is the exception's documented description, because no captured real response text was available.Item 2: DisabledException text. Status: FIXED (commit
3db06062)DisabledException: arn:aws:kms:us-west-2:187416307283:key/7e5a8e30-... is disabled.(status 400), for EnableKeyRotation on a disabled key. The DisabledException reference only documents the meaning, not the text.requireUsableinproviders/aws/kms/crypto.gonow returnsdriver.WithMessage(driver.ErrKeyDisabled, "%s is disabled.", ARN). Every operation that returns DisabledException goes throughrequireUsable: Encrypt, Decrypt, ReEncrypt, the GenerateDataKey family, Sign, Verify, GenerateMac, VerifyMac, DeriveSharedSecret, GetPublicKey, Enable/DisableKeyRotation and RotateKeyOnDemand. So they all get the same text.errors.Is(err, driver.ErrKeyDisabled)still matches, so Secrets Manager'sisKeyStateErrand other callers still work.Provider Coverage
Checklist
go test ./...)cloudemu_test.go(not needed: covered by provider and SDK wire tests)go generate ./...leaves no diff, since no driver interface changed)Test Plan
New tests. Each one fails on
e7c262cband passes now:server/aws/kms/pagination_test.goTestSDKListBadMarker: uses the real aws-sdk-go-v2 client. It runs all 8 list operations with an unknown Marker (InvalidMarkerException, orValidationExceptionfor ListKeyPolicies), a 1025-character Marker and a Marker with U+0100 (bothValidationException). Before the fix, the 7 InvalidMarkerException subtests failed withgot ValidationException: invalid Marker.providers/aws/kms/key_state_test.goTestDisabledKeyMessageNamesKey: Encrypt, GenerateDataKey and EnableKeyRotation on a disabled key. It checkserrors.Is(err, driver.ErrKeyDisabled)and the message<ARN> is disabled.server/aws/kms/key_state_sdk_test.goTestSDKDisabledExceptionMessage: the typed*types.DisabledExceptionwithErrorMessage() == "<ARN> is disabled.".Checks:
go build ./...: okgo veton the kms packages: okgo generate ./...: no diffgo test -raceon providers/aws/kms, server/aws/kms, services/kms, persist, providers/aws/kmscrypto, providers/aws/secretsmanager, providers/aws/ssm, server/aws/secretsmanager and providers/aws: all okgo test -p 2over all 657 packages, in 22 chunks of 30: all okgolangci-lint run --new-from-rev=origin/fix/aws-kms-parity-1523on providers/aws/kms, server/aws/kms and services/kms: 0 issues, so no new findings in touched packagesAWS CLI v2 against
cloudemu serve -providers aws -aws-port 4612, built from this branch:Raw
X-Amz-Targetrequests with"Marker":"not-a-marker":InvalidMarkerExceptionValidationExceptionbadĀmarker:ValidationExceptionwith1 validation error detected: Value '...' at 'marker' failed to satisfy constraint: ...list-keys --page-size 1auto-pagination still returns every key.Known limits:
InvalidMarkerExceptionmessage is the documented description of the exception, not text captured from real KMS.ValidationExceptionis the only modeled error that fits, so it stays."Marker":""is still treated as no Marker. Real KMS would reject it undermin: 1, but SDKs never send it.KMSInvalidStateExceptionfor other key states still uses a generic message. That wasn't in this issue.Related Issues
Part of #1668