Skip to content

Fix key-chain key ID false positive in password anonymization#247

Merged
dhalperi merged 1 commit into
intentionet:masterfrom
manonfgoo:fix/key-chain-false-positive
Mar 24, 2026
Merged

Fix key-chain key ID false positive in password anonymization#247
dhalperi merged 1 commit into
intentionet:masterfrom
manonfgoo:fix/key-chain-false-positive

Conversation

@manonfgoo

@manonfgoo manonfgoo commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

The generic key regex matched Juniper key-chain key IDs (e.g. "key 10" in authentication-key-chains) and either anonymized them into huge invalid numbers or scrubbed the entire line.

Two fixes:

  • Capture-group regex: change \S+ to \S{7,} so only values 7+ characters are anonymized. Key IDs are at most 2 digits; real passwords are longer.
  • Scrub-only regex: require ascii-text/hexadecimal qualifier when the keyword is bare "key" (not "pre-shared-key" or "failover key").

The minimum-length pattern was evaluated for broader application across all password regexes but is not needed elsewhere: the other regexes have specific prefixes (password, secret, pre-shared-key, etc.) that already prevent false positives on short values.


This change is Reviewable

@dhalperi dhalperi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@dhalperi reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on manonfgoo).


netconan/default_pwd_regexes.py line 90 at r1 (raw file):

    # Require 7+ chars to avoid false positives on short values like key-chain
    # key IDs (e.g. "key 10" in Juniper authentication-key-chains).
    [(r"(?P<prefix>key( \d| hexadecimal)? )(\S{7,})", 3)],

I get the problem, but this feels too risky to me. Netconan doesn't try to have perfect output configs, privacy is more important than avoiding manual cleanups. (False positives better than False negatives).

If you make it 3+/4+, I'm game.

@manonfgoo

Copy link
Copy Markdown
Contributor Author

Reduced the minimum length threshold from 7+ to 3+ characters, per your feedback.

@dhalperi

Copy link
Copy Markdown
Member

Something went wrong here. It's no longer either a merge or rebase on master. Can you take a look?

Require 3+ chars after 'key' to avoid anonymizing short key-chain
key IDs (e.g. "key 10"). Also narrow the ikev1 pre-shared-key regex
to not over-match plain 'key' lines.
@manonfgoo manonfgoo force-pushed the fix/key-chain-false-positive branch from 38ac018 to f4bcc49 Compare March 24, 2026 00:11
@manonfgoo

manonfgoo commented Mar 24, 2026

Copy link
Copy Markdown
Contributor Author

Something went wrong here. It's no longer either a merge or rebase on master. Can you take a look?

pls try again

@dhalperi dhalperi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@dhalperi reviewed 20 files and all commit messages, and resolved 1 discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved.

@dhalperi dhalperi merged commit 2419365 into intentionet:master Mar 24, 2026
8 checks passed
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.

2 participants