summaryrefslogtreecommitdiff
Commit message (Collapse)AuthorAgeFilesLines
* settings/ifupdown: use _NMLOG() macros for loggingth/settings-cleanupThomas Haller2018-09-053-58/+88
| | | | | | | | Does not address the issues that the existing logging is much too verbose and does not provide necessary context for what it's complaining. The logging messages should be improved. At least, the logging macro gives all messages a "ifupdown: " prefix.
* settings/ifupdown: various cleanup in nms-ifupdown-parser.cThomas Haller2018-09-051-47/+32
|
* settings/ifupdown: optimize allocating parser dataThomas Haller2018-09-053-18/+24
|
* settings/ifupdown: use c-list for data structure of ifupdown parserThomas Haller2018-09-055-127/+114
| | | | We already have a linked-list implementation. Use it.
* settings/ifupdown: don't use global variables for /e/n/i parserThomas Haller2018-09-054-185/+163
|
* settings/ifupdown: hide internal functions in "nms-ifupdown-interface-parser.h"Thomas Haller2018-09-052-9/+4
|
* settings/ifupdown: refactor string prefix matching in parserThomas Haller2018-09-051-27/+28
|
* settings/ifupdown: use nm_streq() in parserThomas Haller2018-09-052-47/+45
|
* settings/ifupdown: adjust coding style for "nms-ifupdown-interface-parser"Thomas Haller2018-09-053-192/+193
|
* settings/ifupdown: in plugin drop listening to udev for devicesThomas Haller2018-09-051-192/+4
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Don't listen to udev to find out about devices. First of all, using udev for this is already very wrong, because we have the platform cache. Anyway, all that the device information is used, is pointless as well. Drop it. It's pointless because: The entires in eni_ifaces are already indexed by the interface name. Likewise, all NMIfupdownConnection set "connection.interface-name" to restict the profile by name. /e/n/i matches devices is by name, that's it. We don't need udev to look up the MAC address (by name!!) to later ignore/match devices by MAC address. Especially, because NetworkMaanger can already restrict profiles to devices based on the interface name. Likewise, NetworkMaanger can use the interface name for the unmanaged-specs. It's wrong to extend the on-disk configuration from /e/n/i with runtime information from udev, especially, because other NetworkMaanger layers are perfectly content using the interface name for this purpose. Also, bind_device_to_connection() was fundamentally wrong. It's wrong to modify the settings connection at random moments (on udev event). If at all, that should only happen during connection load/reload. This may have been necessary a long time ago, when unmanaged devices were not expressed by device-match-specs, but by the HAL UDI. That was since improved, for example by commit c9067d8fedf6f6f2d530fd68bbfca7ce68638d38.
* settings/ifupdown: merge eni_ifaces and connections hashes in pluginThomas Haller2018-09-051-50/+63
| | | | | | | | | | | The "connections" hash contains a mapping of block->name (iface) to the NMSettingsConnection. The "eni_ifaces" hash contains the name of all blocks which are mentioned, but for which no connection was created. Merge the two hashes. We don't need to keep track of whether a connections was successfully created ("connections"), but the same name also has a non-connection block. This information is unnecessary, so one hash is enough.
* settings/ifupdown: change plugin's field @unmanage_well_known to ↵Thomas Haller2018-09-051-12/+12
| | | | | | | | | | | @ifupdown_managed @unmanage_well_known directly depends on the "ifupdown.managed" setting from NetworkManager.conf. Rename it (and invert the meaning) so that this relation ship becomes clearer. Also, the double negation of "if (!unmanaged_well_known)" hurts the brain.
* settings/ifupdown: drop unused define ALWAYS_UNMANAGE in pluginThomas Haller2018-09-051-9/+4
| | | | | | | | | This is only useful for testing. But since the managed flag is configurable via NetworkManager.conf, there is no point in having a define as well. If you want to test it, just configure it. And if you really want to patch the code, then patch "priv->unmanage_well_known" to be always TRUE.
* settings/ifupdown: cleanup plugin's loggingThomas Haller2018-09-051-31/+37
| | | | | | | | | | | | | | - use _NMLOG() macro and give logging message a sensible prefix - downgrade logging severity. Most of these messages are not important to warrant <info> or <warn> level. - the logging is generally rather bad. Messages like "bind-to-connection: locking wired connection setting" don't indicate which profile is locked to which MAC address. TODO.
* settings/ifupdown: cleanup plugin's get_connections()Thomas Haller2018-09-051-11/+7
|
* settings/ifupdown: cleanup parsing bridge in plugin's initialize()Thomas Haller2018-09-051-7/+8
|
* settings/ifupdown: refactor parsing loop in plugin's initialize()Thomas Haller2018-09-051-8/+15
|
* settings/ifupdown: replace strcmp() usage with nm_streq()/NM_IN_STRSET() in ↵Thomas Haller2018-09-051-15/+13
| | | | plugin
* settings/ifupdown: minor cleanup of auto-ifaces in plugin's initialize()Thomas Haller2018-09-051-13/+14
| | | | | | | | | | | | | - use gs_unref_hashtable for managing lifetime - only allocate the hashtable if necessary, and use g_hash_table_add() which is optimized by HashTable. - actually copy the block->name that is used as key. While not necessary at the moment, it is very ugly how ifparser_getfirst() returns static data. Optimally, this would be fixed and we create and destroy the parser results. Hence, ensure the lifetime of the key.
* settings/ifupdown: cleanup lifetime and memory handling of dictionaries in ↵Thomas Haller2018-09-051-15/+13
| | | | | | | | | | | | | | plugin - initialize the hash tables in the plugins constructor, not during initialize(). - let all dictionaries own a copy/reference of the keys and values, and properly free them when the values are removed. In general, avoid leaks by properly managing lifetimes. - in @eni_ifaces, don't add a pointless dummy value "known". It has overhead for no benefit.
* settings: cleanup loading settings pluginsThomas Haller2018-09-052-66/+46
| | | | Drop the unnecessary @list argument and various cleanups.
* settings: disconnect signals from plugins when destroying NMSettingsThomas Haller2018-09-051-1/+7
| | | | | Currently we anyway leak everything on shutdown, so this doesn't matter. But to be correct, we must disconnect signal handlers.
* settings: make NMSettingsPlugin a regular GObject instance and not an interfaceThomas Haller2018-09-056-205/+224
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | NMSettingsPlugin was a glib interface, not a regular GObject instance. Accordingly, settings plugins would implement this interface instead of subclassing a parent type. Refactor the code, and make NMSettingsPlugin a GObject type. Plugins are now required to subclass this type. Glib interfaces are more cumbersome than helpful. At least, unless there is a good reason for using them. Our settings plugins are all internal API and are entirely under our control. It also means, this change is fine, as there are no implementations outside of this source tree. Using interfaces do would allow more flexibility in implementing the settings plugin. For example, the plugin would be able to derive from any other GObject type, like NMKimchiRefrigerator. But why would we even? Let's not add monster classes that implement house appliances beside NMSettingsPluginInterface. The settings plugin should have one purpose only: being a settings plugin. Hence, requiring it to subclass NMSettingsPlugin is more than resonable. We don't need interfaces for this. Now that NMSettingsPlugin is a regular object instance, it may also have state, and potentially could provide common functionality for the plugin implementation -- if that turns out to be useful. Arguably, an interface can have state too, for example by attaching the state somewhere else (like NMConnection does). But let's just say no. On a minor note, this also avoids some tiny overhead that comes with glib interfaces.
* settings: drop unused get_plugin() checksThomas Haller2018-09-051-31/+1
| | | | | | Nowadays, keyfile settings plugin is always loaded. Hence, this function never returns %NULL and the checks always evalute the the same.
* settings: rename NMSettingsPluginInterface.init() to initialize()Thomas Haller2018-09-054-9/+9
| | | | | | | | | | | | The virtual function init() naturally leads to calling the wrapper function nm_settings_plugin_init(). However, such ${TYPE}_init() functions are generated by G_DEFINE_TYPE(). Rename to avoid the naming conflict, which will matter next, when the interface will be converted to a regular GObject class. Note that while these are settings plugin, there is no public or stable API which we need to preserve.
* settings: remove empty NMSettingsPluginInterface.init() implementationsThomas Haller2018-09-052-12/+0
|
* settings/keyfile: always return path from nms_keyfile_writer_connection()Thomas Haller2018-09-052-8/+3
| | | | | | | | | Previously, nms_keyfile_writer_connection() would only return @out_path, if it differed from @existing_path. That might make sense, if we could thereby avoid duplicating @existing_path, however, we never did that optimization. Just consistently always return the path, let the caller deal with this.
* libnm: assert in nm_utils_is_uuid() for valid argumentThomas Haller2018-09-051-0/+2
| | | | | nm_utils_is_uuid() is public API. Commonly we check input arguments for such functions with g_return_*() assertions.
* ifupdown: properly handle special "none" keyword for bridge_portsMichael Biebl2018-09-051-0/+3
| | | | | | | | | If this option is set, we should not add a device named "none" but simply don't add any devices at all. https://manpages.debian.org/testing/bridge-utils/bridge-utils-interfaces.5.en.html https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/11
* contrib/rpm: fix invalid %if condition for building on RHELThomas Haller2018-09-041-1/+1
| | | | Fixes: 5ef81dc0fbbe41b85abe7929563d41b03eaf6989
* crypto: merge branch 'th/crypto-secrets'Thomas Haller2018-09-0442-3911/+4274
|\ | | | | | | https://github.com/NetworkManager/NetworkManager/pull/191
| * ifcfg-rh: don't use 802-1x certifcate setter functionsThomas Haller2018-09-044-210/+207
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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.
| * ifcfg-rh: rework parsing secretsThomas Haller2018-09-041-79/+130
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | - rename secret related functions to have a "_secret" prefix. Also, rename read_8021x_password() because it's not only useful for 802-1x. - In particular, this patch adds _secret_read_ifcfg() helper (formerly read_8021x_password()), which is smart enough to obtain secrets from the keys ifcfg file. We have other places where we don't get this right. - on a minor note, the patch also makes an effort to clear passwords and certifcate data from memory. Yes, there are countless places where we don't do that, but in this case, it's done and is as simple as replacing gs_free with nm_auto_free_secret, etc.
| * ifcfg-rh/trivial: rename variable for ifcfg keys fileThomas Haller2018-09-041-59/+59
| | | | | | | | | | | | | | | | The term "keys" is used ambigiously. Rename occurances which reference the "keys" ifcfg-rh file. While at it, rename the file "parsed" to "main_ifcfg". It follows the same pattern as the "keys_ifcfg" name.
| * libnm-core: expose _nm_utils_str2bin_full() as internal APIThomas Haller2018-09-042-10/+17
| | | | | | | | | | | | | | | | | | We only exposed wrappers around this function, but all of them have different behavior, and none exposes all possible features. For example, nm_utils_hexstr2bin() strips leading "0x", but it does not clear the data on failure (nm_explicit_bzero()). Instead of adding more wrappers, expose the underlying implementation, so that callers may use the function the way they want it.
| * libnm/keyfile: clear memory when reading certificates from keyfileThomas Haller2018-09-041-20/+33
| | | | | | | | | | | | | | | | | | Of course, there are countless places where we get it wrong to clear the memory. In particular, glib's GKeyFile implementation does not care about that. Anyway, the keyfile do contain private sensitive data. Adjust a few places to zero out such data from memory.
| * libnm/802-1x: refactor getting private-key formatThomas Haller2018-09-041-67/+63
| | | | | | | | Move similar code to a helper function/macro.
| * libnm/802-1x: refactor setting certificate from pathThomas Haller2018-09-043-506/+243
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | NMSetting8021x has various utility functions to set the certificate: - nm_setting_802_1x_set_ca_cert() - nm_setting_802_1x_set_client_cert() - nm_setting_802_1x_set_private_key() - nm_setting_802_1x_set_phase2_ca_cert() - nm_setting_802_1x_set_phase2_client_cert() - nm_setting_802_1x_set_phase2_private_key() They support: - accepting a plain PKCS11 URI, with scheme set to NM_SETTING_802_1X_CK_SCHEME_PKCS11. - accepting a filename, with scheme set to NM_SETTING_802_1X_CK_SCHEME_BLOB or NM_SETTING_802_1X_CK_SCHEME_PATH. In the latter case, the function tries to load the file and verify it. In case of the private-key setters, this also involves accepting a password. Depending on whether the scheme is BLOB or PATH, the function will either set the certificate to a PATH blob, or take the blob that was read from file. The functions seem misdesigned to me, because their behavior is rather obscure. E.g. they behave fundamentally different, depending on whether scheme is PKCS11 or BLOB/PATH. Anyway, improve them: - refactor the common code into a function _cert_impl_set(). Previously, their non-trivial implementations were copy+pasted several times, now they all use the same implementation. - if the function is going to fail, don't touch the setting. Previously, the functions would first clear the certificate before trying to validate the input. It's more logical, that if a functions is going to fail to check for failure first and don't modify the settings. - not every blob can be represented. For example, if we have a blob which starts with "file://", then there is no way to set it, simply because we don't support a prefix for blobs (like "data:;base64,"). This means, if we try to set the certificate to a particular binary, we must check that the binary is interpreted with the expected scheme. Add this check.
| * libnm/802-1x: cleanup NMSetting8021x:verify()Thomas Haller2018-09-041-25/+62
| |
| * libnm/802-1x: don't verify certificates in GObject property setterThomas Haller2018-09-041-49/+6
| | | | | | | | | | | | | | | | | | | | | | | | First of all, g_warning() is not a suitable error handling. In particular, note how this code is reached when obtaining a setting from D-Bus, that is, the user is not at fault. The proper way to handle this, is allowing the setter to set the invalid value. Only later, during verify() we will fail. This way, NetworkManager can extend the format and older libnm clients don't break. This is how forward-compatibility (with older libnm vs. newer daemon) is supposed to work.
| * libnm/802-1x: refactor certificate handling in settingsThomas Haller2018-09-041-190/+94
| | | | | | | | | | | | | | - all this code duplication. Add functions and macros to simplify the implementation of certificate properties. Overall, pretty trival. Replace code with a macro.
| * libnm/802-1x: call g_bytes_unref() directly without checking for NULLThomas Haller2018-09-041-29/+14
| | | | | | | | Since its inception, g_bytes_unref() is fine with NULL argument.
| * libnm/802-1x/trivial: reorder codeThomas Haller2018-09-041-505/+514
| |
| * libnm/802-1x: refactor GObject properties of NMSetting8021xThomas Haller2018-09-041-323/+280
| |
| * libnm/crypto: mark nm_crypto_make_des_aes_key() as test-only functionThomas Haller2018-09-042-15/+15
| |
| * libnm/crypto: clean crypto implementations for gnutls/nssThomas Haller2018-09-042-181/+171
| | | | | | | | | | | | - refactor to use cleanup attribute and return-early - reorder some code
| * libnm/crypto: rework endianness detection for crypto_verify_pkcs12()Thomas Haller2018-09-044-31/+16
| | | | | | | | | | | | | | | | At other places, we already use __BYTE_ORDER define to detect endianness. We don't need multiple mechanisms. Also note that meson did not do the correct thing as AC_C_BIGENDIAN, so meson + nss + big-endian was possibly broken.
| * libnm/crypto: refactor to use enum for supported ciphersThomas Haller2018-09-045-134/+190
| | | | | | | | | | | | | | | | | | We need to (and already did) define our own identifier for ciphers, because the gnutls/nss identifiers must be abstracted. Don't use a string for that. The number of supported ciphers is not generic but fixed and known at compiler time. An enum is better suited.
| * libnm/crypto: remove unused argument key_type for decrypt functionsThomas Haller2018-09-044-6/+0
| |
| * libnm/crypto: don't initialize buffer for nm_crypto_make_des_aes_key() with zeroThomas Haller2018-09-041-1/+1
| | | | | | | | | | | | | | | | @key is directly passed to nm_crypto_md5_hash(), which cannot (by API design) fail. No need to initialize it. Also, no need to allocate an additional trailing NUL byte. The key is binary, every attempt to use it as a string will horribly fail.
View Lorry Controller Status