Skip to content

Conversation

zricethezav
Copy link
Collaborator

@zricethezav zricethezav commented Jul 1, 2025

Description:

Screenshot 2025-07-01 at 3 45 15 PM

Example composite rule

[[rules]]
id = "aws-access-key"
description = "AWS Access Key ID - requires secret key and region"
regex = '''AKIA[0-9A-Z]{16}'''
keywords = ["AKIA"]
[[rules.requires]]
id = "aws-secret-key"
within_lines = 20
[[rules.requires]]
id = "aws-region"
within_lines = 20

[[rules]]
id = "aws-secret-key"
description = "AWS Secret Access Key"
regex = '''[A-Za-z0-9/+=]{40}'''
keywords = ["secret", "key"]
skipReport = true

[[rules]]
id = "aws-region"
description = "AWS Region"
regex = '''(us|eu|ap|ca|sa)-(north|south|east|west|central)-[0-9]'''
keywords = ["region", "us-", "eu-", "ap-"]
skipReport = true

Fun diagrams for the visual learners (also seen in the readme)

    Fragment-level matching:               
    Any auxiliary finding within           
    the fragment boundary                  
    matches.                               
          ┌────────┐                       
   ┌──────┤fragment├─────┐                 
   │      └──────┬─┤     │ ┌───────┐       
   │             │a│◀────┼─│✓ MATCH│       
   │          ┌─┐└─┘     │ └───────┘       
   │┌─┐       │p│        │                 
   ││a│    ┌─┐└─┘        │ ┌───────┐       
   │└─┘    │a│◀──────────┼─│✓ MATCH│       
   └─▲─────┴─┴───────────┘ └───────┘       
     │    ┌───────┐                        
     └────│✓ MATCH│                        
          └───────┘                        
                                           
                                           
   example:                                
   `within_columns = 3`                    
          ┌────────┐                       
   ┌────┬─┤fragment├─┬───┐                 
   │      └──────┬─┤     │ ┌───────────┐   
   │    │        │a│◀┼───┼─│+1C ✓ MATCH│   
   │          ┌─┐└─┘     │ └───────────┘   
   │┌─┐ │     │p│    │   │                 
┌──▶│a│  ┌─┐  └─┘        │ ┌───────────┐   
│  │└─┘ ││a│◀────────┼───┼─│-2C ✓ MATCH│   
│  │       ┘             │ └───────────┘   
│  └── -3C ───0C─── +3C ─┘                 
│  ┌─────────┐                             
│  │ -4C ✗ NO│                             
└──│  MATCH  │                             
   └─────────┘                             
                                           
                                           
   example:                                
   `within_lines = 4`                      
         ┌────────┐                        
   ┌─────┤fragment├─────┐                  
  +4L─ ─ ┴────────┘─ ─ ─│                  
   │                    │                  
   │              ┌─┐   │ ┌────────────┐   
   │         ┌─┐  │a│◀──┼─│+1L ✓ MATCH │   
   0L  ┌─┐   │p│  └─┘   │ ├────────────┤   
   │   │a│◀──┴─┴────────┼─│-1L ✓ MATCH │   
   │   └─┘              │ └────────────┘   
   │                    │ ┌─────────┐      
  -4L─ ─ ─ ─ ─ ─ ─ ─┌─┐─│ │-5L ✗ NO │      
   │                │a│◀┼─│  MATCH  │      
   └────────────────┴─┴─┘ └─────────┘      
                                           
                                           
   example:                                
   `within_lines = 4`                      
   `within_columns = 3`                    
         ┌────────┐                        
   ┌─────┤fragment├─────┐                  
  +4L   ┌└────────┴ ┐   │                  
   │            ┌─┐     │ ┌───────────────┐
   │    │       │a│◀┼───┼─│+2L/+1C ✓ MATCH│
   │         ┌─┐└─┘     │ └───────────────┘
   0L   │    │p│    │   │                  
   │         └─┘        │                  
   │    │           │   │ ┌────────────┐   
  -4L    ─ ─ ─ ─ ─ ─┌─┐ │ │-5L/+3C ✗ NO│   
   │                │a│◀┼─│   MATCH    │   
   └───-3C────0L───+3C┴─┘ └────────────┘   

Question for the community:

  1. Thoughts on the terms required, skipReport, Auxiliary?
  2. What rules should be updated to be composite rules?
  3. Do we need skipReport?

Checklist:

  • Does your PR pass tests?
  • Have you written new tests for your changes?
  • Have you lint your code locally prior to submission?

@bplaxco
Copy link
Contributor

bplaxco commented Jul 3, 2025

Thoughts on the terms required, skipReport, Auxiliary?

  • skipReport - sounds good to me! 👍
  • Auxiliary - I'd prefer RelatedFindings but I'm not totally against Auxiliary. ^_^ Rationale:
    • If skipReport = False then they're not really additional findings, they just happen to be related.
    • If there are more ways to relate the findings in the future, then we already have a place to put them and we can just note the relationship in the report somewhere.
  • requires - I can also dig it 👌 the only other thing that would come to mind would be something like this, but I think it's probably scope creep:
    [[rules.related]]
       id = "aws-secret-key"
       withinLines = 20
       required = true

Do we need skipReport?

I think it's great and will be really helpful for one of my use cases ^_^. In v9 I would like to see Secret -> Target (or something similar) on finding since some of the findings from these compost rules might pull in things that aren't actually secrets they're just related information needed to verify the secret.

config/config.go Outdated
Comment on lines 67 to 68
WithinLines *int `mapstructure:"within_lines"`
WithinColumns *int `mapstructure:"within_columns"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to do camel case to match the format of the other settings values? (though I do prefer snake case).

Suggested change
WithinLines *int `mapstructure:"within_lines"`
WithinColumns *int `mapstructure:"within_columns"`
WithinLines *int `mapstructure:"withinLines"`
WithinColumns *int `mapstructure:"withinColumns"`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah shoot, you're right. Looks like we have regexTarget not regex_target. Consistency wins this one, camel case it is

config/config.go Outdated
Comment on lines 178 to 180
// if _, ok := rulesMap[r.RuleID]; !ok {
// return Config{}, fmt.Errorf("%s: [[rules.required]] rule ID '%s' does not exist", cr.RuleID, r.RuleID)
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

I kinda like this check. Any reason for commenting it out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It needs to happen after all the rules have been seen otherwise it'll flag an err

@@ -360,6 +360,10 @@ func (d *Detector) detectRule(fragment Fragment, newlineIndices [][]int, current
}()
)

if r.SkipReport == true && !fragment.InheritedFromFinding {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if r.SkipReport == true && !fragment.InheritedFromFinding {
if r.SkipReport && !fragment.InheritedFromFinding {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should signal to folks that this code is 100% handcrafted by human hands and not generated by an LLM

@@ -68,3 +98,29 @@ func maskSecret(secret string, percent uint) string {

return secret[:lth] + "..."
}

func (f *Finding) PrintAuxiliaryFindings() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this here instead of util because auxiliaryFindings is private? Would it make sense for the all the Print* functions to go in report since it seems like they're more focused on reporting info back to the user instead of the detection itself?

Comment on lines +59 to +60
WithinLines *int
WithinColumns *int
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish go had an easy way to set defaults for struct values like this. Then you could set it to -1 and wouldn't have to deference pointers to ints.

detect/detect.go Outdated
inheritedFragment.InheritedFromFinding = true

// Call detectRule once for each required rule
requiredFindings := d.detectRule(inheritedFragment, newlineIndices, currentRaw, rule, encodedSegments)
Copy link
Contributor

@bplaxco bplaxco Jul 3, 2025

Choose a reason for hiding this comment

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

If two rules have the same required rule, would d.detectRule get called twice for that rule?

Would something like this save extra scans and passes to see if the reqs are met? (doing it in python/pseudo code):

def detect_with_reqs(rule:Rule, fragment:Fragement, findings_by_rule:dict[str,list[Finding]], to_report:list[Finding]) -> list[Finding]:
    # Already processed just return the findings
    if rule.id in findings_by_rule:
        return findings_by_rule[rule.id]

    # Run detections for this rule
    findings = detect(rule, fragment, ...)

    # If there's nothing left to do, add the findings to the report set and
    # return the findings
    if not findings or not rule.requires:
        if not rule.skip_report:
            to_report.extend(findings)
        return findings

    # Handle the requirements
    for req in rule.requires:
        req_rule = rules.config[req.id]

        # This handles the req tree recursively
        req_findings = detect_with_reqs(req_rule, fragment, findings_by_rule, to_report)

        # No need to go further if a requirement didn't have findings
        if not req_findings:
            return []

        # Now check if the reqs' findings meet the criteria
        for finding in findings:
            # Filter the req_findings to the one that meet the proximity requirement
            req_findings = within_proximity(req, finding, req_findings)

            # No need to go further given a requirement wasn't met
            if not req_findings:
                return []

            # Requirement met! add the findings
            finding.add_aux_findings(req_findings)

    return findings


def detect(fragment):
    to_report = []
    findings_by_rule = {}

    for rule in config.rules.values():
        # These cases will be handled by detect_with_reqs
        if rule.skip_report:
            continue

        _ = detect_with_reqs(rule, fragment, findings_by_rule, to_report)

    return to_report

@bufferoverflow
Copy link
Contributor

I really like this approach!

  1. Thoughts on the terms required, skipReport, Auxiliary?

What about naming Auxiliary more like the definition itself, e.g. RelatedRuleId or RquiredRuleId to have it more inline with the definition?

  1. What rules should be updated to be composite rules?

The aws-access-token rule or maybe just create a new one aws-credentials

  1. Do we need skipReport?

I would say yes, as otherwise reports get flooded.

Another alternative is to just use those definitions inline only so that they are only used by the composite rule and not as a regular rule. Could also be beneficial as you typically use those other roles only as part of a composite rule, e.g. like this:

[[rules]]
id = "aws-access-key"
description = "AWS Access Key ID - requires secret key and region"
regex = '''AKIA[0-9A-Z]{16}'''
keywords = ["AKIA"]
[[required]]
id = "aws-secret-key"
description = "AWS Secret Access Key"
regex = '''[A-Za-z0-9/+=]{40}'''
keywords = ["secret", "key"]
within_lines = 20
[[required]]
id = "aws-region"
description = "AWS Region"
regex = '''(us|eu|ap|ca|sa)-(north|south|east|west|central)-[0-9]'''
keywords = ["region", "us-", "eu-", "ap-"]
within_lines = 20

@zricethezav
Copy link
Collaborator Author

whelp in she goes

@zricethezav zricethezav merged commit b1c9c7e into master Jul 20, 2025
5 checks passed
@zricethezav zricethezav deleted the composite-rules branch July 20, 2025 16:13
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.

3 participants