fix(security): resolve gosec findings and pin Go toolchain to 1.26.5 - #630
Merged
Conversation
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
github-actions
Bot
added
dependencies
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
Contributor
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #630 +/- ##
=======================================
Coverage 70.92% 70.92%
=======================================
Files 36 36
Lines 3570 3570
=======================================
Hits 2532 2532
Misses 876 876
Partials 162 162
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gosec became blocking in the shared workflow and flagged six findings here; govulncheck failed on four stdlib advisories. None of the gosec findings was a real defect, so each carries a reason that can be checked rather than a bare suppression. G402, TLS verification disabled (internal/web/server.go): opt-in, not a shipped bypass. TLSSkipVerify is a plain bool that only LDAP_TLS_SKIP_VERIFY / --tls-skip-verify sets, its zero value is false, a test pins that default, and enabling it logs a warning at startup. G704, SSRF (cmd/ldap-manager/main.go): the URL is the LDAP server address from operator configuration, never derived from request data. G404, weak randomness (internal/retry/retry.go): rand.Float64 supplies backoff jitter. It produces no token, nonce or session ID. G103, unsafe (internal/ldap_cache/cachetest/helpers.go): reflection writes to simple-ldap-go's unexported Object.dn/cn when building test fixtures. That library sets those fields only in objectFromEntry and exposes no constructor taking them, so a fixture cannot be built through its public API — filed as netresearch/simple-ldap-go#191. The scaffolding cannot move into a _test.go file the way it did in raybeam: the tests of both internal/web and internal/ldap_cache import it, and Go does not share _test.go declarations across package boundaries. It therefore stays an ordinary package, compiled in but reachable from no production path, and the comment now says why instead of asserting "test-only". govulncheck: GO-2026-5856 (crypto/tls), GO-2026-5039 (net/textproto), GO-2026-5037 (crypto/x509), GO-2026-4971 (net). The module moves to go 1.26 with toolchain go1.26.5, matching ofelia. Verified: gosec 0 issues, govulncheck 0 affecting vulnerabilities, go vet clean, tests pass. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
CybotTM
force-pushed
the
fix/gosec-and-toolchain
branch
from
July 28, 2026 18:38
e38b9c8 to
c7e4933
Compare
|
Contributor
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
CybotTM
added a commit
to netresearch/simple-ldap-go
that referenced
this pull request
Jul 28, 2026
`Object.cn` and `Object.dn` are unexported and written only by
`objectFromEntry`, so a consumer cannot build a `User`, `Group` or
`Computer` fixture with a DN through the public API. Both downstream
repos do it with reflection plus an `unsafe` write to the unexported
fields, which gosec flags as G103 now that it is blocking in the shared
workflow.
```go
// NewObject builds an Object with the given common name and distinguished name.
func NewObject(cn, dn string) Object
```
## Why a constructor, and only this one
The fields are unexported on purpose — an `Object` mirrors what the
directory returned, and it should not be editable afterwards into
something the directory never said. Setters would give that away
permanently for the sake of test fixtures. A constructor taking both
values at build time does not: after `NewObject` returns, the object is
as immutable as one decoded from an entry, and the only way to produce a
DN that disagrees with the directory is to state it explicitly at
construction, in a fixture, which is the point.
The issue left open whether `NewUser`/`NewGroup`/`NewComputer` belong
here too. They don't. Every other field on those three types is already
exported, so the embedded `Object` is the only part a caller cannot set,
and `ldap.User{Object: ldap.NewObject(cn, dn), SAMAccountName: …}`
covers the whole case with one function instead of four. Adding per-type
constructors would also mean a positional or option-based mirror of
every field on `User` — an API surface that has to track the struct
forever, to solve a problem the struct literal already solves. Same
reasoning against a separate `ldaptest` subpackage: it would be a second
import path and a second thing to version for a single three-line
function, and the constructor is legitimate outside tests anyway (any
caller assembling an `Object` from a source that is not a
`*ldap.Entry`).
The one cost is that `cn` and `dn` are both strings and adjacent, so a
swapped call compiles. The argument order follows the field order and
the issue's sketch, and the doc comment names it.
Additive and non-breaking: nothing existing changes behaviour or
signature, and the `go 1.25.0` directive is untouched.
## Tests
`TestNewObject` covers the `DN()`/`CN()` round-trip including empty and
unicode values; `TestNewObjectMatchesObjectFromEntry` asserts a
constructed Object equals one decoded from the equivalent `*ldap.Entry`;
`TestNewObjectComposesIntoFixtures` builds a `User`, a `Group` and a
`Computer` from it and reads DN/CN back through the embedded methods.
## Follow-up, not this PR
Removing the `unsafe` from [ldap-manager
`internal/ldap_cache/cachetest/helpers.go`](https://github.com/netresearch/ldap-manager/blob/main/internal/ldap_cache/cachetest/helpers.go)
and [raybeam
`internal/server/test_helpers_test.go`](https://github.com/netresearch/raybeam/blob/main/internal/server/test_helpers_test.go)
needs a simple-ldap-go release and a dependency bump in each repo first,
so it is out of scope here. ldap-manager is the one that actually gains
something: its helpers are shared across two packages and so cannot live
in a `_test.go` file, which is why that repo currently compiles `unsafe`
into the binary behind a `#nosec`
([ldap-manager#630](netresearch/ldap-manager#630)).
Closes #191
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
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.
gosec became blocking in the shared workflow (netresearch/.github#312) and flagged six findings here; govulncheck failed on four stdlib advisories. None of the gosec findings turned out to be a real defect — so each one carries a reason a reviewer can check, not a bare suppression.
internal/web/server.gocmd/ldap-manager/main.go×2internal/retry/retry.gointernal/ldap_cache/cachetest/helpers.go×2G402 is the one worth stating plainly:
TLSSkipVerifyis a plainboolthat onlyLDAP_TLS_SKIP_VERIFY/--tls-skip-verifysets. Its zero value isfalse, a test pins that default, and enabling it logs a warning at startup. Verification is not disabled unless an operator asks for it.G404:
rand.Float64supplies jitter inaddJitter. It produces no token, nonce or session ID —crypto/randwould buy nothing.G103: the helpers write simple-ldap-go's unexported
Object.dn/cnthrough reflection when building test fixtures. That library sets those fields only inobjectFromEntryand exposes no constructor taking them, so a fixture cannot be built through its public API — filed as simple-ldap-go#191.An earlier revision of this PR moved the scaffolding into a
_test.gofile, the way raybeam#254 did, which removesunsafefrom the binary entirely. That does not work here: the tests of bothinternal/webandinternal/ldap_cacheimport these helpers, and Go does not share_test.godeclarations across package boundaries. Duplicating them into each package cleared gosec but tripped SonarCloud's duplication gate at 20.8% — correctly, since it was duplication. So the shared package stays, and its comment now explains why it is an ordinary package instead of asserting "test-only". It is compiled in, reachable from no production path.govulncheck
GO-2026-5856 (crypto/tls), GO-2026-5039 (net/textproto), GO-2026-5037 (crypto/x509), GO-2026-4971 (net). The module moves to
go 1.26withtoolchain go1.26.5, matching ofelia.Verification
Issues : 0, exit 0go build,go vet,go test ./...all pass