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

Use crypto/rand instead of math/rand #359

Merged
merged 1 commit into from
Oct 24, 2023
Merged

Conversation

dkess
Copy link

@dkess dkess commented Oct 23, 2023

math/rand isn't suitable for security-sensitive contexts. This PR replaces it with crypto/rand (which uses the somewhat inconvenient big.Int type).

@Sean-Der
Copy link
Member

Nice catch! Thank you @dkess

MInd using randutil.GenerateCryptoRandomString

After pion/turn was created we started moving our duplicated random/crypto logic to this repo. Sorry to make you do the extra work :/

@dkess
Copy link
Author

dkess commented Oct 24, 2023

No problem. Updated the PR. Thanks for the pointer.

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Files Coverage Δ
server.go 65.35% <0.00%> (-0.52%) ⬇️
internal/server/turn.go 58.60% <0.00%> (-0.66%) ⬇️
internal/server/util.go 54.44% <42.85%> (+3.89%) ⬆️

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

Use crypto/rand and pion/randutil instead of math/rand

math/rand isn't suitable for security-sensitive contexts, so replace it
with cryptographically secure random string generation.
@Sean-Der Sean-Der merged commit 1f1673d into pion:master Oct 24, 2023
14 of 15 checks passed
@Sean-Der
Copy link
Member

merged!

@dkess any interest in staying involved? I would love to add you to the Pion org. If you have other things that could be improved with this repo, would love to support you :)

@dkess
Copy link
Author

dkess commented Oct 24, 2023

Happy to be part of the org :)

I don't know of any improvements yet, but I do plan to use the ICE library for an upcoming project at my workplace. I'll definitely be keeping an eye on the project and plan to participate as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants