vdirsyncer 0.19: auth_cert broken

[hanophora]

So I was investigating why my calendar wasn't syncing anymore, with 401 responses. After seeing my client certificate wasn't being received on the server, I realized this was coming from vdirsyncer. A bit of digging and I found these lines in http.py:

    # TODO: Support for client-side certifications.
    #...
    kwargs.pop("cert", None)  # TODO XXX FIXME!

So, yeah. A line in the changelog would have saved me a bit of time. 🥲
Since I need this I'll see what I can do.

[hanophora]

Looks like something like this works fine.

diff --git i/vdirsyncer/http.py w/vdirsyncer/http.py
index b35035b..2791433 100644
--- i/vdirsyncer/http.py
+++ w/vdirsyncer/http.py
@@ -127,7 +127,11 @@ async def request(
 
     assert isinstance(kwargs.get("data", b""), bytes)
 
-    kwargs.pop("cert", None)  # TODO XXX FIXME!
+    cert = kwargs.pop("cert", None)
+    if cert is not None:
+        ssl_context = create_default_context()
+        ssl_context.load_cert_chain(cert)
+        kwargs['ssl'] = ssl_context
 
     response = await session.request(method, url, **kwargs)
 

Needs a bit more work obviously (supporting list arg for auth_cert, tests), but it's late where I am.

[WhyNotHugo]

Whoops, sorry, that TODO should not have shipped in that state. 😢

[WhyNotHugo]

I don't have a server set up with client certificates.

Do you have your own server? Is setting up a test user feasible?

[WhyNotHugo]

Can you confirm if #1037 is sufficient to address this? I believe the previous call to prepare_client_cert in vdirsyncer/storage/dav.py should cover everything else.

Right now I don't have a test setup where I can log in with client certificates. If you have a public server where you can provide a test account, that would be super useful. I'm currently looking for hosted Dav servers to improve testing.

[tom-kuca]

I found two errors in the patch:

It breaks server certificate verification

A new blank SSL context is created. There might already be one with CA certifificate setup.

-ssl_context = create_default_context()
+ssl_context = kwargs.pop("ssl", create_default_context())

It doesn't support private key in a separate file.

From the documentation

auth_cert – Optional. Either a path to a certificate with a client certificate and the key or a list of paths to the files with them.

The list is converted to a tuple, load_cert_chain expects private key as a separate argument.

-ssl_context.load_cert_chain(cert)
+ssl_context.load_cert_chain(*cert) 

It works for both cases.

The complete snippet that works for me:

    cert = kwargs.pop("cert", None)
    if cert is not None:
        ssl_context = kwargs.pop("ssl", create_default_context())
        ssl_context.load_cert_chain(*cert)
        kwargs['ssl'] = ssl_context

Tested on version 0.19.1.dev28+gdf14865

[WhyNotHugo]

Thanks for digging into this and sharing all the details!