Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

re-enable gosec/G404 #1757

Merged
merged 4 commits into from
Feb 23, 2024
Merged

re-enable gosec/G404 #1757

merged 4 commits into from
Feb 23, 2024

Conversation

vroldanbet
Copy link
Contributor

Re-enables gosec/G404 to detect usages of cryptographically unsafe math/rand, and goes over all the usages, and removes when it makes sense, or adds a // nolint directive with a justification when it does not.

@vroldanbet vroldanbet requested a review from a team as a code owner February 23, 2024 09:50
@vroldanbet vroldanbet self-assigned this Feb 23, 2024
@github-actions github-actions bot added area/api v1 Affects the v1 API area/datastore Affects the storage system area/dependencies Affects dependencies area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Feb 23, 2024
@vroldanbet vroldanbet force-pushed the replace-math-rand branch 2 times, most recently from 7923ee8 to 5878479 Compare February 23, 2024 12:40
replaces the generator using math/rand with rs/xid

Benchmarked current vs google UUID vs rs/xid

BenchmarkUUID-12     475.9 ns/op  64 B/op   2 allocs/op
BenchmarkMathRand-12 397.4 ns/op  176 B/op  2 allocs/op
BenchmarkXid-12    	 55.03 ns/op  24 B/op   1 allocs/op

xid is not cryptographically secure, but guarantees
16,777,216 unique values per second and per host/process.
That number seems higher than we expect a single
SpiceDB process to handle in terms of requests per second.
after updating the golanci-lint action
to the latest version, revive is flagging
a lot of "unused-parameter" and is
very annoying
all these uses to do not have security concerns, and
math rand is used because contention of crypto/rand
can hurt throughput/latency on the critical path.
@vroldanbet vroldanbet force-pushed the replace-math-rand branch 2 times, most recently from 4c0738d to 2bc5d90 Compare February 23, 2024 13:44
@jzelinskie jzelinskie added this pull request to the merge queue Feb 23, 2024
Merged via the queue into main with commit 7440ed3 Feb 23, 2024
20 checks passed
@jzelinskie jzelinskie deleted the replace-math-rand branch February 23, 2024 16:39
@github-actions github-actions bot locked and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/api v1 Affects the v1 API area/datastore Affects the storage system area/dependencies Affects dependencies area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants