Skip to content

Logging mask credentials #12292

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

Merged
merged 4 commits into from
Feb 20, 2025
Merged

Logging mask credentials #12292

merged 4 commits into from
Feb 20, 2025

Conversation

cloutierMat
Copy link
Contributor

Motivation

This pr introduces a logging filter that can be used to mask sensitive values in our http logger. As we log every incoming HTTP request raw body, we can user this filter to find sensitive keys in a json string and mask their value.

Changes

  • Base class to allow for value masking
  • Test including an example subclass

@cloutierMat cloutierMat added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Feb 20, 2025
@cloutierMat cloutierMat added this to the 4.2 milestone Feb 20, 2025
@cloutierMat cloutierMat self-assigned this Feb 20, 2025
Copy link

github-actions bot commented Feb 20, 2025

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 50m 51s ⏱️ - 1m 30s
4 099 tests ±0  3 767 ✅ ±0  332 💤 ±0  0 ❌ ±0 
4 101 runs  ±0  3 767 ✅ ±0  334 💤 ±0  0 ❌ ±0 

Results for commit 0ec5b36. ± Comparison against base commit 3b6bee2.

♻️ This comment has been updated with latest results.

@cloutierMat cloutierMat marked this pull request as ready for review February 20, 2025 19:47
@cloutierMat cloutierMat requested a review from bentsku February 20, 2025 19:47
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM! Nice test, pretty neat 👌

I only have one comment regarding the dict usage for storing the pattern + key, I wonder if we could save the replacement value already at init time. Very minor, not really blocking!

Comment on lines 91 to 92
for key, pattern in self.patterns.items():
message = re.sub(pattern, to_bytes(f'"{key}": "******"'), message)
Copy link
Contributor

Choose a reason for hiding this comment

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

question/nit: should we actually save the pattern and the replacement value already in self.patterns?
Maybe we do not need a dict, and could use a list of tuple?
So that we would need to convert the replacement value every time. It's quite a minor optimisation, but I'm not seeing the value of a dict directly here as we never access it by key.
Something like:

self.patterns = [(re.compile(to_bytes(rf'"{key}":\s*"[^"]+"')), to_bytes(f'"{key}": "******"')) for key in self.sensitive_keys]
...
for pattern, repl_value in self.patterns.items():
    message = re.sub(pattern, repl_value, message)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are totally right 👀. I missed that simple fact! 🤣 I will update 😉

@cloutierMat cloutierMat merged commit f269fe2 into master Feb 20, 2025
31 checks passed
@cloutierMat cloutierMat deleted the logging-mask-credentials branch February 20, 2025 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants