file_to_secure_bytes() tried to load the file from disk and ensure that
the data will be cleared. It did so poorely, because g_file_get_contents()
cannot be used for that.
Add a helper function nm_crypto_read_file() to get this right.
g_file_get_contents() may use re-alloc to load the file. Each time
it re-allocated the buffer, it does not bother clearing the loaded
buffer from memory.
Alternatively, g_file_get_contents() may use stat() and only allocate
one buffer. But also in this mode, without realloc(), it does not
clear the buffer if reading the file fails with IO error later.
Use nm_utils_file_get_contents() which does that.
While at it, don't load files larger that 100 MB.
It's only used for testing, so this change is not very relevant.
Anyway, I think our crypto code should succeed in not leaving
key material in memory. Refactor the code to do that, though,
how the pem file gets composed is quite a hack (for tests good
enough though).
nm_utils_rsa_key_encrypt() is internal API which is only uesd for testing.
Move it to nm-crypto.h (where it fits better) and rename it to make the
testing-aspect obvious.
In nm-crypto.c we have functions that are only called from tests.
Maybe these functions should move away from libnm-core to the
test.
Leave it, but at least rename them to make it clear that these
functions are not relevant for libnm's actual usage. For a
reviewer that makes a big difference as crypto functions in libnm
have a significantly higher requirement for quality.
There is nothing new here. We already have other *nmtst* functions
beside our regular code. The concention is, that functions that
are only for testing are named explicitly ("nmtst"), and that they
can only be called by test functions themselves.
The GBytes has a suitable cleanup function, which zeros the certificate
from memory.
Also, all callers that require the certificate, actually later converted
it into a GBytes anyway. This way, they can re-used the same instance
(avoiding an additionaly copying of the data), and they will properly
clear the memory when freed.
Rename _nm_crypto_verify_cert() to _nm_crypto_verify_x509().
Also, don't let it return a NMCryptoFileFormat result. This
function only checks for a particular format, hence it
should only return true/false.
Also, fix setting error output argument when the function fails.
We already do matrix-builds with autotools|meson and gcc|clang.
Make the selected crypto backend depending on the compiler, so
that we get more coverage.
If the library is available, let's at least compile both
crypto backends.
That is helpful when developing on crypto backends, so that
one does not have to configure the build twice.
With autotools, the build is only run during `make check`.
Not for meson, but that is generally the case with our meson
setup, that it also builds tests during the regular build step.
Follow our convention, that items in headers are all named with
an "NM" prefix.
Also, "nm-crypto-impl.h" contains internal functions that are to be implemented
by the corresponding crypto backends. Distinguish their names as well.
There are two aspects: the public crypto API that is provided by
"nm-crypto.h" header, and the internal header which crypto backends
need to implement. Split them.
Data that we load from crypto files should be cleared once it's
no longer used.
Just a small step. There are many other places where we copy the data
and leave it around.
crypto_make_des_aes_key() asserts that iv-lenght is at least
8 characters. Whatever the reason. That means, decrypt_key()
must check for that condition first, and gracefully fail.
Also, don't use strtol() to convert a pair of hex digits to
integer.
Also, don't keep the IV in memory. Yes, it's not very critical,
but this is crypto code, we should not leave data behind.
There should be a clear distinction between whether an array
is a NUL terminated string or binary with a length.
crypto_md5_hash() is already complicated enough. Adjust it's
API to only support binary arguments, and thus have "guint8 *" type.
the comment and code made it sound like parse_old_openssl_key_file() would
set @key_type if the parsing was only done partially. That is not the case,
@key_type is only set, if parsing was successful. Adjust the code.
While at it, don't require the caller to initialize @out_key_type. It's
just an enum, if we care to always set it, just do it.
PROP_0 is how we commonly name this property when we don't use
NM_GOBJECT_PROPERTIES_DEFINE(). Rename it.
Also, allow to skip PROP_0 in nm_gobject_notify_together(), that
is handy to optionally invoke a notification, like
nm_gobject_notify_together (obj,
PROP_SOMETHING,
changed ? PROP_OTHER : PROP_0);
When reading a file, we may allocate intermediate buffers (realloc()).
Also, reading might fail halfway through the process.
Add a new flag that makes sure that this memory is cleared. The
point is when reading secrets, that we don't accidentally leave
private sensitive material in memory.
We already had nm_free_secret() to clear the secret out
of a NUL terminated string. That works well for secrets
which are strings, it can be used with a cleanup attribute
(nm_auto_free_secret) and as a cleanup function for a
GBytes.
However, it does not work for secrets which are binary.
For those, we must also track the length of the allocated
data and clear it.
Add two new structs NMSecretPtr and NMSecretBuf to help
with that.
Internally, GByteArray is actually a GArray, so it would be safe to
use "gs_unref_array" macro. However, that is rather ugly, and means
to rely on an internal implementation detail of GByteArray.
Instead, add a cleanup macro for GByteArray.
I think GSList is not a great data type. Most of the time when we used
it, we better had choosen another data type.
These utility functions were unused, and I think we should use GSList
less.
Drop them.
NMConnection is an interface, which is implemented by the types
NMSimpleConnection (libnm-core), NMSettingsConnection (src) and
NMRemoteConnection (libnm).
NMSettingsConnection does a lot of things already:
1) it "is-a" NMDBusObject and exports the API of a connection profile
on D-Bus
2) it interacts with NMSettings and contains functionality
for tracking the profiles.
3) it is the base-class of types like NMSKeyfileConnection and
NMIfcfgConnection. These handle how the profile is persisted
on disk.
4) it implements NMConnection interface, to itself track the
settings of the profile.
3) and 4) would be better implemented via delegation than inheritance.
Address 4) and don't let NMSettingsConnection implemente the NMConnection
interface. Instead, a settings-connection references now a NMSimpleConnection
instance, to which it delegates for keeping the actual profiles.
Advantages:
- by delegating, there is a clearer separation of what
NMSettingsConnection does. For example, in C we often required
casts from NMSettingsConnection to NMConnection. NMConnection
is a very trivial object with very little logic. When we have
a NMConnection instance at hand, it's good to know that it is
*only* that simple instead of also being an entire
NMSettingsConnection instance.
The main purpose of this patch is to simplify the code by separating
the NMConnection from the NMSettingsConnection. We should generally
be aware whether we handle a NMSettingsConnection or a trivial
NMConnection instance. Now, because NMSettingsConnection no longer
"is-a" NMConnection, this distinction is apparent.
- NMConnection is implemented as an interface and we create
NMSimpleConnection instances whenever we need a real instance.
In GLib, interfaces have a performance overhead, that we needlessly
pay all the time. With this change, we no longer require
NMConnection to be an interface. Thus, in the future we could compile
a version of libnm-core for the daemon, where NMConnection is not an
interface but a GObject implementation akin to NMSimpleConnection.
- In the previous implementation, we cannot treat NMConnection immutable
and copy-on-write.
For example, when NMDevice needs a snapshot of the activated
profile as applied-connection, all it can do is clone the entire
NMSettingsConnection as a NMSimpleConnection.
Likewise, when we get a NMConnection instance and want to keep
a reference to it, we cannot do that, because we never know
who also references and modifies the instance.
By separating NMSettingsConnection we could in the future have
NMConnection immutable and copy-on-write, to avoid all unnecessary
clones.
The limit of trying up to 10000 was arbitrary. In practice, we are not expected
that we need that many searches. If that would be the case (and we would have
10000 conflicting connections that take all the names), then we anyway would
need to refactor the code not to scale with O(n^2).
Replace the arbitrary limit with an even larger one. The new limit is so
large that in practice it's impossible to reach it.
Add a helper function nm_device_parent_find_for_connection() to
unify implementations of setting the parent in update_connection().
There is some change in behavior, in particular for nm-device-vlan.c,
which no longer compares the link information from platform. But
update_connection() is anyway a questionable concept, only used
for external assumed connection (which itself, is questionable). Meaning,
update_connection() is a hack not science, and it's not at all clear
what the correct behavior is.
Also, note how vlan's implementation differs from all others. Why?
Should we always resort to also check the information from platform?
Either way, one of the two approaches should be used consistently and
nm_device_parent_find_for_connection() opts to not consult platform
cache.
Don't call the 'if{up,down}-routes' scripts because in next Fedora
versions network scripts will be deprecated and will not be present in
the default installation.
Instead, just copy and adapt the code from those scripts.
https://bugzilla.redhat.com/show_bug.cgi?id=1618419
suspend-resume must be selectable, out of for possible options.
We can do auto-detection based on present libraries, but it
shall be selectable. Like it is with autotools.
