potiuk commented on code in PR #70956:
URL: https://github.com/apache/airflow/pull/70956#discussion_r4175325354
##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
@@ -244,6 +283,58 @@ def is_log_masking_enabled(cls) -> bool:
"""Check if secret masking in logs is enabled."""
return cls.mask_secrets_in_logs
+ @classmethod
+ def enable_content_pattern_masking(cls) -> None:
+ """Enable value-content pattern masking (well-known secret formats)."""
+ cls.mask_content_patterns = True
+
+ @classmethod
+ def disable_content_pattern_masking(cls) -> None:
+ """Disable value-content pattern masking."""
+ cls.mask_content_patterns = False
+
+ @classmethod
+ def is_content_pattern_masking_enabled(cls) -> bool:
+ """Check if value-content pattern masking is enabled."""
+ return cls.mask_content_patterns
+
+ def add_content_patterns(self, patterns: dict[str, str]) -> None:
+ """
+ Register additional named regex patterns for value-content masking.
+
+ Keys are pattern names (used for diagnostics), values are regex
+ source strings. Existing entries with the same name are replaced.
+ Invalid regexes are skipped with a warning rather than raising, so
+ a misconfigured deployment does not disable the whole masker.
+ """
+ changed = False
+ for name, source in patterns.items():
+ try:
+ re.compile(source)
Review Comment:
This validates each regex on its own, but `_get_content_pattern_replacer()`
compiles them as one alternation. A pattern that's valid alone but not in a
union passes validation here, and then `re.compile(combined)` raises on every
`_redact()` call. Two examples: an inline flag like `(?i)acme-[a-z]{8}`, or two
patterns that both define the same named group. The `except` in `_redact()`
then turns every string into `<redaction-failed>` and logs a warning per
string. Validating the combined pattern before accepting an entry would close
this.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
@@ -79,6 +79,42 @@ def to_dict(self) -> dict[str, Any]: ...
SECRETS_TO_SKIP_MASKING = {"airflow"}
"""Common terms that should be excluded from masking in both production and
tests"""
+KNOWN_SECRET_PATTERNS: dict[str, str] = {
+ # Word-boundary lookarounds keep AWS keys from matching inside longer
+ # uppercase runs (e.g. an unrelated 20-char identifier that happens to
+ # start with "AKIA").
+ "aws_access_key": r"(?<![A-Z0-9])(?:AKIA|ASIA)[0-9A-Z]{16}(?![A-Z0-9])",
+ "github_token": r"\bgh[pousr]_[A-Za-z0-9]{36,255}\b",
+ "slack_token": r"\bxox[baprs]-[A-Za-z0-9-]{10,255}\b",
+ "google_api_key": r"\bAIza[0-9A-Za-z_\-]{35}\b",
+ "stripe_live_key": r"\bsk_live_[0-9A-Za-z]{24,64}\b",
Review Comment:
This stops at 64 chars after `sk_live_`. Current Stripe secret keys are much
longer, so with the trailing `\b` they never match, and only the legacy 24-char
format is caught. A larger upper bound fixes it.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
@@ -79,6 +79,42 @@ def to_dict(self) -> dict[str, Any]: ...
SECRETS_TO_SKIP_MASKING = {"airflow"}
"""Common terms that should be excluded from masking in both production and
tests"""
+KNOWN_SECRET_PATTERNS: dict[str, str] = {
+ # Word-boundary lookarounds keep AWS keys from matching inside longer
+ # uppercase runs (e.g. an unrelated 20-char identifier that happens to
+ # start with "AKIA").
+ "aws_access_key": r"(?<![A-Z0-9])(?:AKIA|ASIA)[0-9A-Z]{16}(?![A-Z0-9])",
+ "github_token": r"\bgh[pousr]_[A-Za-z0-9]{36,255}\b",
+ "slack_token": r"\bxox[baprs]-[A-Za-z0-9-]{10,255}\b",
+ "google_api_key": r"\bAIza[0-9A-Za-z_\-]{35}\b",
Review Comment:
This ends in `\b`, but `-` is in the character class, so a key ending in `-`
isn't matched. A negative lookahead avoids that:
```suggestion
"google_api_key": r"\bAIza[0-9A-Za-z_\-]{35}(?![0-9A-Za-z_\-])",
```
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
shared/secrets_masker/tests/secrets_masker/test_secrets_masker.py:
##########
@@ -1611,3 +1612,184 @@ def test_k8s_objects_still_detected_when_imported(self):
# Should be redacted since "password" is a sensitive field name
assert redacted["value"] == "***"
assert redacted["name"] == "password"
+
+
+class TestContentPatternMasking:
+ """Value-content pattern masking for well-known credential formats."""
+
+ @pytest.fixture
+ def masker(self):
+ m = SecretsMasker()
+ configure_secrets_masker_for_test(m)
+ return m
+
+ @pytest.mark.parametrize(
+ ("label", "sample"),
+ [
+ ("aws_access_key", "AKIAIOSFODNN7EXAMPLE"),
+ ("aws_session_key", "ASIAY34FZKBOKMUTVV7A"),
+ ("github_pat", "ghp_" + "a" * 40),
+ ("github_oauth", "gho_" + "b" * 40),
+ ("slack_bot", "xoxb-1234567890-abcdefghij"),
+ ("slack_user", "xoxp-1234567890-0987654321-abcdefghij"),
+ ("google_api_key", "AIza" + "A" * 35),
+ ("stripe_live_key", "sk_live_" + "0" * 24),
+ (
+ "jwt",
+
"eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.dozjgNryP4J3jVmNHl0w5N_XgL0n3I9PlFUP0THsR8U",
+ ),
+ ],
+ )
+ def test_known_pattern_is_redacted_when_enabled(self, masker, label,
sample):
+ masker.enable_content_pattern_masking()
+ try:
+ text = f"prefix {sample} suffix"
+ assert masker.redact(text) == "prefix *** suffix"
+ finally:
+ masker.disable_content_pattern_masking()
+
+ def test_pem_private_key_block_is_redacted_when_enabled(self, masker):
+ masker.enable_content_pattern_masking()
+ try:
+ pem = (
+ "-----BEGIN RSA " + "PRIVATE KEY-----\n"
+
"MIIBOgIBAAJBAKj34GkxFhD90vcNLYLInFEX6Ppy1tPf9Cnzj4p4WGeKLs1Pt8Qu\n"
+ "-----END RSA " + "PRIVATE KEY-----"
+ )
+ redacted = masker.redact(f"key: {pem} end")
+ assert redacted == "key: *** end"
+ finally:
+ masker.disable_content_pattern_masking()
+
+ def test_off_by_default(self, masker):
+ # By default, with a fresh masker and no explicit enable, an AWS-shaped
+ # value must pass through unchanged.
+ assert not masker.is_content_pattern_masking_enabled()
+ aws = "AKIAIOSFODNN7EXAMPLE"
+ assert masker.redact(aws) == aws
+
+ def test_disable_restores_passthrough(self, masker):
+ masker.enable_content_pattern_masking()
+ aws = "AKIAIOSFODNN7EXAMPLE"
+ assert masker.redact(aws) == "***"
+ masker.disable_content_pattern_masking()
+ assert masker.redact(aws) == aws
+
+ @pytest.mark.parametrize(
+ "benign",
+ [
+ "AKIA_LOOKS_LIKE_ONE",
+ "not-a-jwt.header.only",
+ "sk_live_short",
+ "gh_notatokentype_prefix",
+ "xox-not-a-slack-token",
+ "just some normal log line with no secrets in it at all",
+ ],
+ )
+ def test_benign_strings_are_not_redacted(self, masker, benign):
+ masker.enable_content_pattern_masking()
+ try:
+ assert masker.redact(benign) == benign
+ finally:
+ masker.disable_content_pattern_masking()
+
+ def test_content_masking_composes_with_key_name_masking(self, masker):
+ # A dict whose key name is sensitive triggers the existing recursive
+ # redaction; content-pattern masking on top must not regress that path.
+ masker.enable_content_pattern_masking()
+ try:
+ data = {"password": "some_user_password", "log_line":
"token=AKIAIOSFODNN7EXAMPLE end"}
+ redacted = masker.redact(data)
+ assert redacted["password"] == "***"
+ assert redacted["log_line"] == "token=*** end"
+ finally:
+ masker.disable_content_pattern_masking()
+
+ def test_content_masking_composes_with_explicit_add_mask(self, masker):
+ # A secret registered via add_mask() must still be redacted alongside
+ # a same-string pattern-detected secret in the same value.
+ masker.enable_content_pattern_masking()
+ try:
+ masker.add_mask("my-custom-secret-1234")
+ text = "my-custom-secret-1234 and gh" + "p_" + "z" * 40
+ redacted = masker.redact(text)
+ assert "my-custom-secret-1234" not in redacted
+ assert "ghp_" not in redacted
+ assert redacted.count("***") == 2
+ finally:
+ masker.disable_content_pattern_masking()
+
+ def test_add_content_patterns_registers_and_masks(self, masker):
+ masker.enable_content_pattern_masking()
+ try:
+ masker.add_content_patterns({"acme_key": r"\bACME-[A-Z0-9]{8}\b"})
+ assert masker.redact("id=ACME-ABCD1234 done") == "id=*** done"
+ finally:
+ masker.disable_content_pattern_masking()
+
+ def test_add_content_patterns_ignores_invalid_regex(self, masker, caplog):
+ # Invalid regex must not raise or break the existing pattern set.
+ masker.enable_content_pattern_masking()
+ try:
+ masker.add_content_patterns({"broken": "([unterminated"})
+ # Known patterns still work after a bad entry was rejected.
+ assert masker.redact("AKIAIOSFODNN7EXAMPLE") == "***"
+ finally:
+ masker.disable_content_pattern_masking()
+
+ def test_reset_masker_restores_default_content_patterns(self, masker):
+ masker.enable_content_pattern_masking()
+ try:
+ masker.add_content_patterns({"custom_x": r"\bCUSTOMX-[0-9]{4}\b"})
+ assert masker.redact("CUSTOMX-1234") == "***"
+ masker.reset_masker()
+ # Custom pattern gone.
+ assert masker.redact("CUSTOMX-1234") == "CUSTOMX-1234"
+ # Built-in defaults restored.
+ assert masker.redact("AKIAIOSFODNN7EXAMPLE") == "***"
+ finally:
+ masker.disable_content_pattern_masking()
+
+ def test_default_pattern_set_matches_known_secret_patterns_constant(self,
masker):
+ # Guardrail so a rename of the module-level constant does not silently
+ # drop the default set from newly-constructed maskers.
+ assert set(masker._content_pattern_sources) ==
set(KNOWN_SECRET_PATTERNS)
+
+ def test_log_filter_masks_content_pattern_without_registered_secret(self,
caplog):
Review Comment:
This asserts on `caplog.text` (we're moving away from that — please use
structured `caplog` assertions) and calls `dictConfig` without restoring the
previous config afterwards.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
shared/secrets_masker/tests/secrets_masker/test_secrets_masker.py:
##########
@@ -1611,3 +1612,184 @@ def test_k8s_objects_still_detected_when_imported(self):
# Should be redacted since "password" is a sensitive field name
assert redacted["value"] == "***"
assert redacted["name"] == "password"
+
+
+class TestContentPatternMasking:
+ """Value-content pattern masking for well-known credential formats."""
+
+ @pytest.fixture
+ def masker(self):
+ m = SecretsMasker()
+ configure_secrets_masker_for_test(m)
+ return m
+
+ @pytest.mark.parametrize(
+ ("label", "sample"),
+ [
+ ("aws_access_key", "AKIAIOSFODNN7EXAMPLE"),
+ ("aws_session_key", "ASIAY34FZKBOKMUTVV7A"),
+ ("github_pat", "ghp_" + "a" * 40),
+ ("github_oauth", "gho_" + "b" * 40),
+ ("slack_bot", "xoxb-1234567890-abcdefghij"),
+ ("slack_user", "xoxp-1234567890-0987654321-abcdefghij"),
+ ("google_api_key", "AIza" + "A" * 35),
+ ("stripe_live_key", "sk_live_" + "0" * 24),
+ (
+ "jwt",
+
"eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.dozjgNryP4J3jVmNHl0w5N_XgL0n3I9PlFUP0THsR8U",
+ ),
+ ],
+ )
+ def test_known_pattern_is_redacted_when_enabled(self, masker, label,
sample):
+ masker.enable_content_pattern_masking()
+ try:
+ text = f"prefix {sample} suffix"
+ assert masker.redact(text) == "prefix *** suffix"
+ finally:
+ masker.disable_content_pattern_masking()
+
+ def test_pem_private_key_block_is_redacted_when_enabled(self, masker):
+ masker.enable_content_pattern_masking()
+ try:
+ pem = (
+ "-----BEGIN RSA " + "PRIVATE KEY-----\n"
+
"MIIBOgIBAAJBAKj34GkxFhD90vcNLYLInFEX6Ppy1tPf9Cnzj4p4WGeKLs1Pt8Qu\n"
+ "-----END RSA " + "PRIVATE KEY-----"
+ )
+ redacted = masker.redact(f"key: {pem} end")
+ assert redacted == "key: *** end"
+ finally:
+ masker.disable_content_pattern_masking()
+
+ def test_off_by_default(self, masker):
+ # By default, with a fresh masker and no explicit enable, an AWS-shaped
+ # value must pass through unchanged.
+ assert not masker.is_content_pattern_masking_enabled()
+ aws = "AKIAIOSFODNN7EXAMPLE"
+ assert masker.redact(aws) == aws
+
+ def test_disable_restores_passthrough(self, masker):
+ masker.enable_content_pattern_masking()
+ aws = "AKIAIOSFODNN7EXAMPLE"
+ assert masker.redact(aws) == "***"
+ masker.disable_content_pattern_masking()
+ assert masker.redact(aws) == aws
+
+ @pytest.mark.parametrize(
+ "benign",
+ [
+ "AKIA_LOOKS_LIKE_ONE",
+ "not-a-jwt.header.only",
+ "sk_live_short",
+ "gh_notatokentype_prefix",
+ "xox-not-a-slack-token",
+ "just some normal log line with no secrets in it at all",
+ ],
+ )
+ def test_benign_strings_are_not_redacted(self, masker, benign):
+ masker.enable_content_pattern_masking()
+ try:
+ assert masker.redact(benign) == benign
+ finally:
+ masker.disable_content_pattern_masking()
+
+ def test_content_masking_composes_with_key_name_masking(self, masker):
+ # A dict whose key name is sensitive triggers the existing recursive
+ # redaction; content-pattern masking on top must not regress that path.
+ masker.enable_content_pattern_masking()
+ try:
+ data = {"password": "some_user_password", "log_line":
"token=AKIAIOSFODNN7EXAMPLE end"}
+ redacted = masker.redact(data)
+ assert redacted["password"] == "***"
+ assert redacted["log_line"] == "token=*** end"
+ finally:
+ masker.disable_content_pattern_masking()
+
+ def test_content_masking_composes_with_explicit_add_mask(self, masker):
+ # A secret registered via add_mask() must still be redacted alongside
+ # a same-string pattern-detected secret in the same value.
+ masker.enable_content_pattern_masking()
+ try:
+ masker.add_mask("my-custom-secret-1234")
+ text = "my-custom-secret-1234 and gh" + "p_" + "z" * 40
+ redacted = masker.redact(text)
+ assert "my-custom-secret-1234" not in redacted
+ assert "ghp_" not in redacted
+ assert redacted.count("***") == 2
+ finally:
+ masker.disable_content_pattern_masking()
+
+ def test_add_content_patterns_registers_and_masks(self, masker):
+ masker.enable_content_pattern_masking()
+ try:
+ masker.add_content_patterns({"acme_key": r"\bACME-[A-Z0-9]{8}\b"})
+ assert masker.redact("id=ACME-ABCD1234 done") == "id=*** done"
+ finally:
+ masker.disable_content_pattern_masking()
+
+ def test_add_content_patterns_ignores_invalid_regex(self, masker, caplog):
Review Comment:
This takes `caplog` but doesn't check that the warning for the invalid
pattern is actually logged.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
