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

Why does HMACStrategy.Generate uses a lock? #803

Open
3 of 5 tasks
mitar opened this issue Mar 10, 2024 · 3 comments
Open
3 of 5 tasks

Why does HMACStrategy.Generate uses a lock? #803

mitar opened this issue Mar 10, 2024 · 3 comments
Labels
bug Something is not working.

Comments

@mitar
Copy link
Contributor

mitar commented Mar 10, 2024

Preflight checklist

Ory Network Project

No response

Describe the bug

While reading the code, I noticed that HMACStrategy.Generate uses a mutex lock, but I do not get why it exists. No code there has any global or local state?

This was introduced in b4b9be5, but I also do not get why.

Given that this is used a lot, I think mutex could be removed?

Reproducing the bug

N/A

Relevant log output

No response

Relevant configuration

No response

Version

latest master

On which operating system are you observing this issue?

None

In which environment are you deploying?

None

Additional Context

No response

@mitar mitar added the bug Something is not working. label Mar 10, 2024
@james-d-elliott
Copy link
Contributor

I suspect it may have been implemented at a time where there was a lot of information about it being unsafe for concurrent usage, which I believe from memory was false and it was based on a couple articles from prominent bloggers who misinterpreted the issues with math/rand's global Read func instead of crypto/rand's global Reader.

But I can't say for sure.

@mitar
Copy link
Contributor Author

mitar commented Mar 10, 2024

You mean RandomBytes? But then the lock could be in RandomBytes?

@james-d-elliott
Copy link
Contributor

Yeah I'm not sure, I would have thought so too.. I can't see anything else unless the crypto module being used at the time needed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working.
Projects
None yet
Development

No branches or pull requests

2 participants