Commit e3ac45c0 authored by Thomas Haller's avatar Thomas Haller

ifcfg-rh: don't use 802-1x certifcate setter functions

The certificate setter function like nm_setting_802_1x_set_ca_cert()
actually load the file from disk, and validate whether it is a valid
certificate. That is very wrong to do.

For one, the certificates are external files, which are not embedded
into the NMConnection. That means, strongly validating the files while
loading the ifcfg files, is wrong because:
 - if validation fails, loading the file fails in its entirety with
   a warning in the log. That is not helpful to the user, who now
   can no longer use nmcli to fix the path of the certificate (because
   the profile failed to load in the first place).
 - even if the certificate is valid at load-time, there is no guarantee
   that it is valid later on, when we actually try to use the file. What
   good does such a validation do? nm_setting_802_1x_set_ca_cert() might
   make sense during nmcli_connection_modify(). At the moment when we
   create or update the profile, we do want to validate the input and
   be helpful to the user. Validating the file later on, when reloading
   the profile from disk seems undesirable.
 - note how keyfile also does not perform such validations (for good
   reasons, I presume).

Also, there is so much wrong with how ifcfg reader handles EAP files.
There is a lot of duplication, and trying to be too smart. I find it
wrong how the "eap_readers" are nested. E.g. both eap_peap_reader() and
"tls" method call to eap_tls_reader(), making it look like that
NMSetting8021x can handle multiple EAP profiles separately. But it cannot. The
802-1x profile is a flat set of properties like ca-cert and others. All
EAP methods share these properties, so having this complex parsing
is not only complicated, but also wrong. The reader should simply parse
the shell variables, and let NMSetting8021x::verify() handle validation
of the settings. Anyway, the patch does not address that.

Also, the setting of the likes of NM_SETTING_802_1X_CLIENT_CERT_PASSWORD was
awkwardly only done when
     privkey_format != NM_SETTING_802_1X_CK_FORMAT_PKCS12
  && scheme == NM_SETTING_802_1X_CK_SCHEME_PKCS11
It is too smart. Just read it from file, if it contains invalid data, let
verify() reject it. That is only partly addressed.

Also note, how writer never actually writes the likes of
IEEE_8021X_CLIENT_CERT_PASSWORD. That is another bug and not fixed
either.
parent 6b763af1
......@@ -588,4 +588,13 @@ const NMSettInfoProperty *_nm_sett_info_property_get (NMSettingClass *setting_cl
/*****************************************************************************/
NMSetting8021xCKScheme _nm_setting_802_1x_cert_get_scheme (GBytes *bytes, GError **error);
GBytes *_nm_setting_802_1x_cert_value_to_bytes (NMSetting8021xCKScheme scheme,
const guint8 *val_bin,
gssize val_len,
GError **error);
/*****************************************************************************/
#endif
......@@ -279,8 +279,8 @@ nm_setting_802_1x_check_cert_scheme (gconstpointer pdata, gsize length, GError *
return scheme;
}
static NMSetting8021xCKScheme
_cert_get_scheme (GBytes *bytes, GError **error)
NMSetting8021xCKScheme
_nm_setting_802_1x_cert_get_scheme (GBytes *bytes, GError **error)
{
const char *data;
gsize length;
......@@ -307,7 +307,7 @@ _cert_verify_scheme (NMSetting8021xCKScheme scheme,
nm_assert (bytes);
scheme_detected = _cert_get_scheme (bytes, &local);
scheme_detected = _nm_setting_802_1x_cert_get_scheme (bytes, &local);
if (scheme_detected == NM_SETTING_802_1X_CK_SCHEME_UNKNOWN) {
g_set_error (error,
NM_CONNECTION_ERROR,
......@@ -327,11 +327,11 @@ _cert_verify_scheme (NMSetting8021xCKScheme scheme,
return TRUE;
}
static GBytes *
_cert_value_to_bytes (NMSetting8021xCKScheme scheme,
const guint8 *val_bin,
gssize val_len,
GError **error)
GBytes *
_nm_setting_802_1x_cert_value_to_bytes (NMSetting8021xCKScheme scheme,
const guint8 *val_bin,
gssize val_len,
GError **error)
{
gs_unref_bytes GBytes *bytes = NULL;
guint8 *mem;
......@@ -388,7 +388,7 @@ _cert_get_path (GBytes *bytes)
G_STMT_START { \
NMSetting8021xCKScheme scheme; \
\
scheme = _cert_get_scheme ((cert), NULL); \
scheme = _nm_setting_802_1x_cert_get_scheme ((cert), NULL); \
if (scheme != check_scheme) { \
g_return_val_if_fail (scheme == check_scheme, ret_val); \
return ret_val; \
......@@ -404,7 +404,7 @@ _cert_get_path (GBytes *bytes)
\
_cert = NM_SETTING_802_1X_GET_PRIVATE (_setting)->cert_field; \
\
return _cert_get_scheme (_cert, NULL); \
return _nm_setting_802_1x_cert_get_scheme (_cert, NULL); \
} G_STMT_END
#define _cert_impl_get_blob(setting, cert_field) \
......@@ -487,7 +487,7 @@ _cert_impl_set (NMSetting8021x *setting,
if (!value) {
/* pass. */
} else if (scheme == NM_SETTING_802_1X_CK_SCHEME_PKCS11) {
cert = _cert_value_to_bytes (scheme, (guint8 *) value, -1, error);
cert = _nm_setting_802_1x_cert_value_to_bytes (scheme, (guint8 *) value, -1, error);
if (!cert)
goto err;
} else {
......@@ -518,7 +518,7 @@ _cert_impl_set (NMSetting8021x *setting,
if (!_cert_verify_scheme (scheme, cert, error))
goto err;
} else {
cert = _cert_value_to_bytes (scheme, (guint8 *) value, -1, error);
cert = _nm_setting_802_1x_cert_value_to_bytes (scheme, (guint8 *) value, -1, error);
if (!cert)
goto err;
}
......@@ -627,7 +627,7 @@ _cert_impl_get_key_format_from_bytes (GBytes *private_key)
if (!private_key)
return NM_SETTING_802_1X_CK_FORMAT_UNKNOWN;
switch (_cert_get_scheme (private_key, NULL)) {
switch (_nm_setting_802_1x_cert_get_scheme (private_key, NULL)) {
case NM_SETTING_802_1X_CK_SCHEME_BLOB:
if (nm_crypto_is_pkcs12_data (g_bytes_get_data (private_key, NULL),
g_bytes_get_size (private_key),
......@@ -674,7 +674,7 @@ _cert_verify_property (GBytes *bytes,
if (!bytes)
return TRUE;
scheme = _cert_get_scheme (bytes, &local);
scheme = _nm_setting_802_1x_cert_get_scheme (bytes, &local);
if (scheme == NM_SETTING_802_1X_CK_SCHEME_UNKNOWN) {
g_set_error (error,
NM_CONNECTION_ERROR,
......
......@@ -1901,10 +1901,8 @@ test_read_write_802_1X_subj_matches (void)
gs_unref_object NMConnection *reread = NULL;
NMSetting8021x *s_8021x;
NMTST_EXPECT_NM_WARN ("*missing IEEE_8021X_CA_CERT*peap*");
connection = _connection_from_file (TEST_IFCFG_DIR"/ifcfg-test-wired-802-1X-subj-matches",
NULL, TYPE_ETHERNET, NULL);
g_test_assert_expected_messages ();
/* ===== 802.1x SETTING ===== */
s_8021x = nm_connection_get_setting_802_1x (connection);
......@@ -1922,16 +1920,12 @@ test_read_write_802_1X_subj_matches (void)
g_assert_cmpstr (nm_setting_802_1x_get_phase2_altsubject_match (s_8021x, 0), ==, "x.yourdomain.tld");
g_assert_cmpstr (nm_setting_802_1x_get_phase2_altsubject_match (s_8021x, 1), ==, "y.yourdomain.tld");
NMTST_EXPECT_NM_WARN ("*missing IEEE_8021X_CA_CERT for EAP method 'peap'; this is insecure!");
_writer_new_connec_exp (connection,
TEST_SCRATCH_DIR,
TEST_IFCFG_DIR"/ifcfg-System_test-wired-802-1X-subj-matches.cexpected",
&testfile);
g_test_assert_expected_messages ();
NMTST_EXPECT_NM_WARN ("*missing IEEE_8021X_CA_CERT for EAP method 'peap'; this is insecure!");
reread = _connection_from_file (testfile, NULL, TYPE_ETHERNET, NULL);
g_test_assert_expected_messages ();
nmtst_assert_connection_equals (connection, TRUE, reread, FALSE);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment