GNOME Bugzilla – Bug 770880
Add strict certificate match
Last modified: 2016-12-15 20:37:53 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.
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.
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.
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... :)
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?
(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
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 :)
Created attachment 337778 [details] [review] Patch of the certificate authentication dialog and VPN configuration editor
Created attachment 337779 [details] Image of the new warning dialog
Created attachment 337780 [details] Image of the modified VPN configuration editor
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.
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?
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.
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?
(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).
Created attachment 337825 [details] [review] Redesign of the certificate authentication warning
Created attachment 337826 [details] [review] Prevent user from manually accepting invalid certificates
(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.
(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.
(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 :)
Patches applied; thanks.
Updated package for Fedora 25: https://bodhi.fedoraproject.org/updates/FEDORA-2016-ba7028b69c