From 53f1490b5083af4adadca05fc3a9ce6750525db4 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Tue, 14 May 2019 15:27:45 +0200 Subject: [PATCH] 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) (cherry picked from commit 51896e1e6b24e0b5d6aefce3c4945d27a5b9f5b7) --- 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 0fa2633ce9..a6acb04f33 100644 --- a/Makefile.am +++ b/Makefile.am @@ -2907,6 +2907,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 \ @@ -2976,6 +2977,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 921e757a39..2803230900 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -3110,6 +3110,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, @@ -3145,10 +3149,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; @@ -3158,6 +3164,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 f0ee202307..cde4349457 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