ifcfg-rh: use PKCS #12 private key also as client cert in readerbg/802-1x-client-cert-rh1705054

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')

6 files changed, 82 insertions, 8 deletions

diff --git a/Makefile.am b/Makefile.am
index 3fa232b84c..b69d1701a9 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2988,6 +2988,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 \
@@ -3057,6 +3058,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 d19cafd86b..4f7a2e2583 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 9934042111..17209b9295 100644
--- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c
+++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c
@@ -197,6 +197,7 @@ write_object (NMSetting8021x *s_8021x,
               GHashTable *secrets,
               GHashTable *blobs,
               const Setting8021xSchemeVtable *objtype,
+              gboolean force_write,
               GError **error)
 {
 	NMSetting8021xCKScheme scheme;
@@ -274,7 +275,10 @@ 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);
+	if (force_write)
+		svSetValue(ifcfg, objtype->ifcfg_rh_key, "");
+	else
+		svUnsetValue (ifcfg, objtype->ifcfg_rh_key);
 	return TRUE;
 }
 
@@ -325,31 +329,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 0000000000..edc2af75ca
--- /dev/null
+++ b/src/settings/plugins/ifcfg-rh/tests/network-scripts/test_client.p12
diff --git a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
index 49ab04d4db..eaaa749052 100644
--- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
+++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
@@ -1974,6 +1974,27 @@ test_read_802_1x_ttls_eapgtc (void)
 }
 
 static void
+test_read_802_1x_tls_p12_no_client_cert (void)
+{
+	gs_unref_object NMConnection *connection = NULL;
+	NMSetting8021x *s_8021x;
+	const char *path;
+
+	connection = _connection_from_file (TEST_IFCFG_DIR"/ifcfg-test-wired-8021x-tls-p12-no-client-cert",
+	                                    NULL, TYPE_ETHERNET, NULL);
+
+	s_8021x = nm_connection_get_setting_802_1x (connection);
+	g_assert (s_8021x);
+
+	g_assert_cmpint (nm_setting_802_1x_get_private_key_scheme (s_8021x), ==, NM_SETTING_802_1X_CK_SCHEME_PATH);
+	path = nm_setting_802_1x_get_private_key_path (s_8021x);
+	g_assert (path);
+
+	g_assert_cmpint (nm_setting_802_1x_get_client_cert_scheme (s_8021x), ==, NM_SETTING_802_1X_CK_SCHEME_PATH);
+	g_assert_cmpstr (path, ==, nm_setting_802_1x_get_client_cert_path (s_8021x));
+}
+
+static void
 test_read_write_802_1x_password_raw (void)
 {
 	nmtst_auto_unlinkfile char *testfile = NULL;
@@ -10223,6 +10244,8 @@ int main (int argc, char **argv)
 	g_test_add_func (TPATH "802-1x/subj-matches", test_read_write_802_1X_subj_matches);
 	g_test_add_func (TPATH "802-1x/ttls-eapgtc", test_read_802_1x_ttls_eapgtc);
 	g_test_add_func (TPATH "802-1x/password_raw", test_read_write_802_1x_password_raw);
+	g_test_add_func (TPATH "802-1x/tls-p12-no-client-cert", test_read_802_1x_tls_p12_no_client_cert);
+
 	g_test_add_data_func (TPATH "wired/read/aliases/good/0", GINT_TO_POINTER (0), test_read_wired_aliases_good);
 	g_test_add_data_func (TPATH "wired/read/aliases/good/3", GINT_TO_POINTER (3), test_read_wired_aliases_good);
 	g_test_add_func (TPATH "wired/read/aliases/bad1", test_read_wired_aliases_bad_1);