GNOME Bugzilla – Bug 770922
[patch] Implement verify-x509-name option in NetworkManager-OpenVPN
Last modified: 2017-01-19 18:34:29 UTC
Created attachment 334867 [details] [review] 0001-connection-add-support-for-verify-x509-name-property.patch Patches are attached to implement support for the verify-x509-name OpenVPN option, which is an important hardening tool, and replaces the tls-remote option. --- connection: add support for verify-x509-name property The verify-x509-name option deprecates the tls-remote option starting with OpenVPN 2.3. Add support for it in the connection properties, as it is an important security feature for avoiding man-in-the-middle attacks. The connection property is stored as `{type}:{value}`, with type being one of (name, name-prefix, subject), and value a comma-separated sequence of `key=value` comparisons. It is combined/split-up into the two OpenVPN option parameters when import/exporting. --- properties: add support for verify-x509-name Expose the verify-x590-name option in the editor UI by reusing the tls-remote subject entry, and adding a combo box for selecting a verification mode. Selectable modes map to their respective verify-x509-name types ('subject', 'name' or 'name-prefix'), except for 'legacy', which sets tls-remote and omits verify-x509-name, and 'none', which omits both. Descriptions and tooltips are updated to attempt to describe the different modes in a straightforward way, but their actual operation is unfortunately not very simple. Updates to translations will be necessary later.
Created attachment 334868 [details] [review] 0002-properties-add-support-for-verify-x509-name.patch
Review of attachment 334867 [details] [review]: ::: properties/import-export.c @@ +1927,3 @@ + + name = strchr (x509_name, ':'); + g_assert (name); This causes the library to crash in face of an invalid setting. An invalid setting should probably be just ignored and not be exported. Maybe we should export the invalid setting to as a comment, like args_write_line_verbatim (f, "# invalid: verify-x509-name %s", x509_name) (note that you cannot use the current args_write_*() functions, because they would escape special caracters) @@ +1932,3 @@ + g_assert (type != name); + + name_export = nmv_utils_str_utf8safe_unescape (name + 1); all settings a strings (on D-Bus), thus they must be value utf8. For filenames, we support that by reading them with back and forth with nmv_utils_str_utf8safe_escape/unescape. Do you think it is necessary that name might non-utf8? I think not. In fact, during import you check with args_params_check_arg_utf8() that the value is utf8. Also, you either must both escape and unescape during import/export, or not. Unescaping only during export is wrong. @@ +1933,3 @@ + + name_export = nmv_utils_str_utf8safe_unescape (name + 1); + args_write_line (f, NMV_OVPN_TAG_VERIFY_X509_NAME, name_export, type); whitespace ::: src/nm-openvpn-service.c @@ +1443,3 @@ + } + + if (!name || !type_len || *name == '\0') { type_len may be uninitialized (at least, the compiler thinks so)
Review of attachment 334868 [details] [review]: I like how you extended to UI. ::: properties/auth-helpers.c @@ +1226,3 @@ + + gtk_tree_model_get (model, &iter, TLS_REMOTE_MODE_COL_VALUE, &tls_remote_mode, -1); + entry_enabled = g_strcmp0(tls_remote_mode, NM_OPENVPN_TLS_REMOTE_MODE_NONE) != 0; whitespace @@ +2097,3 @@ + model = gtk_combo_box_get_model (GTK_COMBO_BOX (combo)); + + if (value && strlen (value) && gtk_combo_box_get_active_iter (GTK_COMBO_BOX (combo), &iter)) { is there an easy way to disable the "Ok" button if value is empty (with the tls_remote_mode != NONE) ::: properties/nm-openvpn-dialog.ui @@ +1884,3 @@ + +config: verify-x509-name match [mode] +config (legacy modew): tls-remote match</property> typo: legacy modew
Created attachment 335057 [details] [review] 0001-connection-add-support-for-verify-x509-name-property.patch
Created attachment 335058 [details] [review] 0002-properties-add-support-for-verify-x509-name.patch
Created attachment 335059 [details] [review] 0003-properties-use-helpers-for-dev.-name-entry-errors.patch
Created attachment 335060 [details] [review] 0004-Updated-Braziian-Portuguese-translation.patch
Created attachment 335061 [details] [review] 0004-Updated-Brazilian-Portuguese-translation.patch
I have updated the patches with the requested changes. * Don't use assertions to check validity when importing/exporting. Just raise errors during importing, and export possibly bad modes - OpenVPN will throw an error in that case, which is better than failing the export altogether or omitting the property completely. * Don't use the UTF-8 (un)escaping functions when importing/exporting. Just validate that the subject name is valid UTF-8 when necessary. * Move the UI mode constants to properties/auth-helpers.c. * Create shared constants in shared/nm-service-defines.h. * Fix uninitialized variable warning (variable no longer exists). * Fix typos in UI labels (improve messages slightly) * Import widget error highlighting helpers from gnome-control-center * Disable OK button and highlight subject entry box when a mode other than 'none' is selected, but the text is empty. * Use the newly added helpers instead of manually setting the background color for TUN/TAP device name selection. * Update pt_BR translation
Patches look good to me.
merged to master: https://git.gnome.org/browse/network-manager-openvpn/commit/?id=763548d00a0472577ebb35083939fa3039ff17de Thanks Daniel. (sidenote: as a follow-up, it would be great if the ok_button would have a register_error() and unregister_error() function. So, that multiple components could register their error... currently, if you would have two input fields that are wrong, they would compete with each other when enabling the ok_button. Also, then the ok_button should show a tooltip with the error-reason)
Thanks Thomas, your feedback was really helpful. I'll look into the competing errors issue later, I think we can solve it by storing an error count instead of just manually flipping the button state in each location.
(In reply to Daniel Miranda from comment #12) > Thanks Thomas, your feedback was really helpful. I'll look into the > competing errors issue later, I think we can solve it by storing an error > count instead of just manually flipping the button state in each location. currently, I think only your component even disables the ok_button, so it's correct for now. Later, it may be useful for other components to also disable ok_button. So, yes, a counter might help there. But I would just register/unregister and track them with some additional data. At least with a error message that can be used as a tooltip, so optimally you need more then a counter.
*** Bug 675439 has been marked as a duplicate of this bug. ***