Skip to content

fix: prevent obfuscation false positives on common words and substrings - #114

Draft
trevorwilliams2025 wants to merge 2 commits into
openshift:mainfrom
trevorwilliams2025:fix-obfuscation-false-positives
Draft

fix: prevent obfuscation false positives on common words and substrings#114
trevorwilliams2025 wants to merge 2 commits into
openshift:mainfrom
trevorwilliams2025:fix-obfuscation-false-positives

Conversation

@trevorwilliams2025

Copy link
Copy Markdown

Summary

  • Azure resource names like "service", "network", or "proxy" discovered from ARM
    paths were being blindly replaced in all free text via strings.ReplaceAll, corrupting
    unrelated strings (e.g. "containerd.service" → "containerd.obfuscated-resource-name")
  • Keywords obfuscator had the same substring problem — a keyword like "dns" would match
    inside "coredns"

Fix

  • Keywords obfuscator: switched from strings.ReplaceAll to \b-bounded regex matching,
    so replacements respect word boundaries (underscores and digits are word characters;
    dots and hyphens are boundaries)
  • Azure resource obfuscator: added two guards for free-text replacement:
    1. replaceNotInsideWord — skips matches embedded inside larger alphabetic tokens
      (e.g. "Proxy1" inside "MyProxy1Handler")
    2. isGenericWord — skips single-case alphabetic strings ("service", "GPU") that are
      too common to safely replace in free text
    3. Short canonicals (< 5 chars) are skipped entirely ("0", "vm", "rg")

Test plan

  • New test: "service" discovered from ARM path does not corrupt "containerd.service"
  • New test: keyword "dns" does not match inside "coredns"
  • Edge case tests for: mixed-case identifiers, digit suffixes, URL-encoded strings,
    hyphenated names, multiple occurrences, embedded-in-token protection
  • Word boundary semantics verified for: dots, hyphens, underscores, colons, equals, digits
  • go build ./... and go test ./pkg/obfuscator/... pass

@openshift-ci

openshift-ci Bot commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 21, 2026
@trevorwilliams2025
trevorwilliams2025 marked this pull request as ready for review April 21, 2026 05:49
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 21, 2026
@openshift-ci
openshift-ci Bot requested review from janboll and sairameshv April 21, 2026 05:50
@openshift-ci

openshift-ci Bot commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

@trevorwilliams2025: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

)

func isLetter(b byte) bool {
return (b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what about cases like service-foo, that would be replaced to obfuscated-foo?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Replying to @janboll's comment on isLetter (line 23):

what about cases like service-foo, that would be replaced to obfuscated-foo?

This is handled by two layers of protection:

  1. genericSkipWordsservice is in the skip list (protected.go:38), so it's never replaced in free text even if discovered as an ARM resource name.

  2. End-to-end testTestAzureResourceObfuscatorContents has a case called "common word resource name does not corrupt hyphenated compounds" (azure_resources_test.go:733-741) that proves exactly this:

    • Input: ARM path discovers service as a resource name, then free text contains service-foo is ready
    • Output: service-foo is ready — unchanged

Replying to @janboll's comment on replaceNotInsideWord (line 28):

this needs more documentation or individual unit test to better understand how it's functioning

The function has been renamed to replaceStandalone and now has:

Comment thread pkg/obfuscator/azure_resources.go Outdated

// replaceNotInsideWord is like strings.ReplaceAll but skips matches embedded
// inside a larger alphabetic token (e.g. "Proxy1" inside "MyProxy1Handler").
func replaceNotInsideWord(s, old, repl string) (uint, string) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this needs more documentation or individual unit test to better understand how it's functioning

@trevorwilliams2025
trevorwilliams2025 marked this pull request as draft May 7, 2026 01:12
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 7, 2026
@openshift-ci

openshift-ci Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: trevorwilliams2025
Once this PR has been reviewed and has the lgtm label, please assign lmzuccarelli for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants