weidai11 / cryptopp

free C++ class library of cryptographic schemes
https://cryptopp.com
Other
4.66k stars 1.47k forks source link

Update osrng.cpp #1232

Open Kronix69 opened 9 months ago

Kronix69 commented 9 months ago

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 commented 9 months ago

Thanks @Kronix69.

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

Kronix69 commented 9 months ago

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 commented 9 months ago

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

Kronix69 commented 9 months ago

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

wyattoday commented 9 months ago

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 commented 9 months ago

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 commented 7 months ago

Any status on the merge?

Kronix69 commented 7 months ago

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