semgrep: add disabled-rules knob + narrower MD5 crypto-context rule#23
Merged
Merged
Conversation
Two related changes that let consumers cut noise from over-broad registry-pack rules without losing real coverage: 1. New `disabled-rules` input on the reusable workflow + composite action — comma-separated list of Semgrep rule IDs (dotted check_id) forwarded to `semgrep scan --exclude-rule`. Intended for org-wide registry-pack noise where the rule's threat model doesn't apply to SC code (e.g. MD5-as-fingerprint, untyped-yaml-unmarshal for log obfuscation). Per-call false positives should still use inline `// nosemgrep:` rather than this knob. 2. New `go-md5-in-crypto-context` rule in go-canon.yml that fires ONLY when MD5 is used in a real cryptographic context: inside `hmac.New(md5.New, ...)`, or assigned to a variable whose name suggests integrity/auth (`mac`, `hmac`, `sig`, `signature`, `tag`, `auth`, `token`, `password`, `digest`). MD5 used as a non-cryptographic content fingerprint — S3 etag, K8s annotation change-hash, dedup suffix, cache buster — is NOT flagged here. This is the rule that backs up disabling the over-broad gosec `use-of-md5` at the consumer level. 3. Updated the existing yaml-unmarshal-into-interface note with a cross-ref explaining why consumers should disable the registry's `unsafe-deserialization-interface`: the rule is calibrated to Python pickle/yaml.load (which executes on unmarshal); Go's yaml.v3/encoding-json do not execute, so the threat model doesn't transfer. Self-tests: 34/34 cases on go-canon.yml (was 26/26, +8 fixtures for the new rule covering hmac.New(md5.New), 3 sensitive-name positives, and 4 fingerprint-style negatives). Consumer wiring lands in a follow-up PR on each repo that opts into `disabled-rules`. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Security Scan ResultsRepository:
Scanned at 2026-05-24 15:30 UTC |
Semgrep Scan ResultsRepository:
Scanned at 2026-05-24 15:30 UTC |
4 tasks
Cre-eD
added a commit
to simple-container-com/api
that referenced
this pull request
May 24, 2026
…verage (#291) ## Summary Cuts the 10 Semgrep WARNING findings the job has been posting on every PR (and on #289) by disabling two registry-pack rules whose threat models don't apply to SC code. Reachability-aware replacement coverage ships in [actions#23](simple-container-com/actions#23). ## Findings being silenced All 10 are pre-existing intentional patterns: **MD5-as-fingerprint (7)** — `use-of-md5` fires on every `md5.Sum()` regardless of context. SC call sites are: | File | Use | |---|---| | [pkg/clouds/pulumi/aws/static_website.go:166](pkg/clouds/pulumi/aws/static_website.go#L166) | upload-bundle checksum (S3 etag-style) | | [pkg/clouds/pulumi/gcp/bucket_uploader.go:55](pkg/clouds/pulumi/gcp/bucket_uploader.go#L55) | upload-bundle checksum (S3 etag-style) | | [pkg/clouds/pulumi/gcp/gke_autopilot_stack.go:243,251](pkg/clouds/pulumi/gcp/gke_autopilot_stack.go#L243), [pkg/clouds/pulumi/kubernetes/kube_run.go:225,233](pkg/clouds/pulumi/kubernetes/kube_run.go#L225) | K8s annotation hash for Caddy restart trigger | | [pkg/clouds/pulumi/cloudflare/registrar.go:71](pkg/clouds/pulumi/cloudflare/registrar.go#L71) | dedup suffix for Pulumi resource names on duplicate-name DNS records | **yaml-into-interface{} (3)** — `unsafe-deserialization-interface` is calibrated to Python pickle/yaml.load (executes on unmarshal). Go's `yaml.v3` / `encoding/json` don't execute. SC call sites: | File | Use | |---|---| | [pkg/assistant/chat/commands_project.go:943](pkg/assistant/chat/commands_project.go#L943) | obfuscate embedded YAML credentials before logging | | [pkg/assistant/security/secure_file_reader.go:93,339](pkg/assistant/security/secure_file_reader.go#L93) | redact secrets in user YAML before logging | The SC team already documented this exact reasoning in `go-canon.yml` round-6 triage, where they dropped their own equivalent yaml-unmarshal rule. ## What still catches the real threats Disabling these two rules does NOT lose coverage; the genuine patterns are caught by narrower rules in `go-canon.yml`: - **MD5 in real crypto context** — new `go-md5-in-crypto-context` rule (actions#23) fires on `hmac.New(md5.New, ...)` and on md5 result assigned to a variable named `mac` / `hmac` / `signature` / `tag` / `auth` / `token` / `password` / `digest`. - **Untrusted-source → unmarshal → exec** — taint-aware coverage in `p/golang` registry pack remains enabled. Only the generic shape-only rule is disabled. ## Dependencies ⚠ **Pinned to a branch SHA on actions.** The reusable-workflow ref currently points at `2a7604ddac4fd9f912a1ab6896ac87c5b07bc537` (the head of actions#23). Before merging this PR, **bump the pin to the main-merge SHA** that actions#23 produces. ## Test plan - [ ] actions#23 merges and CI is green - [ ] Bump reusable-workflow SHA pin to actions main-merge commit - [ ] CI re-run: Semgrep posts 0 WARNING findings - [ ] Spot-check: `go-md5-in-crypto-context` still fires if someone writes `hmac.New(md5.New, key)` — verified in actions#23 fixtures (8 new cases) --------- Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related changes that let consumer repos cut noise from over-broad Semgrep registry-pack rules without losing real security coverage:
New
disabled-rulesinput on both the composite action (semgrep-scan/action.yml) and the reusable workflow (.github/workflows/semgrep.yml). Comma-separated list of Semgrep rule IDs (full dottedcheck_id), forwarded tosemgrep scan --exclude-rule. Validated against[A-Za-z0-9._-]token regex.New
go-md5-in-crypto-contextrule ingo-canon.ymlthat fires ONLY when MD5 is used cryptographically:hmac.New(md5.New, ...)mac,hmac,sig,signature,tag,auth,token,password,digestMD5 used as a non-cryptographic content fingerprint (S3 etag, K8s annotation change-hash, dedup suffix, cache buster) is not flagged. This is the focused replacement for the broad gosec
use-of-md5rule, which fires on everymd5.Sum(...)regardless of context.Updated yaml-unmarshal note with a cross-ref explaining why consumers should disable the registry's
unsafe-deserialization-interface: it's calibrated to Python pickle / yaml.load (which executes on unmarshal). Go'syaml.v3/encoding/jsondon't execute; threat model doesn't transfer.Why this matters
The
apirepo's Semgrep job currently reports 10 WARNING findings on every PR — 7 MD5-as-fingerprint and 3 yaml-unmarshal-for-log-obfuscation. All are documented intentional patterns:yaml.Unmarshal(_, &interface{})for obfuscating credentials before loggingThe team already documented in
go-canon.ymlthat they intentionally dropped their own equivalent yaml-unmarshal rule for the same reason. The newdisabled-rulesknob lets consumers disable the noisy registry rule at the call site, and the narrowergo-md5-in-crypto-contextrule covers the genuine threat (HMAC-MD5, MD5 password hashing, MD5 signing).Self-tests
go-canon.yml(was 26/26; +8 fixtures: 4 positives —hmac.New(md5.New),mac := md5.New(),signature := md5.Sum(...),password := md5.Sum(...)— and 4 negatives —checksum,etag,hashBytesfor K8s annotation, inlinereturn md5.Sum(...)).Follow-up
Once merged, consumer repos that opt in will bump their workflow's SHA pin and pass
disabled-rules:for the two registry IDs:go.lang.security.audit.crypto.use_of_weak_crypto.use-of-md5go.lang.security.deserialization.unsafe-deserialization-interface.go-unsafe-deserialization-interfaceA follow-up PR will wire this into
simple-container-com/api.Test plan
run-tests.shgreen: 34/34 canon cases, full-repo scan 0 findingsapiPR drops to 0 Semgrep WARNING findings after consumer wiring