Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Exception level logging statements #566

Closed
HUSSTECH opened this issue Feb 25, 2023 · 1 comment
Closed

Remove Exception level logging statements #566

HUSSTECH opened this issue Feb 25, 2023 · 1 comment

Comments

@HUSSTECH
Copy link

Problem:

I believe a couple of _LOGGER.exception statements in the code should be removed or turned into an explicit exception raise. During our applications handling of a DecryptKeyError from a decryption_session.decrypt_text(..) operation (because it was intentionally being given the wrong kms key), the console logs would contain this text.

Error on closing
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/aws_encryption_sdk/__init__.py", line 196, in decrypt
    plaintext = decryptor.read()
  File "/usr/local/lib/python3.8/site-packages/aws_encryption_sdk/streaming_client.py", line 260, in read
    self._prep_message()
  File "/usr/local/lib/python3.8/site-packages/aws_encryption_sdk/streaming_client.py", line 792, in _prep_message
    self._header, self.header_auth = self._read_header()
  File "/usr/local/lib/python3.8/site-packages/aws_encryption_sdk/streaming_client.py", line 830, in _read_header
    decryption_materials = self.config.materials_manager.decrypt_materials(request=decrypt_materials_request)
  File "/usr/local/lib/python3.8/site-packages/aws_encryption_sdk/materials_managers/caching.py", line 251, in decrypt_materials
    new_result = self.backing_materials_manager.decrypt_materials(request)
  File "/usr/local/lib/python3.8/site-packages/aws_encryption_sdk/materials_managers/default.py", line 150, in decrypt_materials
    data_key = self.master_key_provider.decrypt_data_key_from_list(
  File "/usr/local/lib/python3.8/site-packages/aws_encryption_sdk/key_providers/base.py", line 323, in decrypt_data_key_from_list
    raise DecryptKeyError("Unable to decrypt any data key")
aws_encryption_sdk.exceptions.DecryptKeyError: Unable to decrypt any data key

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/aws_encryption_sdk/streaming_client.py", line 228, in __exit__
    self.close()
  File "/usr/local/lib/python3.8/site-packages/aws_encryption_sdk/streaming_client.py", line 995, in close
    raise SerializationError("Footer not read")
aws_encryption_sdk.exceptions.SerializationError: Footer not read

For a long time I thought I was not catching the Exception correctly, and I was adding except SerializationError everywhere, until I realised this was logger text output.

The problem this causes, except for a messy console, is that many code instrumentation tools that capture exceptions and logs, are automatically set to capture logs of level error and above. In particular with Python's exception log level exc_info is set to True, and this means that often local variables (including ciphertext and plaintext) are captured and sent to these external tools.

Solution:

except AWSEncryptionSDKClientError:
# All known exceptions in close are safe to ignore.
# Only raise unknown exceptions in close.
_LOGGER.exception("Error on closing")

As per the comment in the code, it suggests this log line does not need to be there.

@ShubhamChaturvedi7
Copy link
Contributor

Hi @HUSSTECH ,

These logging statements provide visibility in cases where something unexpected goes wrong. They can certainly be more concise, but removing them and adding a raise would be a breaking change and requires a major version revision. I've created a Github Issue to track this.

Our implementation discourages the emission of the plaintext data key
via Python's repr semantics.
(See the data_key classes in structures.py).

If you still think that certain sensitive information is being logged from the ESDK, please report this as directed in our Security Policy instead of a public GitHub issue.

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

No branches or pull requests

2 participants