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

Update osrng.cpp #1232

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Update osrng.cpp #1232

wants to merge 5 commits into from

Conversation

Kronix69
Copy link

Added the possibility of using Microsoft's default RNG provider, removes the need to acquire a provider (which solves the problem of not having an available provider and also saves resources)

Added the possibility of using Microsoft's default RNG provider, removes the need to acquire a provider (which solves the problem of not having an available provider and also saves resources)
@noloader
Copy link
Collaborator

Thanks @Kronix69.

Where is CRYPTOPP_DEFAULT_AES_PROVIDER configured? I only see it used in the change request.

@Kronix69
Copy link
Author

Kronix69 commented Sep 28, 2023

I momentarily left it like that so the user can enable it if they desire and so if you guys believed it to be better to enable it by default

@Kronix69
Copy link
Author

Kronix69 commented Sep 28, 2023

We could completely remove the need for a provider, I haven't encountered any issues with using the default provider so we could just erase the whole acquiring of a provider, remove the if defined for CRYPTOPP_DEFAULT_AES_PROVIDER and just make it be the standard

Made using the default provider the standard, removes the need to acquire a provider and solves issues for those who were getting an invalid handle
I made a small mistake, if the CRYPTOAPI is used we still need to use the provider
@Kronix69
Copy link
Author

Kronix69 commented Sep 30, 2023

Hey @noloader, any status on merging? I changed the code and applied the changes without the #ifdef.

@wyattoday
Copy link
Contributor

wyattoday commented Oct 5, 2023

BCRYPT_RNG_ALG_HANDLE is only defined (and usable) on Windows 10 and newer. So, this will fail if targeting older versions of Windows.

You could conditionally define BCRYPT_RNG_ALG_HANDLE and do a runtime detection of the version and optionally create the MicrosoftCryptoProvider() (and use that handle) or use the BCRYPT_RNG_ALG_HANDLE depending on what version of Windows the end-user is on.

@Kronix69
Copy link
Author

Kronix69 commented Oct 5, 2023

BCRYPT_RNG_ALG_HANDLE is only defined (and usable) on Windows 10 and newer. So, this will fail if targeting older versions of Windows.

You could conditionally define BCRYPT_RNG_ALG_HANDLE and do a runtime detection of the version and optionally create the MicrosoftCryptoProvider() (and use that handle) or use the BCRYPT_RNG_ALG_HANDLE depending on what version of Windows the end-user is on.

Done, I instead used the WINVER define which specifies the Windows version to check for Windows 10 and higher.

@Kronix69
Copy link
Author

Any status on the merge?

@Kronix69
Copy link
Author

Nevermind I realised the issue with static checking, will devise a way to runtime check

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

Successfully merging this pull request may close these issues.

3 participants