Unsolicited code review: crypto
While searching how to load certificates and keys from memory for the FreeRTOS server, or adding PubSub security...
sopc_key_manager.c
-
SOPC_KeyManager_SerializedAsymmetricKey_CreateFromData
is unused (hence untested), asSOPC_KeyManager_AsymmetricKey_CreateFromBuffer
is usually used in tests:- this function should be used in the near future, it is relevant to add unit tests.
-
SOPC_KeyManager_SerializedCertificate_CreateFromDER
usesSOPC_Buffer_Create
but it would be simplier to useSOPC_Buffer_Attach
, -
SOPC_KeyManager_SerializedAsymmetricKey_CreateFromData
could useSOPC_SecretBuffer_NewFromExposedBuffer
, -
SOPC_KeyManager_Certificate_GetThumbprint
expects a pre-allocated buffer, but the function is only used in aSOPC_Buffer
context; moreover it only operates on a Certificate and not on a CertificateList.
sopc_crypto_profiles.h
-
FnSymmetricEncrypt
andFnSymmetricDecrypt
are very similar types, and may be factorized, - Same with
FnSymmetricSign
andFnSymmetricVerify
, - Same with
FnAsymmetricEncrypt
,FnAsymmetricSign
, andFnAsymmetricVerify
, - Can
sopc_g_cp*
be moved to.c
?
mbedtls/crypto_functions_lib.c
-
CryptoProvider_SymmVerify_*
are very similar toCryptoProvider_SymmSign_*
but do not use them, - Verify context deletion in case of abusive early returns (e.g.
CryptoProvider_SymmEncrypt_AES256
), - Factorize
CryptoProvider_SymmVerify_HMAC_SHA*
functions: they differ only by a constant, -
struct SOPC_Certificate { mbedtls_x509_crt crt; /**< Certificate as a lib-dependent format */ uint8_t* crt_der; /**< Certificate in the DER format, which should be canonical. Points to internal mbedtls buffer.*/ uint32_t len_der; /**< Length of crt_der. */ };
crt_der
points to the certificate in the mbedtls structure, which could be used to actually have aCertificate_Copy
, and render useless theSerializedCertificate
structure, -
SOPC_KeyManager_AsymmetricKey_ToDER
is tested but unused, as the OPC UA protocol only requires certificate exchanges.
sopc_crypto_provider.[hc]
-
SOPC_CryptoProvider
is defined in the header, and is not opaque, - Can
SOPC_CryptoProvider_Init
andDeinit
bestatic
? - Have
const SOPC_SecretBuffer *pKey
.
tests/helpers/sopc_crypto_certificates.h
- There are serialized certificates that look like the same as in
tests/services/static_security_data.h
, should we homogenize and keep only one of the duplicates?
Thread safety of the PKI
The PKI is not thread safe.
The only use for now is in the chunk manager, through SOPC_CryptoProvider_Certificate_Validate
.
It is always present in SOPC_SecureChannel_Config
, even when it will not be used (the config created by the server, see SC_ServerTransition_ScInit_To_ScConnecting
).
Maybe it will be used in the future to implement x509 user identity tokens.
The question: keep the reference in SC_ServerTransition_ScInit_To_ScConnecting
or not?
PKIProvider_Create
- Function
_Create
is heterogeneous; it expects serialized certificates but non serialized CRL, - Using Certificates instead of SerializedCertificates should simplify memory management and
ServerConfig
(see thread safety), - Flag
WITH_STATIC_SECURITY_DATA
should not be used beforetoolkit_test_server
is cleaned and the previous problem solved.
Edited by Pierre-Antoine BRAMERET