Load specified or default SSL CA list

[jjd-git]

If an SSL CA list was specified, load it (correctly). Otherwise, load the default SSL CA list paths.

[ghost]

It makes a bit of code duplication but I guess it's worth it, as it allows clearly seeing which loading failed. That being said, it still is a potentially breaking change, as a failure to load the custom CA list will fail, without attempting to fallback to the default CA list...

[jjd-git]

Actually, the original code is more serious than that.

If the "load" call works, the "default" call will be executed (ouch!), negating the "load". (Carefully think about the return codes, the "!", and the "||".)

With the original code, if you specify an ssl_ca_list, it really won't get used, but (at least) the default CA list will get used, and you can verify most certificates. If you omit ssl_ca_list, there won't be any CA list at all, and you won't be able to verify any certificates.

I thought about adding the "default" call in the body of the "load" call's error path. But I decided if the user wants to use his own ssl_ca_list and it fails, it isn't appropriate to use the default list that he was trying to circumvent.

From: David Pineau [mailto:notifications@github.com] Sent: Tuesday, March 29, 2016 12:06 PM To: scality/Droplet Cc: James Demski Subject: Re: [scality/Droplet] Load specified or default SSL CA list (#216)

It makes a bit of code duplication but I guess it's worth it, as it allows clearly seeing which loading failed. That being said, it still is a potentially breaking change, as a failure to load the custom CA list will fail, without attempting to fallback to the default CA list...

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHubhttps://github.com/scality/Droplet/pull/216#issuecomment-202976558

[ghost]

From the function's manpage:

RETURN VALUES
       The following return values can occur:

       0   The operation failed because CAfile and CApath are NULL or the processing at one of the locations specified failed. Check the error stack to find out the reason.

       1   The operation succeeded.

This means that testing the first call for failure is testing !call(), meaning then that || makes sense (its behavior in return codes is opposite to most of the droplet code). That being said, I cannot find the manpage for the second function called so I'm not sure.

Also, I was talking about breaking behavior change, but ultimately, I agree about not loading the default CA path if a specified one fails.

[jjd-git]

Basically the code is:

            if (!load() || !default())

If load() succeeds, it returns 1. So in the successful case, we have:

            if (!1 || !default())

which turns into

            if (0 || !default())

And default() is executed, which is not what we want.

[ghost]

Oh right, I'll attribute that to fatigue... Then I guess I got nothing else to add here :) :+1:

[jjd-git]

In my opinion, the original code is tricky to follow. Especially when you "know" what it does.