Skip to content

Conversation

@wbingli
Copy link

@wbingli wbingli commented Jul 24, 2020

Issue #, if available:

Description of changes:

The current error message in call chain will include full Java class name, which leaks internal implementation. It also miss important information such as request ID etc.

Just use standard SDK exception message which includes important information as well as not expose internal java class as event message.

Event Message Before:

Exception=[class software.amazon.awssdk.services.kms.model.KmsException] ErrorCode=[AccessDeniedException],  ErrorMessage=[User: arn:aws:sts::1234567890:assumed-role/abcd is not authorized to perform: kms:UntagResource on resource: arn:aws:kms:eu-central-1:1234567890:key/10b977e9-da47-4c82-929f-c3c3506709d0]

Event Message After:

User: arn:aws:sts::1234567890:assumed-role/abcd is not authorized to perform: kms:UntagResource on resource: arn:aws:kms:eu-central-1:1234567890:key/10b977e9-da47-4c82-929f-c3c3506709d0 (Service: AWSKMS; Status Code: 400;  Request ID: c86b48d0-2820-4ff3-a1de-d52ed24f922c)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@wbingli wbingli requested review from ammokhov, dchakrav-github and rjlohan and removed request for dchakrav-github and rjlohan July 24, 2020 00:34
Copy link
Contributor

@rjlohan rjlohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not too concerned here. This 'internal implementation' is the AWS SDK, which is open source, being called from a Resource Type handler, which is also open source. There is no real meaningful information leak here, and we lose the structure in diagnostics with this change.

@wbingli
Copy link
Author

wbingli commented Aug 3, 2020

I'm actually not too concerned here. This 'internal implementation' is the AWS SDK, which is open source, being called from a Resource Type handler, which is also open source. There is no real meaningful information leak here, and we lose the structure in diagnostics with this change.

However, this structure is not standard SDK exception message, and it introduces a new structure comparing with all existing resources. It causes inconsistent for customer to read the error message between different resources. Also it loses some important information, such as Request ID.

In the meanwhile, exposing a full Java class exception name doesn't seems proper as service error. e.g. Exception=[class software.amazon.awssdk.services.kms.model.KmsException]. How much value for customer can get to diagnose their stack error? Especially the full class name, it's clearly internal resource implementation, even the class is open source but we should not expose it.

Not all resource are public before they are published. This at least some level of information leaking to expose such low level error message to customer.

@rjlohan rjlohan merged commit 3a38ac0 into aws-cloudformation:master Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants