From 5a5cd8d05dfbde11b0983e09a5a37f6929bb2178 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Tue, 14 May 2019 13:59:00 +0200 Subject: [PATCH 1/3] ifcfg-rh: write client certificate even if it is pkcs12 The writer should only persist properties without too much additional logic, which should be instead embedded in the setting itself. (cherry picked from commit a995244e9bf526b2d10143858655c3ea3731bf91) --- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 4 ---- .../plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 24 ++++++------------- 2 files changed, 7 insertions(+), 21 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index e444f1dbfe..1c1fc83aad 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -3117,10 +3117,6 @@ eap_tls_reader (const char *eap_method, &client_cert, error)) return FALSE; - /* FIXME: writer does not actually write IEEE_8021X_CLIENT_CERT_PASSWORD and other - * certificate related passwords. It should, because otherwise persisting such profiles - * to ifcfg looses information. As this currently only matters for PKCS11 URIs, it seems - * a seldom used feature so that it is not fixed yet. */ _secret_set_from_ifcfg (s_8021x, ifcfg, keys_ifcfg, diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index bf181f0595..652cba1190 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -358,23 +358,13 @@ write_8021x_certs (NMSetting8021x *s_8021x, if (!write_object (s_8021x, ifcfg, secrets, blobs, otype, error)) return FALSE; - /* Client certificate */ - if (otype->vtable->format_func (s_8021x) == NM_SETTING_802_1X_CK_FORMAT_PKCS12) { - /* Don't need a client certificate with PKCS#12 since the file is both - * the client certificate and the private key in one file. - */ - svSetValueStr (ifcfg, - phase2 ? "IEEE_8021X_INNER_CLIENT_CERT" : "IEEE_8021X_CLIENT_CERT", - NULL); - } else { - /* Save the client certificate */ - if (!write_object (s_8021x, ifcfg, secrets, blobs, - phase2 - ? &setting_8021x_scheme_vtable[NM_SETTING_802_1X_SCHEME_TYPE_PHASE2_CLIENT_CERT] - : &setting_8021x_scheme_vtable[NM_SETTING_802_1X_SCHEME_TYPE_CLIENT_CERT], - error)) - return FALSE; - } + /* Save the client certificate */ + if (!write_object (s_8021x, ifcfg, secrets, blobs, + phase2 + ? &setting_8021x_scheme_vtable[NM_SETTING_802_1X_SCHEME_TYPE_PHASE2_CLIENT_CERT] + : &setting_8021x_scheme_vtable[NM_SETTING_802_1X_SCHEME_TYPE_CLIENT_CERT], + error)) + return FALSE; return TRUE; } From c28db67a781388e1f742b3406e26a35c8c2522a8 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Tue, 14 May 2019 14:32:19 +0200 Subject: [PATCH 2/3] ifcfg-rh: don't check for 802.1x private key or client cert in reader Let the setting check it in verify(). (cherry picked from commit d9b3b2b8cec9fdb984a6103240688dc46f33866e) --- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 1c1fc83aad..01c884c315 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -3077,6 +3077,7 @@ eap_tls_reader (const char *eap_method, svGetValueStr (ifcfg, "IEEE_8021X_IDENTITY", &identity_free), NULL); + /* CA certificate */ if (!_cert_set_from_ifcfg (s_8021x, ifcfg, phase2 ? "IEEE_8021X_INNER_CA_CERT" : "IEEE_8021X_CA_CERT", @@ -3090,6 +3091,7 @@ eap_tls_reader (const char *eap_method, phase2 ? "IEEE_8021X_INNER_CA_CERT_PASSWORD" : "IEEE_8021X_CA_CERT_PASSWORD", phase2 ? NM_SETTING_802_1X_PHASE2_CA_CERT_PASSWORD : NM_SETTING_802_1X_CA_CERT_PASSWORD); + /* Private key */ if (!_cert_set_from_ifcfg (s_8021x, ifcfg, phase2 ? "IEEE_8021X_INNER_PRIVATE_KEY" : "IEEE_8021X_PRIVATE_KEY", @@ -3102,14 +3104,8 @@ eap_tls_reader (const char *eap_method, keys_ifcfg, phase2 ? "IEEE_8021X_INNER_PRIVATE_KEY_PASSWORD" : "IEEE_8021X_PRIVATE_KEY_PASSWORD", phase2 ? NM_SETTING_802_1X_PHASE2_PRIVATE_KEY_PASSWORD : NM_SETTING_802_1X_PRIVATE_KEY_PASSWORD); - if (!privkey) { - g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, - "Missing %s for EAP method '%s'.", - phase2 ? "IEEE_8021X_INNER_PRIVATE_KEY" : "IEEE_8021X_PRIVATE_KEY", - eap_method); - return FALSE; - } + /* Client certificate */ if (!_cert_set_from_ifcfg (s_8021x, ifcfg, phase2 ? "IEEE_8021X_INNER_CLIENT_CERT" : "IEEE_8021X_CLIENT_CERT", @@ -3122,12 +3118,6 @@ eap_tls_reader (const char *eap_method, keys_ifcfg, phase2 ? "IEEE_8021X_INNER_CLIENT_CERT_PASSWORD" : "IEEE_8021X_CLIENT_CERT_PASSWORD", phase2 ? NM_SETTING_802_1X_PHASE2_CLIENT_CERT_PASSWORD : NM_SETTING_802_1X_CLIENT_CERT_PASSWORD); - if (!client_cert) { - g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, - "Missing certificate for EAP method '%s'.", - eap_method); - return FALSE; - } return TRUE; } From 51896e1e6b24e0b5d6aefce3c4945d27a5b9f5b7 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Tue, 14 May 2019 15:27:45 +0200 Subject: [PATCH 3/3] ifcfg-rh: use PKCS #12 private key also as client cert in reader Before commit e3ac45c02610 the reader set the private key in the setting using the libnm function, which also set the key as client certificate if it was in PKCS #12 format. After the commit, existing connections with a PKCS #12 private key but without a client certificate became invalid. Restore the old behavior. Fixes: e3ac45c02610 ('ifcfg-rh: don't use 802-1x certifcate setter functions') (cherry picked from commit 9a410fc312c50ac405c57ff4e9eb692e798e248d) --- Makefile.am | 2 ++ .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 28 ++++++++++++++++-- .../plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 21 +++++++++---- ...fg-test-wired-8021x-tls-p12-no-client-cert | 13 ++++++++ .../tests/network-scripts/test_client.p12 | Bin 0 -> 2848 bytes .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 23 ++++++++++++++ 6 files changed, 79 insertions(+), 8 deletions(-) create mode 100644 src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wired-8021x-tls-p12-no-client-cert create mode 100644 src/settings/plugins/ifcfg-rh/tests/network-scripts/test_client.p12 diff --git a/Makefile.am b/Makefile.am index d78bfdeda8..8c470df31a 100644 --- a/Makefile.am +++ b/Makefile.am @@ -3014,6 +3014,7 @@ EXTRA_DIST += \ src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wired-8021x-peap-mschapv2 \ src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wired-8021x-tls-agent \ src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wired-8021x-tls-always \ + src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wired-8021x-tls-p12-no-client-cert \ src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wired-auto-negotiate-on \ src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wired-autoip \ src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wired-ctc-static \ @@ -3083,6 +3084,7 @@ EXTRA_DIST += \ src/settings/plugins/ifcfg-rh/tests/network-scripts/route6-test-wired-ipv6-manual \ src/settings/plugins/ifcfg-rh/tests/network-scripts/test1_key_and_cert.pem \ src/settings/plugins/ifcfg-rh/tests/network-scripts/test_ca_cert.pem \ + src/settings/plugins/ifcfg-rh/tests/network-scripts/test_client.p12 \ $(NULL) # make target dependencies can't have colons in their names, which ends up diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 01c884c315..379cdd876a 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -3071,6 +3071,10 @@ eap_tls_reader (const char *eap_method, gs_unref_bytes GBytes *privkey = NULL; gs_unref_bytes GBytes *client_cert = NULL; gs_free char *identity_free = NULL; + gs_free char *value_to_free = NULL; + const char *client_cert_var; + const char *client_cert_prop; + NMSetting8021xCKFormat format; g_object_set (s_8021x, NM_SETTING_802_1X_IDENTITY, @@ -3106,10 +3110,12 @@ eap_tls_reader (const char *eap_method, phase2 ? NM_SETTING_802_1X_PHASE2_PRIVATE_KEY_PASSWORD : NM_SETTING_802_1X_PRIVATE_KEY_PASSWORD); /* Client certificate */ + client_cert_var = phase2 ? "IEEE_8021X_INNER_CLIENT_CERT" : "IEEE_8021X_CLIENT_CERT"; + client_cert_prop = phase2 ? NM_SETTING_802_1X_PHASE2_CLIENT_CERT : NM_SETTING_802_1X_CLIENT_CERT; if (!_cert_set_from_ifcfg (s_8021x, ifcfg, - phase2 ? "IEEE_8021X_INNER_CLIENT_CERT" : "IEEE_8021X_CLIENT_CERT", - phase2 ? NM_SETTING_802_1X_PHASE2_CLIENT_CERT : NM_SETTING_802_1X_CLIENT_CERT, + client_cert_var, + client_cert_prop, &client_cert, error)) return FALSE; @@ -3119,6 +3125,24 @@ eap_tls_reader (const char *eap_method, phase2 ? "IEEE_8021X_INNER_CLIENT_CERT_PASSWORD" : "IEEE_8021X_CLIENT_CERT_PASSWORD", phase2 ? NM_SETTING_802_1X_PHASE2_CLIENT_CERT_PASSWORD : NM_SETTING_802_1X_CLIENT_CERT_PASSWORD); + /* In the past when the private key and client certificate + * were the same PKCS #12 file we used to write only the + * private key variable. Still support that even if it means + * that we have to look into the file content, which makes + * the connection not self-contained. + */ + if ( !client_cert + && privkey + && !svGetValue (ifcfg, client_cert_var, &value_to_free)) { + if (phase2) + format = nm_setting_802_1x_get_phase2_private_key_format (s_8021x); + else + format = nm_setting_802_1x_get_private_key_format (s_8021x); + + if (format == NM_SETTING_802_1X_CK_FORMAT_PKCS12) + g_object_set (s_8021x, client_cert_prop, privkey, NULL); + } + return TRUE; } diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index 652cba1190..d692241a1a 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -209,6 +209,7 @@ write_object (NMSetting8021x *s_8021x, GHashTable *secrets, GHashTable *blobs, const Setting8021xSchemeVtable *objtype, + gboolean force_write, GError **error) { NMSetting8021xCKScheme scheme; @@ -287,7 +288,7 @@ write_object (NMSetting8021x *s_8021x, */ standard_file = utils_cert_path (svFileGetName (ifcfg), objtype->vtable->file_suffix, extension); g_hash_table_replace (blobs, standard_file, NULL); - svUnsetValue (ifcfg, objtype->ifcfg_rh_key); + svSetValue (ifcfg, objtype->ifcfg_rh_key, force_write ? "" : NULL); return TRUE; } @@ -338,31 +339,39 @@ write_8021x_certs (NMSetting8021x *s_8021x, shvarFile *ifcfg, GError **error) { - const Setting8021xSchemeVtable *otype = NULL; + const Setting8021xSchemeVtable *pk_otype = NULL; + gs_free char *value_to_free = NULL; /* CA certificate */ if (!write_object (s_8021x, ifcfg, secrets, blobs, phase2 ? &setting_8021x_scheme_vtable[NM_SETTING_802_1X_SCHEME_TYPE_PHASE2_CA_CERT] : &setting_8021x_scheme_vtable[NM_SETTING_802_1X_SCHEME_TYPE_CA_CERT], + FALSE, error)) return FALSE; /* Private key */ if (phase2) - otype = &setting_8021x_scheme_vtable[NM_SETTING_802_1X_SCHEME_TYPE_PHASE2_PRIVATE_KEY]; + pk_otype = &setting_8021x_scheme_vtable[NM_SETTING_802_1X_SCHEME_TYPE_PHASE2_PRIVATE_KEY]; else - otype = &setting_8021x_scheme_vtable[NM_SETTING_802_1X_SCHEME_TYPE_PRIVATE_KEY]; + pk_otype = &setting_8021x_scheme_vtable[NM_SETTING_802_1X_SCHEME_TYPE_PRIVATE_KEY]; /* Save the private key */ - if (!write_object (s_8021x, ifcfg, secrets, blobs, otype, error)) + if (!write_object (s_8021x, ifcfg, secrets, blobs, pk_otype, FALSE, error)) return FALSE; - /* Save the client certificate */ + /* Save the client certificate. + * If there is a private key, always write a property for the + * client certificate even if it is empty, so that the reader + * doesn't have to read the private key file to determine if it + * is a PKCS #12 one which serves also as client certificate. + */ if (!write_object (s_8021x, ifcfg, secrets, blobs, phase2 ? &setting_8021x_scheme_vtable[NM_SETTING_802_1X_SCHEME_TYPE_PHASE2_CLIENT_CERT] : &setting_8021x_scheme_vtable[NM_SETTING_802_1X_SCHEME_TYPE_CLIENT_CERT], + !!svGetValue (ifcfg, pk_otype->ifcfg_rh_key, &value_to_free), error)) return FALSE; diff --git a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wired-8021x-tls-p12-no-client-cert b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wired-8021x-tls-p12-no-client-cert new file mode 100644 index 0000000000..2439747352 --- /dev/null +++ b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wired-8021x-tls-p12-no-client-cert @@ -0,0 +1,13 @@ +# Intel Corporation 82540EP Gigabit Ethernet Controller (Mobile) +TYPE=Ethernet +DEVICE=eth0 +HWADDR=00:11:22:33:44:ee +BOOTPROTO=dhcp +ONBOOT=yes +NM_CONTROLLED=yes +KEY_MGMT=IEEE8021X +IEEE_8021X_EAP_METHODS=TLS +IEEE_8021X_IDENTITY="David Smith" +IEEE_8021X_CA_CERT=test_ca_cert.pem +IEEE_8021X_PRIVATE_KEY=test_client.p12 +IEEE_8021X_PRIVATE_KEY_PASSWORD="test1" diff --git a/src/settings/plugins/ifcfg-rh/tests/network-scripts/test_client.p12 b/src/settings/plugins/ifcfg-rh/tests/network-scripts/test_client.p12 new file mode 100644 index 0000000000000000000000000000000000000000..edc2af75ca6beabf5b4997ebcdf637e0408a296a GIT binary patch literal 2848 zcmY+^c{CJ^8U}E~%oq$a2HBT{EX6R%zKg-A>`eAO>t}0Fwn; zh-V)C9S#dnA``v4_%cfC9ztz5?>A0~<^-RhTr&yl=40G9ar zx^zB;$gUT=CA$2EyV`E}PX7_oTe~u+N{3`>LCEsa3liw|sB+2jhdvA_A?TfB05ptYZWa#FPTXF7mg(quA$RB7|(G=8!( z<0IiD?dFgT8c? zF(EYVvMe0BLQrs}R=Wq;%BuPe+2MlC{Rx>)WGo`Rrq-oC0tU&?r=#JT%^0^cN_Qsl zpXQp0o~2K%9`AO8LiuzIg43yGOsvE3ceBO*KZUZ~78$Io#R=?9`fsZ>hhNTIb?anR zwtA@`2sD2tv(&|l#H4#$Z?fEOOe*+pl*}z`z+&BTEU+(4EU<@7?|eTH0fG5gPJeui zjy7V$vc*7yq0$b{+FE34QFb}uSCVaJ5>3wXhi?yi+bhFM<%!2*x z?nAC;2?g4Vpmb{P5|2qiAvWrnGUIeD$h_cvfLm2J*6gEW zn`4d~C)R=uRHZU&sAU}R{E_Rg^MV%Bu9`4z!x3?NoAa!)VcUbQ;{aWkdyV{`yKL&8 zAj}U?lUK?=^uZ>oNdm-cE>w+*#u&+6q5K5)OcCohJMFNx;v7C9Sb%pny`g}oP zUD~ydGatk?kD`|8+_O+VevN^pFIy`cdovAQ+~uj#mh6Ighv+!h9)~GvlCHa1M^A!v zBj10$8~E(DpK=5&J_=xr$7PTAUKkv<5Dr79MEZWl)X%MJql?IoBuo=?-pSS0&|gGdUO3m|3lWMFtX9$OxVmN)s-RJJc`tf1 zPaTj!5#efM>SEC~U)T*oUe&+P*V=YhX?`{es!Uxx{JGANF}`+ib#_Z^X)uG*bpu!g zOW;d9_&U{odC~1vN1GJb8MSvhAaVXyK7OOFK1MgXaOdo zZBD)C1y2PC;z8eXm?eEkuS(G&cC4k9@4tW-UX34mP2D*_FbF+7qqpv&DJFcLTjrAS z{?gX9_({3jUmU^*>(aKp)r!Cb7Y^`W>AbYjWQ`IZfu&T(cutqlol)#&$FZ; z)=sMQ@WJZ_(!2`Gh*I$>C{m~HkKD*W1qZX?0LUFOjbr)pNs$EM{C=aYp8Bf$K%=L4 zUGkx+Qd6Fr;iz7xB6|(}Mt_i+EEKkRa-#a&@irOUC4J=N6BaT1;oJy!*rFm0Pus}>jAkdx-N?0?t6I~!o$*0@aT7J zJxC0n0~Zf_23;w>TxmTQ6($i`9I+xC>(9Hox?;coJ-^{N4xQycq4eWuKPID5g*hXg zt1NNV#y2N5m)sXBp=-Eot4^$ymq#8Rt9<8X=pbAGFiNnNnOyyX!%7Lo7F+X=G&pz~ ztzf${=ZsuWJYGM@xNijP@g-># z{ECnEGHJ|c)v;A+eXEf5pS^&oL9bW-!c{6WeTenSgB!QRGq{l9OAH4)_;B3*-I)sNm#rx<{yK^hz4sNcu=GMefp?7J z8vSBr!=~uOVinfh-d&CO>UR-!E0;Bakd#Q<{yF|jRp;orrK^=@`w{hxx_NjC?JcBZ zRb0PC1-CMi`=pNLDS>H;FKqDb!b9W^=Hn(cxklpSnM|U+ zY=RDSw?!rkF>x+-CcnWVR^`qA29{(}0`0XcI>Mj_E)Xg>I^fB>5=AeZh_1rZE3} zi^EgMuS&kVTRrXeh~Um;QDn`Uecjm&j(rL`5oiw`w68Xdti>YSdm(;ddn*D->2MxZ zJ6|lnnA@jh+-WdlZ-q#+w+4&Fjdrx9ghe%_UV2vfOhM)e)3>#@-B+Wgr4dZh);~bH zJt5By_TJ`M6q1MCE_%e8tIyZ`y`-6jl