-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 26/08/16 09:19, Steffan Karger wrote:
> Hi,
>
> On 26 August 2016 at 00:49, David Sommerseth
> <[email protected]> wrote:
>> Finally some time to review this :)
>
> Great! You having more time to spend on the codebase is really
> helping the project move forward.
>
>> if (error_depth >= 0 && error_depth < MAX_CERT_DEPTH) { if
>> (!session->cert_hash_set) ALLOC_OBJ_CLEAR
>> (session->cert_hash_set, struct cert_hash_set); * if
>> (!session->cert_hash_set->ch[error_depth]) ALLOC_OBJ
>> (session->cert_hash_set->ch[error_depth], struct cert_hash); {
>> struct cert_hash *ch = session->cert_hash_set->ch[error_depth];
>> ASSERT (sizeof (ch->sha256_hash) == BLEN (cert_hash)); memcpy
>> (ch->sha256_hash, BPTR (cert_hash), sizeof (ch->sha256_hash)); }
>> }
>>
>> Look careful, the * marked if-line is only doing the ALLOC_OBJ,
>> but it looks like the {} below is part of the if statement at
>> first glance. It actually took me a few reads to see that they
>> were not related. I'm concerned this style can hide bugs in
>> future. Ideally we should avoid such local scopes when we need
>> to declare new variables, especially so close to for/while/if
>> blocks.
>
> I agree that the code is confusing this way, but I'd blame the
> missing scope of the if statements, rather than the local scope.
> Do you want me to send a follow-up patch for cleanup, or resend in
> ta 1/2 cleanup + 2/2 SHA256?
Depends on how you clean it up ;-) I've been thinking about this, and I
think just adding an extra blank line after each if() statement and
perhaps adding a comment above the {} local scope would suffice.
But if you do more thorough clean-up, it's probably better to split it
up into separate patches.
Does this make sense?
>
>>> void cert_hash_free (struct cert_hash_set *chs) { @@ -251,7
>>> +233,8 @@ cert_hash_compare (const struct cert_hash_set *chs1,
>>> const struct cert_hash_set
>>>
>>> if (!ch1 && !ch2) continue; - else if (ch1 && ch2 &&
>>> !memcmp (ch1->sha1_hash, ch2->sha1_hash, SHA_DIGEST_LENGTH)) +
>>> else if (ch1 && ch2 && !memcmp (ch1->sha256_hash,
>>> ch2->sha256_hash, + sizeof(ch1->sha256_hash)))
>>> continue; else return false;
>>
>> This looks sane. I just got uncertain if we will always have
>> SHA256 hashes from certificates, or do they need be signed with a
>> SHA256 signature by the CA? My gut feeling says me we will
>> always have have this, as it should just be a hash of the
>> certificate itself as is.
>>
>> I have only glared at the code so far, and it looks reasonable.
>> Will test it a bit more thorough in the coming days.
>
> This hash is internal to openvpn, and has nothing to do with the
> CA signatures. We can use whatever we want. I'm not aware of any
> reason to assume SHA256 will be broken anytime soon, and if there
> is some day, we will just switch to SHA-newest ;) (We can't do
> SHA3 now, because our backends do not support is yet.)
Ahh, good, then I won't worry about this. We could do SHA512 though,
but not sure it's really worth it in this context ;-)
David S.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
iQIcBAEBAgAGBQJXwCNLAAoJEIbPlEyWcf3yuSQQAKyS25Dk8Ipvb2IPIYZvZPgO
QOQRYjuq7Nv3AdG9fMq7sEs7AymHP5vNRZUoGR0ev5AbhX6gEysIxvrg+gxuqBDx
wUKvplomkqUSxBwYNWTsTcqkW4IpduYNhJdpiXfFN30bRcePBm7HZvkShG9FsgzR
hfAHPXH/TD5ConJPB8WiYO2sbkD86o6Ik7Ur6Iu5aI2vWneTFCU/gPOz/oj6Ad07
ERGyBk3DSbyVuQ9BjvCRWZ6m/BSlt4NF86wWkijCbcgNRoZufyO2PAHZ0o59Yzpa
ZdaFPl044zV9FHhbSL4Zx09Mw99+DrA333gbbIajWRQGtkjAjIasNb3R7O7R1+Xm
E90aUemfQikC4aRG1rkgMCyUGy9H9Ywe7Thpvi72xtxvxRH34XtCFibFfdDuc1KO
ufYuyFyAahkarAmhjsV/Dm3Loy6Fedmcv31Ice8heZJ/4GSyexp3M7Aimpo9RIck
9UkJ7lha7ExlpoGnwq15BsARLDhXMC0sjuQ9PCv4ExezofBmHgo+1Vxg7L7DCSia
zi3e3PqX81eYgG1uKZJEiTa8lqo11cuNOM3+N9ruWJBLDqZu7YgTa/MCoPDfQXoe
x8yFIRwqOSW++7L9ebCwBxaoxY/5ADeVcDxdV7KhYMIgYsvtsLnLRWG6jPrpDGl8
R9jFm+XTNuCYQyfHhlti
=94yV
-----END PGP SIGNATURE-----
------------------------------------------------------------------------------
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
