[PATCH] Add openconnect_get_client_cert() to API

[Jussi Kukkonen]

On 09/18/2011 12:54 AM, David Woodhouse wrote:
>> In addition to the problems you mention below, I think the certificate
>> warning only appears in the logs when the connection is attempted. I
>> would definitely prefer showing that when the dialog is shown.
> 
> Yes, it happens when you first connect to the chosen server, before you
> get asked for your username/password (which is actually a form that the
> server presents).
> 
> If you want it to happen earlier, you may have to ask for the
> certificate passphrase earlier too; with a PKCS#12 file you'll need the
> passphrase even to parse the certificate.
> 
> And in fact reload_pem_cert() isn't going to work when you have a
> PKCS#12 file either. The way this is supposed to work is that
> load_certificate() sets vpninfo->cert_x509 as it's loading the cert. If
> the cert is PKCS#12 then we parse the file and see its constituent parts
> so we can easily save the X509. If it's a PEM file then that doesn't
> happen (OpenSSL APIs are wildly inconsistent), and we can't even get it
> back out of the SSL_CTX easily. So we have the reload_pem_cert() hack to
> load it again. But that is an *internal* detail of the
> load_certificate() function.

Ah, I see. That explains why it was seemingly so complex...

I don't want to change the user interaction here -- it seems quite
standard and logical -- so either we just live with the expiry warning
appearing at only connection time or provide early warnings only when it
happens to be easy.

I still think it would make sense to make the certificate expiry date
available to the application if possible (I suggested _get_client_cert()
because I imagined other details in the cert could be useful as well).
Creating user messages without the date is doable but not really optimal.

Seeing that I obviously don't understand all the certificate
possibilities here, I'll just leave this for you to decide.

>>> I was thinking that we should just fix the UIs to display PRG_ERR
>>> messages more prominently than just in the hidden-by-default log box. Or
>>> perhaps we should add a new PRG_NOTICE message type just for that
>>> behaviour.
>>
>> Improving this area could be useful, regardless of how we end up solving
>> this particular problem  -- although some of the doubts further below
>> apply here as well (translatability as a first thing).
> 
> I need to fix translations in OpenConnect anyway.
> 
>> I'd say there are three different types of messages from a UI point of
>> view: important notices (like "cert will expire in 3 days"),
>> show-stopper errors ("Your cert has expired"), and everything else. The
>> first two should always be shown to user (with slightly different
>> visuals) and would need to be translatable. Preferably only one
>> "important error/notice" is shown to user at a time -- seeing both "Cert
>> expired" and "HTTPS connection failed" is factually correct but not
>> really useful.
>>
>>> That would allow OpenConnect to complain to the user about anything it
>>> likes, rather than having to put logic into *all* of the UI
>>> implementations as we find new things to bitch about.
>>
>> The question here is, would it still make sense to define specific
>> progress codes for "important errors" + "important notices"  we know
>> about? Then the UI could choose to use openconnect-provided strings or
>> to handle the progress codes it happens to understands itself...
> 
> I was already pondering that kind of thing anyway, because it makes it
> much easier to return errors to NetworkManager from the actual
> connection (as opposed to the auth-dialog which is user-facing anyway).
> 
>> Does this sound feasible? It does sound like an API change. You probably
>> have a feel to how many different progress codes we'd have to define and
>> how good 'coverage' we might expect with it?
> 
> I think we'd change every call to vpn_progress(). And we could probably
> do it in a compatible fashion, so that users who want to provide a new
> progress function can do so, but existing users will just continue to
> behave the same as they do now.

This sounds fine to me.

 - Jussi