[PATCH] Add openconnect_get_client_cert() to API

[Jussi Kukkonen]

On 09/17/2011 02:24 PM, David Woodhouse wrote:
> On Sat, 2011-09-17 at 12:31 +0300, Jussi Kukkonen wrote:
>>
>> I just experienced client certificate expiry with openconnect and
>> figured we could be more informative about this situation. I don't
>> have
>> good suggestions for the openconnect binary -- looking at the code it
>> seems to have warned me a couple of months (!) in advance, and I just
>> hadn't reacted... but the NM and connman UIs are sorely lacking in
>> this
>> regard and it seems they don't have all the information they need to
>> solve the problem.
>>
>> Would this be an acceptable addition to the openconnect api? It would
>> allow the library users to do whatever they want with
>> X509_get_notAfter(), X509_cmp_time(), etc using the client cert.
> 
> There's been a bunch of people coming to me with "my VPN stopped
> working" in the last week or two. Thanks for being one of the people who
> worked it out for themselves and *didn't* come and ask me :)
> 
> Thanks for the patch too... I was also pondering this issue, but my
> approach was going to be slightly different.
> 
> Strictly speaking, you're not quite right when you say that the NM and
> ConnMan UIs don't have the information they need. I believe that their
> ->progress() functions *were* called with the warning message.

Yes, this is true.

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.

> 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'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...

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?

> On the other hand, your approach does perhaps allow the UI to be 'nicer'
> about it, because it knows exactly what's going on so it can add a
> button to 'view certificate' etc., rather than just showing a line of
> arbitrary (and currently untranslated) text with an 'attention' icon.
> But it means that we end up implementing the same certificate check in
> the gNM, kNM, Android and ConnMan UIs separately (and in any future UIs
> like the one I was hoping someone would do for MacOS)
> 
> What do you think?

You just covered my thoughts pretty well (translations, ability to fully
control the user experience). I understand your desire to do this work
in the library, and I don't think that's impossible...
but it may be quite hard to design in a way that makes more than one UI
designer happy -- As an example a mobile app might want to just say
"Certificate expires soon" because that will just fit on the one line
that they have available while a desktop app could want to use a longer
sentence with an actual date & time and give some additional advice,
maybe even provide the issuer name.

I could be wrong about the example above but my instinct in general is
to at least offer applications the ability to handle the known problems
themselves. Defining progress codes in addition to progress strings
would help there, I think. Although access to the client cert could
still be useful for applications.

 - Jussi