GNOME Bugzilla – Bug 779231
[review] lr/pkcs11: add support for choosing 802.1x certificates and keys from PKCS#11 tokens
Last modified: 2017-03-19 20:09:35 UTC
e083c66 build: bump the version to 1.7, NetworkManager requirement to 1.8 2ee8b27 editor: drop use of deprecated NetworkManager API ce3ca49 wireless-security: replace a GtkTable with GtkGrid a655fa2 libnma: fix a typo e906fdd libnma: add a certificate picker interface 7366555 libnma: add a file certificate chooser fdbcd70 build: optionally build with the GCR support 151111c libnma: add PKCS#11 login dialog e4c345b libnma: add PKCS#11 object chooser dialog a1bb362 libnma: add certificate chooser button 99e7772 libnma: add the PKCS#11 capable certificate chooser dea74a8 wireless-security: make eap-tls use the NMACertificateChooser
before bumping the version, let's do a final 1.4.6 release (so that we may optionally branch-off from that point a nm-1-4 branch later). > editor: drop use of deprecated NetworkManager API ce_page_device_entry_get() is rather confusing, but it seems that there are several "goto invalid" places that don't set the output error. > libnma: add a certificate picker interface commit message typo: ...key pair optionaly with... ^^^^^^^^^ The API is bases with possible ^^^^^ src/libnm-gtk/libnm-gtk.ver: libnma_1_8_0 { } libnma_1_0_6; should be libnm-gtk* libnm_gtk_h_pub = \ src/libnm-gtk/nm-wifi-dialog.h \ src/libnm-gtk/nm-wireless-dialog.h \ src/libnm-gtk/nm-mobile-wizard.h \ src/libnm-gtk/nm-ui-utils.h \ src/libnm-gtk/nm-mobile-providers.h \ - src/libnm-gtk/nm-vpn-password-dialog.h + src/libnm-gtk/nm-vpn-password-dialog.h \ + src/libnma/nma-cert-chooser.h ok, makes sense to re-use the source file (wish we had that done for libnm/libnma in general). But I think at least the header file must be copied to have a ~proper~ "nm-cert-chooser.h" name. In nma-cert-chooser.c you can then #if LIBNM_BUILD #include "nma-cert-chooser.h" #else #include "nm-cert-chooser.h" #endif Especially because: +#ifdef LIBNM_BUILD +#include <NetworkManager.h> +#else +#include <nm-setting-8021x.h> +#endif is not gonna work becaues LIBNM_BUILD is not a public define. src/libnma/nma-cert-chooser.c files should be included in the following order: #include "nm-default.h" #include "nma-cert-chooser.h" #include <~external headers~> #include <~libnm headers~> #include "~libnma headers~" #include "~private headers~" <glib/gi18n-lib.h> and "config.h" should not be included, it's provided by nm-default.h we don't have NMA_AVAILABLE_IN_1_8 macros, but I think we should add them. also missing "Since: 1.8" comments. + NMA_CERT_CHOOSER_FLAG_PASSWORDS = 0x2, + NMA_CERT_CHOOSER_FLAG_PEM = 0x4, whitespace as the functions in src/libnma/nma-cert-chooser.c are public API, they should have have proper g_return_val_if_fail (NMA_IS_CERT_CHOOSER (cert_chooser), NULL); preambles.
I can't say I understood everything in the branch, but from a high-level glance looks good. Only minor style issues below. > libnma: add a certificate picker interface +libnma_1_8_0 { +global: + nma_cert_chooser_add_to_size_group; + nma_cert_chooser_get_type; + nma_cert_chooser_set_cert; + nma_cert_chooser_get_cert; + nma_cert_chooser_set_cert_password; + nma_cert_chooser_get_cert_password; + nma_cert_chooser_get_cert_password_flags; Please sort items alphabetically. +typedef struct { + GTypeInterface parent_iface; + + /* virtual m ethods */ Midword spaces. > libnma: add a file certificate chooser +static gchar * +get_cert (NMACertChooser *cert_chooser, NMSetting8021xCKScheme *scheme) +{ + NMAFileCertChooserPrivate *priv = NMA_FILE_CERT_CHOOSER_GET_PRIVATE (cert_chooser); + + g_return_val_if_fail (scheme != NULL, NULL); + *scheme = NM_SETTING_802_1X_CK_SCHEME_PATH; + return gtk_file_chooser_get_filename (GTK_FILE_CHOOSER (priv->cert_button)); +} + + Extra empty line. > build: optionally build with the GCR support + AC_MSG_CHECKING([for GCR usefullness]) usefulness > libnma: add PKCS#11 login dialog +#if 0 +GtkDialogFlags flags, + "use-header-bar", !!(flags & GTK_DIALOG_USE_HEADER_BAR), + "title", title, +#endif Can this be removed?
Addressed all points from the reviews above, pushed an updated version.
libnm_gtk_h_pub = \ src/libnm-gtk/nm-wifi-dialog.h \ src/libnm-gtk/nm-wireless-dialog.h \ src/libnm-gtk/nm-mobile-wizard.h \ src/libnm-gtk/nm-ui-utils.h \ src/libnm-gtk/nm-mobile-providers.h \ - src/libnm-gtk/nm-vpn-password-dialog.h + src/libnm-gtk/nm-vpn-password-dialog.h \ + src/libnma/nma-cert-chooser.h you didn't comment on why not cloning the header for libnm-gtk. Don't you think that's a good idea? E.g. <NetworkManager.h> exists for both libraries, but the header has a completely different purpose on both. As such, is the "nma-cert-chooser.h" not self-contained when using as libnm-gtk header. Also, the name for libnm-gtk goes against the pattern. Also, if you would add NM_AVAILABLE_IN macros, it would require to clone them. Hm, maybe we just shouldn't add new API to libnm-gtk. You explain it somewhat in "libnma: add a certificate picker interface" commit message, but I don't understand why you add this to libnm-gtk at all(??). »···NMA_CERT_CHOOSER_FLAG_PEM = 0x4, whitespace. You didn't comment on adding the NM_AVAILABLE macros. They seem a common pattern in glib world, and actually make a lot of sense. Yes, we can add them later... we can even add them posteriori after releasing 1.8. src_libnma_libnma_HEADERS = \ + $(libnma_h_priv) \ $(libnma_h_pub) do you mean noinst_src_libnma_libnma_HEADERS = \ $(libnma_h_priv) ? Otherwise, the name "priv" doesn't seem to match. Also, I would prefer $(libnma_h_priv_real) to mirror $(libnma_h_priv_gen). - $(GUDEV_CFLAGS) + $(GUDEV_CFLAGS) \ + $(GCR_CFLAGS) \ + -DGCR_API_SUBJECT_TO_CHANGE \ + -DGCK_API_SUBJECT_TO_CHANGE Could we group the "-D" options and the $(*_CFLAGS) separately? (I mean, move -D up a few lines). »···model = gtk_combo_box_get_model (GTK_COMBO_BOX (object)); »···if (model) { »···»···g_object_unref (model); »···»···gtk_combo_box_set_model (GTK_COMBO_BOX (object), NULL); »···} I think it's wrong to unref model. If it's correct, it warrants a comment. /* The first entry with current object name. */ trailing whitespace Regarding "src/libnma/nma-cert-chooser-button.c", does the order of the code in the file have a particular significance? In NM I try to put the GObject code to the bottom (exactly) in the following order: #includes typedef struct TypePrivate G_DEFINE_TYPE() // forward declarations // all code get_property() set_property() notify() type_init() constructor() constructed() type_new() dispose() finalize() type_class_init() interface_init() nm-applet arguably is not consistent about that. I wish new code would be. Same for src/libnma/nma-pkcs11-token-login-dialog.c, etc. Pushed a fixup for file_has_extension(). But let's combine these 3 implementations... for example in src/utils/utils.c. And also combine the identical filters, like src/wireless-security/eap-method.c-default_filter_privkey src/libnma/nma-file-cert-chooser.c-privkey_filter
(In reply to Thomas Haller from comment #4) > libnm_gtk_h_pub = \ > src/libnm-gtk/nm-wifi-dialog.h \ > src/libnm-gtk/nm-wireless-dialog.h \ > src/libnm-gtk/nm-mobile-wizard.h \ > src/libnm-gtk/nm-ui-utils.h \ > src/libnm-gtk/nm-mobile-providers.h \ > - src/libnm-gtk/nm-vpn-password-dialog.h > + src/libnm-gtk/nm-vpn-password-dialog.h \ > + src/libnma/nma-cert-chooser.h > > you didn't comment on why not cloning the header for libnm-gtk. Don't you > think that's a good idea? E.g. <NetworkManager.h> exists for both libraries, > but the header has a completely different purpose on both. As such, is the > "nma-cert-chooser.h" not self-contained when using as libnm-gtk header. > Also, the name for libnm-gtk goes against the pattern. > Also, if you would add NM_AVAILABLE_IN macros, it would require to clone > them. Whoops, sorry for not answering that. Anyway, I think it's not a very good idea, but NM_AVAILABLE_IN leaves me with very little other options anyway... > Hm, maybe we just shouldn't add new API to libnm-gtk. You explain it > somewhat in "libnma: add a certificate picker interface" commit message, but > I don't understand why you add this to libnm-gtk at all(??). The same reason this is a public API in the first place -- the VPN plugins. They all support libnm-glib builds. > »···NMA_CERT_CHOOSER_FLAG_PEM = 0x4, > > whitespace. Am I mentally challenged? I thought I have fixed this two times before. Fixed now. Hopefully. > You didn't comment on adding the NM_AVAILABLE macros. They seem a common > pattern in glib world, and actually make a lot of sense. Yes, we can add > them later... we can even add them posteriori after releasing 1.8. Added now. > src_libnma_libnma_HEADERS = \ > + $(libnma_h_priv) \ > $(libnma_h_pub) > > do you mean > > noinst_src_libnma_libnma_HEADERS = \ > $(libnma_h_priv) > ? > Otherwise, the name "priv" doesn't seem to match. Also, I would prefer > $(libnma_h_priv_real) to mirror $(libnma_h_priv_gen). Yes, sort of. > - $(GUDEV_CFLAGS) > + $(GUDEV_CFLAGS) \ > + $(GCR_CFLAGS) \ > + -DGCR_API_SUBJECT_TO_CHANGE \ > + -DGCK_API_SUBJECT_TO_CHANGE > > Could we group the "-D" options and the $(*_CFLAGS) separately? (I mean, > move -D up a few lines). Moved these to configure.ac where they belong. > »···model = gtk_combo_box_get_model (GTK_COMBO_BOX (object)); > »···if (model) { > »···»···g_object_unref (model); > »···»···gtk_combo_box_set_model (GTK_COMBO_BOX (object), NULL); > »···} > > I think it's wrong to unref model. If it's correct, it warrants a comment. Why would you think that? > /* The first entry with current object name. */ > > trailing whitespace Fixed. > Regarding "src/libnma/nma-cert-chooser-button.c", does the order of the code > in the file have a particular significance? In NM I try to put the GObject > code to the bottom (exactly) in the following order: > > #includes > typedef struct TypePrivate > G_DEFINE_TYPE() > // forward declarations > // all code > get_property() > set_property() > notify() > type_init() > constructor() > constructed() > type_new() > dispose() > finalize() > type_class_init() > interface_init() > > nm-applet arguably is not consistent about that. I wish new code would be. > Same for src/libnma/nma-pkcs11-token-login-dialog.c, etc. No reason. Feel free to push a fixup if you care about this. > Pushed a fixup for file_has_extension(). But let's combine these 3 > implementations... for example in src/utils/utils.c. > And also combine the identical filters, like > src/wireless-security/eap-method.c-default_filter_privkey > src/libnma/nma-file-cert-chooser.c-privkey_filter Moved the constructions of the filters there; applied your fixups (thanks for those). Added one more commit before the patchset. Not technically related, but helped avoid mistakes in development.
-GLIB_DEPRECATED +NMA_DEPRECATED_IN_1_4 GtkWidget * nma_wifi_dialog_nag_user (NMAWifiDialog *self); these functions were deprecated since the beginning (yes, we should have dropped them, a mistake). So, you would need NMA_DEPRECATED_IN_1_2 make check reports: make[3]: Entering directory '/data/src/network-manager-applet' ./src/libnm-gtk/check-exports.sh ./src/libnm-gtk/.libs/libnm-gtk.so ./src/libnm-gtk/libnm-gtk.ver >>library "./src/libnm-gtk/.libs/libnm-gtk.so" exports symbols that are not in linker script "./src/libnm-gtk/libnm-gtk.ver": 1 nma_file_cert_chooser_get_type 2 nma_file_cert_chooser_new + if (flags & NMA_CERT_CHOOSER_FLAG_PEM) + return nma_file_cert_chooser_new (title, flags); + + return nma_file_cert_chooser_new (title, flags); looks wrong + * Copyright (C) 2015,2017 Red Hat, Inc. why 2015? + /* The token doesn't have a PIN pad. Ask for PIN. */ + dialog = nma_pkcs11_token_login_dialog_new (priv->slot); + gtk_wind indention :) + model = gtk_combo_box_get_model (GTK_COMBO_BOX (object)); + if (model) { + g_object_unref (model); + gtk_combo_box_set_model (GTK_COMBO_BOX (object), NULL); + } I still don't understand this. gtk_combo_box_get_model() doesn't transfer ownership, so why does "object" own the additional reference that you are freeing in dispose()? rest lgtm
(In reply to Thomas Haller from comment #6) > -GLIB_DEPRECATED > +NMA_DEPRECATED_IN_1_4 > GtkWidget * nma_wifi_dialog_nag_user (NMAWifiDialog *self); > > these functions were deprecated since the beginning (yes, we should have > dropped them, a mistake). So, you would need NMA_DEPRECATED_IN_1_2 I don't know. I see little point in retroactively creating version macros nobody's going to use anyway. > make check reports: > make[3]: Entering directory '/data/src/network-manager-applet' > ./src/libnm-gtk/check-exports.sh ./src/libnm-gtk/.libs/libnm-gtk.so > ./src/libnm-gtk/libnm-gtk.ver > >>library "./src/libnm-gtk/.libs/libnm-gtk.so" exports symbols that are not in linker script "./src/libnm-gtk/libnm-gtk.ver": > 1 nma_file_cert_chooser_get_type > 2 nma_file_cert_chooser_new Err. Fixed. > + if (flags & NMA_CERT_CHOOSER_FLAG_PEM) > + return nma_file_cert_chooser_new (title, flags); > + > + return nma_file_cert_chooser_new (title, flags); > > looks wrong Yeah; the latter patch that adds another chooser fixes that up. Removed the unnecessary conditional from the early commit. > + * Copyright (C) 2015,2017 Red Hat, Inc. > > why 2015? Well, that's was the time stamp of my earlier certificate chooser prototype i copied this from. > + /* The token doesn't have a PIN pad. Ask for PIN. */ > + dialog = nma_pkcs11_token_login_dialog_new (priv->slot); > + gtk_wind > > indention :) Patience, please. My editor hates me. Thanks for the fixup. > + model = gtk_combo_box_get_model (GTK_COMBO_BOX (object)); > + if (model) { > + g_object_unref (model); > + gtk_combo_box_set_model (GTK_COMBO_BOX (object), NULL); > + } > > I still don't understand this. gtk_combo_box_get_model() doesn't transfer > ownership, so why does "object" own the additional reference that you are > freeing in dispose()? Fixed up. Pushed an updated branch.
5703459fba52053b00073fd7ff1c76c4332d633e