[PATCH] Add openconnect_get_client_cert() to API

[David Woodhouse]

On Sat, 2011-09-17 at 23:12 +0300, Jussi Kukkonen wrote:
> 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.

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

-- 
dwmw2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/openconnect-devel/attachments/20110917/79c8fd8f/attachment.bin>