Conversation
…5 exclusion audit.yml ran on schedule and workflow_dispatch only, so gosec and govulncheck never gated a pull request and the audit learned what had merged on the following Monday. auth/totp merged this morning and shipped in v1.13.0. The first scheduled audit after it went red on auth/totp/totp.go:52 with G505, crypto/sha1. The tag was already cut. A pull_request trigger is what stops a merge outrunning the audit by a week; the schedule still earns its place, because it is what catches an advisory published after a merge. The SHA-1 stays. RFC 6238 defines TOTP over HMAC-SHA1 and every authenticator app implements it, so anything else is a module nobody can pair with, and collision attacks do not carry to HMAC. The import was already annotated, but standalone gosec ignores //nolint pragmas, which is why G104, G306, G115 and G101 are excluded despite carrying the same annotations. G505 was never added. A repository-wide exclusion with nothing watching it is a control that reads as present and permits everything, so sha1_import_test.go bounds it: crypto/sha1 is allowed in auth/totp and nowhere else, and an allowance that stops describing the tree fails too, so the exclusion gets removed with the import rather than outliving it. I checked it fails in the direction that matters: adding a blank crypto/sha1 import to auth/jwt turns it red naming the file, and the sabotage compiled. Worth recording separately, since it is the reason this went unseen: audit freshness has been failing on every pull request for a month and is not a required check. Before today the audit was red on three consecutive weeks for real govulncheck findings, GO-2026-6218, GO-2026-6090 and GO-2026-5972, now closed by the go 1.26.6 directive. Making that check required is a ruleset change and stays open in #354. Closes #354 Signed-off-by: Jose <75870284+Jaro-c@users.noreply.github.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.
audit.ymlran onscheduleandworkflow_dispatchonly, so gosec and govulncheck never gated a pull request and the audit learned what had merged on the following Monday.That stopped being theoretical this morning.
auth/totpmerged (#335), importscrypto/sha1, and shipped in v1.13.0. The first scheduled audit after it, at 11:51 UTC, went red onauth/totp/totp.go:52withG505 (CWE-327). The tag was already cut.Both triggers earn their place and neither replaces the other:
pull_requeststops a merge outrunning the audit by a week, and the schedule is what catches an advisory published after a merge.The SHA-1 stays
RFC 6238 defines TOTP over HMAC-SHA1 and every authenticator app implements that, so a module using anything else is one nobody can pair with, and SHA-1 collision attacks do not carry to HMAC. The import was already annotated in code. Standalone gosec ignores
//nolintpragmas, which is exactly why G104, G306, G115 and G101 sit ingosec-excludedespite carrying the same annotations. G505 was never added whenauth/totplanded.Bounding the exclusion
gosec-excludeis repository-wide, so silencing G505 forauth/totpsilences it for everything, and an exclusion with nothing watching it is the shape this repository keeps finding: a control that reads as present and permits everything.sha1_import_test.gobounds it.crypto/sha1is allowed inauth/totpand nowhere else, so a second package reaching for it fails on its own pull request and has to argue for itself rather than inherit a justification written about TOTP. It fails in the other direction too: an allowance that no longer describes the tree is flagged, so the exclusion gets removed along with the import instead of outliving it.I checked it fails in the direction that matters: a blank
crypto/sha1import added toauth/jwtturns it red naming the file, and the sabotage compiled, so the red is about the import rather than a broken build.What this does not fix
audit freshnesshas been failing on every pull request for a month, correctly, and it is not a required check on develop, where the required set isdco,go / Lint & testandline-limit. Before today the audit was red for three consecutive weeks on real govulncheck findings, GO-2026-6218, GO-2026-6090 and GO-2026-5972, all reachable fromoauth.Client.Exchangeandkeymanager.decodeEd25519PublicPEM, now closed by the go 1.26.6 directive. A check that reports a real problem on every pull request and blocks nothing is one people learn to scroll past, and that is how three weeks of live standard library vulnerabilities stayed on the board.Making it required is a ruleset change, so it is yours rather than mine. #354 carries it.
Closes #354