Thanks for reviewing!

On 23-02-18 10:17, Antonio Quartulli wrote:
> On 07/02/18 20:22, Steffan Karger wrote:
>> -        mbedtls_sha256(cert->tbs.p, cert->tbs.len, sha256_hash, false);
>> +        if (0 != md_full(sha256_kt, cert->tbs.p, cert->tbs.len, 
>> sha256_hash))
> Why not using mbedtls_sha256_ret() since we are already in
> mbedtls-specific code here?
> Any advantage in using a wrapper for another wrapper? :-P
> (mbedtls_sha256_ret() is also the suggested replacement for
> mbedtls_sha256())
> Moreover, SHA256 is statically selected, therefore using
> mbedtls_sha256_ret() would also avoid the md_kt_t local variable.

This was the only place in the code where we took a (direct) dependency
on functions from sha256.h.  By using our internal wrapper, I could also do:

@@ -60,7 +60,6 @@

 #include <mbedtls/oid.h>
 #include <mbedtls/pem.h>
-#include <mbedtls/sha256.h>

Since reducing dependencies usually reduces maintenance burden, I prefer
this solution.

>> +        {
>> +            msg(M_WARN, "WARNING: failed to personalise random");
>> +        }
>> +
> Since we now have a reason for the failure, may it make sense to print a
> proper description based on the return value? (even though I think
> mbedtls_sha256_ret() can't really return something different from 0)

md_full returns true/false, but you're right we can improve error
reporting.  Are you satisfied if I add an mbed_ok() wrapper *inside*
md_full() to print the internal mbed error?  I think that should be
sufficient information.


Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Openvpn-devel mailing list

Reply via email to