After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 770880 - Add strict certificate match
Add strict certificate match
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: VPN: openconnect
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: David Woodhouse
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2016-09-05 09:58 UTC by Frederik Dennig
Modified: 2016-12-15 20:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch of the certificate authentication dialog and VPN configuration editor (14.66 KB, patch)
2016-10-16 06:46 UTC, Frederik Dennig
none Details | Review
Image of the new warning dialog (37.80 KB, image/png)
2016-10-16 06:48 UTC, Frederik Dennig
  Details
Image of the modified VPN configuration editor (70.93 KB, image/png)
2016-10-16 06:49 UTC, Frederik Dennig
  Details
Redesign of the certificate authentication warning (6.64 KB, patch)
2016-10-17 10:42 UTC, Frederik Dennig
none Details | Review
Prevent user from manually accepting invalid certificates (9.80 KB, patch)
2016-10-17 10:43 UTC, Frederik Dennig
none Details | Review

Description Frederik Dennig 2016-09-05 09:58:04 UTC
The NetworkManager OpenConnect plugin allows connections to untrusted servers (e.g. certificate mismatch). There should be an option that disallows connections with such servers by default, that can be disabled manually in the configuration dialog if needed. A similar feature is part of the original Cisco AnyConnect client.
Comment 1 David Woodhouse 2016-09-05 10:20:34 UTC
Hm, users already get a nasty warning prompt that the certificate is untrusted.

What failure mode do you expect to see when a connection is disallowed with such a server?

We *have* to tell the user what happened, and we probably even have to tell them about the configuration option which they can change, if they really need to talk to this server.

So all we're really doing is giving them additional hoops to jump through. Instead of clicking 'accept', they need to go into their configuration and change it, then try again and click 'accept'. There's no fundamental improvement in security here.

I do understand that users are often wont to do the wrong thing. I don't necessarily *object* to making them jump through extra hoops. But perhaps let's model it on what web browsers do. In the pop-up which warns of an invalid certificate, hide the "Proceed anyway" option inside an 'Advanced' or 'I really know what I'm doing' option.
Comment 2 Frederik Dennig 2016-09-07 13:49:28 UTC
The main goal is to prevent users from connecting to malicious servers. In bigger organizations (like universities) most of the users will just click "OK". Many will not read the error message or are unable to understand it. For some businesses this may be a security concern.

If the "Connect to untrusted servers" option is disabled a message is shown where the "I really know what I'm doing" option is greyed out and the user is notified that there was a certificate mismatch and - if really needed - the "Connect to untrusted servers" can be activated in the configuration.

The current dialog shows the whole certificate information, which can be quite overwhelming for not so tech-savvy users. The problem I see with this is, that the information on what went wrong gets kind of lost.

The idea of modeling the error message after the behavior of web browsers, also came to my mind. It could be a good basis for a clean error message, which would be more helpful to the user.
Comment 3 David Woodhouse 2016-09-07 14:04:07 UTC
Yeah, that dialog currently really sucks. It does the bare minimum to allow things to work.

We should replace the certificate information with a nice GUI widget like https://developer.gnome.org/gcr/stable/GcrCertificateWidget.html and also hide the 'accept' facility behind an "I know what I'm doing" expander like web browsers do.

Patches welcome... :)
Comment 4 Frederik Dennig 2016-09-08 12:10:31 UTC
Okay, I will start working on the redesign of the warning message, using the proposed certificate information GUI widget and adding the discussed expander.

I am currently using the network-manager-openconnect 1.2.2-1 source package and Ubuntu 16.04 LTS to modify and build the OpenConnect plugin.

Is this a viable basis for changes?
Comment 5 Thomas Haller 2016-09-08 12:43:15 UTC
(In reply to Frederik Dennig from comment #4)

> Is this a viable basis for changes?

IMO you should use git-master.

  ./autogen.sh
  ./configure --prefix/some/where
  make && make install

You can install the build to a different --prefix, as long as you copy the .name file to /usr/lib/NetworkManager/VPN/nm-openconnect-service.name (note, that for legacy reasons, NetworkManager also looks into /etc/NetworkManager/VPN for .name files).
For libnm (nmcli,nm-connection-editor,nm-applet) you don't actually have to install the .name file. Instead you could do:
  NM_VPN_PLUGIN_DIR=/some/where/lib/NetworkManager/VPN nm-connection-editor
Comment 6 David Woodhouse 2016-09-08 17:20:13 UTC
Note that the auth-dialog is a standalone program and you can test it fairly easily in isolation. You don't need to install it or do anything like that.

[dwoodhou@i7 auth-dialog]$ echo -e DATA_KEY=gateway\\nDATA_VAL=[::1]\\nDONE |  ./nm-openconnect-auth-dialog -u s51112bad-f8dc-4a37-9539-5182435fff65 -n VPN\ connection\ 1 -s org.freedesktop.NetworkManager.openconnect -i 


This has the added advantage that each time you test the 'remember' option to trust that host in future, it won't be stored and you can run your next test without having to manually make it forget that host's cert :)
Comment 7 Frederik Dennig 2016-10-16 06:46:55 UTC
Created attachment 337778 [details] [review]
Patch of the certificate authentication dialog and VPN configuration editor
Comment 8 Frederik Dennig 2016-10-16 06:48:27 UTC
Created attachment 337779 [details]
Image of the new warning dialog
Comment 9 Frederik Dennig 2016-10-16 06:49:15 UTC
Created attachment 337780 [details]
Image of the modified VPN configuration editor
Comment 10 Frederik Dennig 2016-10-16 06:56:11 UTC
Thank your for your help.

I created a patch, which contains the redesign of the authentication warning dialog as discussed above. An image of the new dialog is attached. The expander is initially not expanded.

Furthermore, the "Allow connection to untrusted server" option was added to the VPN configuration editor. This enables the user to skip the warning dialog. This is useful, if despite all warnings, the connection is frequently used. The warning dialog mentions this option, but clearly states that the server should be trustworthy. A picture of the modified configuration editor is also attached.

Both images contain some German text snippets.
Comment 11 David Woodhouse 2016-10-16 11:10:32 UTC
Nice work; thank you. But the dialog in comment 8 is confusing to me. What happens if I click 'connect anyway'? That's equivalent to the current code flow when I click to accept the certificate? And it will *not* warn me about that same certificate again?

In that case.... what's the new option for? Hopefully it doesn't just make it *always* connect to servers with bad certificates, without asking?

And if you went ahead with your original suggestion of needing a config option to *permit* the user to click 'connect anyway', which I didn't like, then I'm a little confused... why do I see both the 'connect anyway' button *and* an exhortation to go and enable that option, in the same image?
Comment 12 David Woodhouse 2016-10-16 11:13:52 UTC
As for the translations... I'm more concerned by the *English* text I see in it, than the German. Now some of it I accept is because you've just added new strings and they aren't translated yet... but is that it? Are all the reason strings (like 'signer not found') translated properly? Do we get those from GnuTLS and do we correctly find translations for them (or does GnuTLS give them to us in the correct form?).

Also, if you have explanation text which is referring to the 'Allow connection from untrusted server' option, let's insert that string as a %s into the dialog: "...this warning can be disabled with the \"%s\" option in the..."

That way, you make sure that in any language you have the *right* translation for that option and people will be looking for the right thing.
Comment 13 Frederik Dennig 2016-10-16 15:01:58 UTC
A click on "Connect anyway" is indeed identical to the current code flow. The user is always warned, even if he previously accepted the certificate.

The new option only disables the warning dialog, other checks are still performed. It acts as automatically clicking "OK" or "Connect anyway". This obviously defies the purpose of this bug report. Thank you, I got a little lost.

I actually should have removed the expander widget and the "Connect anyway" button from the dialog, if the option is active.

All reason strings are provided by GnuTLS in the translated form. The replacement of the "Allow connection from untrusted server" with %s is a very good idea.

Should I create two separate patches, one with the redesigned warning dialog and another (applied on top) one with the corrected "Allow connection from untrusted server" option?
Comment 14 David Woodhouse 2016-10-16 15:23:39 UTC
(In reply to Frederik Dennig from comment #13)
> A click on "Connect anyway" is indeed identical to the current code flow.
> The user is always warned, even if he previously accepted the certificate.

Those two sentences are mutually exclusive. The current code flow only warns the *first* time you accept an "invalid" certificate, and remembers it (for that host) thereafter. We need to make sure that mode still continues to work.

> The new option only disables the warning dialog, other checks are still
> performed. It acts as automatically clicking "OK" or "Connect anyway". This
> obviously defies the purpose of this bug report. Thank you, I got a little
> lost.

Yeah, don't do that :)

> I actually should have removed the expander widget and the "Connect anyway"
> button from the dialog, if the option is active.

Wait, didn't you just say the new option *disables* the warning dialog? So
how does it make sense to remove stuff "if the option is active"... when the option disables the dialog completely anyway?

Were you thinking about the originally-discussed "Do not allow the user to manually accept untrusted certificates" option?

> Should I create two separate patches, one with the redesigned warning dialog
> and another (applied on top) one with the corrected "Allow connection from
> untrusted server" option?

Yes please, and make the latter the other way round: something like "Do not allow invalid certificates to be manually accepted".

Organisations which deploy proper trust chains and *expect* that their VPN servers will have sane trust, can set that option when they deploy VPN configurations to users. Other organisations may not (and 'not' should probably be the default).
Comment 15 Frederik Dennig 2016-10-17 10:42:20 UTC
Created attachment 337825 [details] [review]
Redesign of the certificate authentication warning
Comment 16 Frederik Dennig 2016-10-17 10:43:56 UTC
Created attachment 337826 [details] [review]
Prevent user from manually accepting invalid certificates
Comment 17 Frederik Dennig 2016-10-17 10:52:00 UTC
(In reply to David Woodhouse from comment #14)
> Those two sentences are mutually exclusive. The current code flow only warns
> the *first* time you accept an "invalid" certificate, and remembers it (for
> that host) thereafter. We need to make sure that mode still continues to
> work.

That is strange. Any version that I ever used didn't remember the certificate, even if I used the "Save passwords" option. However, I'm sure that I didn't change any part of the code that is responsible for this functionality.

> Yes please, and make the latter the other way round: something like "Do not
> allow invalid certificates to be manually accepted".
> 
> Organisations which deploy proper trust chains and *expect* that their VPN
> servers will have sane trust, can set that option when they deploy VPN
> configurations to users. Other organisations may not (and 'not' should
> probably be the default).

I created two patches, one with the new warning dialog and an another one for the "Prohibit user from manually accepting invalid certificates" option. This option removes the expander and the "Connect anyway" button form the warning dialog. The user is then unable to accept the certificate.

The exhortation you mention in comment #11 has been completely removed from the new warning dialog, since it became obsolete.
Comment 18 David Woodhouse 2016-10-17 15:09:14 UTC
(In reply to Frederik Dennig from comment #17)
> (In reply to David Woodhouse from comment #14)
> > Those two sentences are mutually exclusive. The current code flow only warns
> > the *first* time you accept an "invalid" certificate, and remembers it (for
> > that host) thereafter. We need to make sure that mode still continues to
> > work.
> 
> That is strange. Any version that I ever used didn't remember the
> certificate, even if I used the "Save passwords" option. However, I'm sure
> that I didn't change any part of the code that is responsible for this
> functionality.

It should have done, but you might have been hitting 
https://bugzilla.redhat.com/show_bug.cgi?id=1332491

> I created two patches, one with the new warning dialog and an another one
> for the "Prohibit user from manually accepting invalid certificates" option.
> This option removes the expander and the "Connect anyway" button form the
> warning dialog. The user is then unable to accept the certificate.
> 
> The exhortation you mention in comment #11 has been completely removed from
> the new warning dialog, since it became obsolete.

Great; thanks. I'm travelling this week but if you haven't seen any update in a week's time, please feel free to prod me here or by direct email. If anyone else (thaller) is watching, I'm happy enough with the concept and we just need to glance over the implementation before committing it.
Comment 19 Frederik Dennig 2016-10-27 13:56:22 UTC
(In reply to David Woodhouse from comment #18)

> It should have done, but you might have been hitting 
> https://bugzilla.redhat.com/show_bug.cgi?id=1332491

This is definitely the problem. This report describes the observed behaviour perfectly.
 
> Great; thanks. I'm travelling this week but if you haven't seen any update
> in a week's time, please feel free to prod me here or by direct email. If
> anyone else (thaller) is watching, I'm happy enough with the concept and we
> just need to glance over the implementation before committing it.

Since no update appeared. I hereby prod you :)
Comment 20 David Woodhouse 2016-12-15 13:19:01 UTC
Patches applied; thanks.
Comment 21 David Woodhouse 2016-12-15 20:37:53 UTC
Updated package for Fedora 25: https://bodhi.fedoraproject.org/updates/FEDORA-2016-ba7028b69c