[pve-devel] Something missing in http://pve.proxmox.com/wiki/HTTPS_Certificate_Configuration_(Version_4.x_and_newer) ?

[Stefan Priebe - Profihost AG]

Am 22.11.2016 um 14:38 schrieb Fabian Grünbichler:
> On Tue, Nov 22, 2016 at 01:57:47PM +0100, Fabian Grünbichler wrote:
>> On Tue, Nov 22, 2016 at 01:09:17PM +0100, Stefan Priebe - Profihost AG wrote:
>>> Hi,
>>>
>>> Am 22.11.2016 um 12:26 schrieb Fabian Grünbichler:
>>>> On Tue, Nov 22, 2016 at 12:11:22PM +0100, Stefan Priebe - Profihost AG wrote:
>>>>> Am 22.11.2016 um 11:56 schrieb Dietmar Maurer:
>>>>>> I think this commit should solve the issue:
>>>>>>
>>>>>> https://git.proxmox.com/?p=pve-manager.git;a=commitdiff;h=333dd203d5e07d9d3e20d3674a2e3ff2fc89fa8c
>>>>>>
>>>>>>> Please can you test with latest version from git?
>>>>>
>>>>> Already running that version ;-) But thank you for pointing me to this
>>>>> commit. If i revert that one it's working fine again.
>>>>>
>>>>> The issue in my case was that the verify in HTTPServer.pm verify_cb was
>>>>> failing.
>>>>>
>>>>> The documentation says:
>>>>> "fullchain.pem (your certificate and all intermediate certificates,
>>>>> excluding the root certificate, in PEM format)"
>>>>>
>>>>> With the full chain it's not working. I then removed the whole chain and
>>>>> only putted my final crt into that one and now it's working fine. With
>>>>> the full chain $depth was 2 in my case.
>>>>
>>>> I will do some more testing later on - is it possible that you put the
>>>> certificates into the file in the "wrong" order (leaf first, then
>>>> intermediate would be correct)?
>>>>
>>>> The pinning is supposed to only verify the last certificate in the chain
>>>> (as returned by the server), so whether you have a chain of depth 2 or 3
>>>> (self-signed root + leaf or root + ca + leaf) does not matter at all.
>>>> But when reading from a .pem file with multiple certificates OpenSSL
>>>> reads the first one in the file, so it's possible that in your case we
>>>> attempt to compare the leaf certificate's fingerprint (from the
>>>> connection / server) to the CA certificate's fingerprint (from the .pem
>>>> file), which obviously does not work.
>>>>
>>>> If you want, you could send me the certificate file off-list (only the
>>>> certificate please! unless those are from a test node with self-signed
>>>> certificates that you don't care about) and I will try to recreate your
>>>> setup for further tests...
>>>
>>> As this is a real life certificate which is not self signed but signed
>>> from the ca i'm not comfortable with that.
>>
>> understandable. but maybe you could confirm the order? you can just copy
>> the file and split at the end/start marker, with "openssl x509 -in
>> PEMFILE -noout -subject" you should get the subject of each resulting
>> PEMFILE
>>
>>>
>>> But may be you're already right and the order in the file is important.
>>>
>>> Wouldn't it be easier for users to just put the sigle .crt into this
>>> location and put the ca chain into pve-root-ca.pem ?
>>
>> that does not work (unfortunately) - AnyEvent cannot load the chain / CA
>> certificate from a separate file.. since the root certificate is not
>> supposed to be part of the chain sent by the server (it needs to be
>> trusted separately anyway), this is not a problem for the self-signed
>> chain with depth 2. but for the "regular" chain of depth 3 you need to
>> put the leaf/server certificate and the intermediate(s) into
>> pveproxy-ssl.pem (and the root into your OS/browser, if it is not
>> already there ;)).
>>
>> I'll test some more, and as a last workaround we could do the split of
>> pveproxy-ssl.pem in the verification code (and simply only take the one
>> that is not a CA certificate - this is easy to check because the CA
>> property is a certificate extension, but of course we have additional
>> overhead and complexity for parsing and splitting..).
> 
> seems like the error was a flipped default return value in one of the
> last refactorings before sending the patch - could you try the following
> patch?
> 
> a wrong order in pveproxy-ssl.pem would already completely break the
> whole pveproxy, so this cannot be the issue ;)
>
> 
> -- >8 --
> 
> Subject: [PATCH manager] fix SSL verify callback for certificate chains
> 
> ignoring parts of the chain means saying they are verified,
> because the verify callback results are chained together
> starting with the highest depth.
> ---
>  PVE/HTTPServer.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/PVE/HTTPServer.pm b/PVE/HTTPServer.pm
> index db1faae..3460050 100755
> --- a/PVE/HTTPServer.pm
> +++ b/PVE/HTTPServer.pm
> @@ -700,7 +700,7 @@ sub proxy_request {
>             verify_cb => sub {
>                 my (undef, undef, undef, $depth, undef, undef, $cert) = @_;
>                 # we don't care about intermediate or root certificates
> -               return 0 if $depth != 0;
> +               return 1 if $depth != 0;
>                 # check server certificate against cache of pinned FPs
>                 return check_cert_fingerprint($cert);
>             },
> 

Yes that works. But is this correct? Does it not skip the whole verify
if depth is != 0?

Stefan