Repository navigation
Conversation
8daceee to
9aed102
Compare
9aed102 to
f047006
Compare
GarrettBeatty
left a comment
There was a problem hiding this comment.
Good improvement. I checked with --debug and cli_history = enabled: inline PEM keys are redacted from both for sign, sign-cookies, --private-key=…, mistyped argument names and ssm put-parameter --value, and tracebacks are covered too. The related history/logger/autoprompt/cloudfront tests pass (524).
I found a few ways key material still gets through (details inline), so the changelog's "Redact PEM private keys passed as argument values" overclaims a bit:
- Base64-encoded keys:
aws acm import-certificate --private-key fileb://key.pemlogs"PrivateKey": "LS0tLS1CRUdJTiBSU0Eg…"in the--debugrequest body and in history (HTTP_REQUEST,API_CALL,CLI_ARGUMENTS). - Unquoted, word-split keys:
--private-key $Kwith no quotes means the header is never contiguous, so the key body appears in theArguments entered to CLIdebug line and in history. - stderr without
--debug: argparse errors print the key verbatim, e.g.Unknown options: -----BEGIN RSA PRIVATE KEY-----\nMIIE…(key passed twice) or--hash-algorithm: Found invalid choice '-----BEGIN…'. CI logs capture stderr. This is outside logging, so it could be a follow-up, but it's worth noting in the PR as out of scope.
Also minor: botocore's own Session.set_stream_logger/set_file_logger (awscli/botocore/session.py:629,657) still use a plain logging.Formatter. The CLI doesn't call them today, so nothing leaks now, but anything that does call them will bypass redaction.
| # PEM private keys. | ||
| register_redaction( | ||
| re.compile( | ||
| r'-----BEGIN [A-Z0-9 ]*PRIVATE KEY-----' |
There was a problem hiding this comment.
This only matches an armored, contiguous header, so two common cases get through.
Base64 of the PEM (blob parameters):
aws acm import-certificate --certificate fileb://cert.pem --private-key fileb://key.pem --debug
# ... botocore.endpoint - DEBUG - Making request ... "PrivateKey": "LS0tLS1CRUdJTiBSU0EgUFJJVkFURSBLRVkt..."
The same value lands in history HTTP_REQUEST, API_CALL and CLI_ARGUMENTS.
An unquoted, word-split key (--private-key $K): argv becomes ['-----BEGIN', 'RSA', 'PRIVATE', 'KEY-----', 'MIIE…', …] and the body shows up in Arguments entered to CLI.
Suggestions: also match base64 starting LS0tLS1CRUdJTi (base64 of -----BEGIN), and/or redact argv values by position after --private-key. Or, if those are out of scope, narrow the changelog to "PEM private keys passed inline".
|
|
||
| def _redact_text(text): | ||
| for pattern, replacement in _REDACTIONS: | ||
| text = pattern.sub(replacement, text) |
There was a problem hiding this comment.
Since register_redaction() is a public hook: replacement goes to re.sub as a template, so a backslash in it (e.g. '\1' or 'C:\…') gets interpreted or raises. Suggest pattern.sub(lambda _: replacement, text). Registration also has no dedupe and no way to unregister, and patterns run in order, so a later pattern can match an earlier replacement. Worth documenting, or deduping on (pattern.pattern, replacement).
| """ | ||
| if isinstance(value, str): | ||
| return _redact_text(value) | ||
| if isinstance(value, bytes): |
There was a problem hiding this comment.
Non-UTF-8 bytes are returned unchanged, so a body with a stray non-UTF-8 byte next to a PEM block isn't redacted. bytearray/memoryview aren't handled either. Suggest value.decode('utf-8', 'surrogateescape') / .encode('utf-8', 'surrogateescape'), or running a bytes version of the pattern directly.
|
|
||
| def format(self, record): | ||
| # The whole formatted record, including any traceback, is redacted. | ||
| return redact(super().format(record)) |
There was a problem hiding this comment.
Minor: super().format(record) caches the unredacted traceback in record.exc_text, so any other handler attached to the same logger reuses the unredacted text. Not reachable with the CLI's handlers today, but cheap to harden by setting record.exc_text = redact(record.exc_text) after formatting.
| object(), | ||
| ], | ||
| ) | ||
| def test_returns_values_without_matches_unchanged(value): |
There was a problem hiding this comment.
Test coverage: there are no negative cases for the bypasses above (a base64-encoded PEM, a word-split argv list, non-UTF-8 bytes containing a PEM block). Even if they're declared out of scope, xfail tests would document the limits.
| { | ||
| "type": "enhancement", | ||
| "category": "``logging``", | ||
| "description": "Redact PEM private keys passed as argument values from ``--debug`` output and the CLI history database." |
There was a problem hiding this comment.
Given the base64 and word-split cases (see the comment on redaction.py), suggest "Redact PEM private keys passed inline as argument values…" unless those cases get covered.
Issue #, if available:
Description of changes:
Description of changes
Redacts PEM private keys from debug output and CLI history. Depends on #10716 and #10715.
Security
--private-key "-----BEGIN RSA PRIVATE KEY-----...") are now redacted from--debugoutput and the CLI history database.-----END) is also redacted up to the end of the text.awscli/redaction.py) provides aregister_redaction()hook so future commands can add their own patterns.Testing
test_redaction.pycovers theredact()function and PEM pattern matching.test_logger.pyverifies the logger applies redaction before writing debug output.test_sign.pyasserts that no key material appears in--debugoutput or the history database whenaws cloudfront signis called with an inline PEM key.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.