can you reproduce this every time? Could you provide debug logfiles?

Do would that by editing /etc/NetworkManager/NetworkManager.conf

[logging]
level=DEBUG
domains=ALL


Thanks

pushed danw/nm-utils-init-bgo740893 for review

(though the fix in the 0.9.10 timeframe is just to have src/main.c call nm_utils_init().)

dcbw had said something about constructors not being run unless they come from shared libraries, but this doesn't seem to be true. _nm_utils_init() does automatically get called from /sbin/NetworkManager with this patch.

Looks very good. I pushed 2 minor fixup commits on your branch.


I pushed two other commits:
  libnm-core: combine duplicate crypto_make_des_aes_key() function
  libnm-core: relax restrictions on input arguments for crypto_md5_hash()

Squashed and repushed.

I also pushed danw/nm-utils-init-0910-bgo740893 with the fix for 0.9.10.

Waiting to see if dcbw has any comments on the constructor stuff...

you bothered backporting "libnm: drop nm_utils_deinit()" but did not do the same for master:libnm-utils.

Rest LGTM

(could we merge this after th/uuid-variant3-bgo740865, because that branch uses crypto_md5_hash() based on the assumption that it cannot pass empty strings. With "libnm-core: relax restrictions on input arguments for crypto_md5_hash()" I would modify nm_utils_generate_from_string(), but I don't want to change the other branch)

> libnm-core: combine duplicate crypto_make_des_aes_key() function

nm_utils_rsa_key_encrypt_aes() isn't used anywhere, though it was added in https://bugzilla.gnome.org/show_bug.cgi?id=729381 for completeness and for testcases.  I'm not sure we should keep it public API in libnm though.

nm_utils_rsa_key_encrypt() is only used in the applet's GConf converter.  I don't think we can kill the converter just yet, due to Debian stable and RHEL6 for upgrade reasons.

But I think we should:

a) Remove nm_utils_rsa_key_encrypt(), and nm_utils_rsa_key_encrypt_aes() from libnm-core as public API.  The applet GConf converter can copy nm_utils_rsa_key_encrypt() since it's the only user, whenever it gets ported to libnm.  nm_utils_rsa_key_encrypt_helper() then dies as well.

b) Now we can also get rid of crypto_randomize() completely (because its only caller is nm_utils_rsa_key_encrypt_helper), and callers that want it can just use GRand instead.

c) now we can make crypto_make_des_aes_key() static because it is no longer needed in nm-utils.c. Maybe even fold it back into decrypt_key().

(In reply to comment #6)
> nm_utils_rsa_key_encrypt() is only used in the applet's GConf converter.

It's also used by ifnet/connection_parser.c.

> b) Now we can also get rid of crypto_randomize() completely (because its only
> caller is nm_utils_rsa_key_encrypt_helper), and callers that want it can just
> use GRand instead.

GRand is not cryptographically strong, so should not be used for that. At any rate, since we can't get rid of nm_utils_rsa_key_encrypt() from the private API, we can't get rid of crypto_randomize() either.

> The applet GConf converter can copy
> nm_utils_rsa_key_encrypt() since it's the only user, whenever it gets
> ported to libnm.

Or, more simply, we could just not bother ever porting the GConf converter to libnm (and then we don't need to worry about where it will get its random bytes from).


pushed an updated branch with a forward-port of "libnm-util: Note that nm_utils_deinit() is a no-op", and a new commit removing nm_utils_rsa_key_encrypt() and nm_utils_rsa_key_encrypt_aes() from the public API. (If, OTOH, we do decide to keep them, we should at least merge them into a single function in libnm.)

(In reply to comment #7)
> (In reply to comment #6)
> > nm_utils_rsa_key_encrypt() is only used in the applet's GConf converter.
> 
> It's also used by ifnet/connection_parser.c.

Ok, true, for the BLOB case.  We removed that capability from ifcfg-rh I guess, but there are likely legacy connections around that use it.

But we can certainly remove it from libnm.ver/public API at least, since no consumers outside of NM itself should really be using it.

> > b) Now we can also get rid of crypto_randomize() completely (because its only
> > caller is nm_utils_rsa_key_encrypt_helper), and callers that want it can just
> > use GRand instead.
> 
> GRand is not cryptographically strong, so should not be used for that. At any
> rate, since we can't get rid of nm_utils_rsa_key_encrypt() from the private
> API, we can't get rid of crypto_randomize() either.

At least crypto_randomize() is internal API.

> > The applet GConf converter can copy
> > nm_utils_rsa_key_encrypt() since it's the only user, whenever it gets
> > ported to libnm.
> 
> Or, more simply, we could just not bother ever porting the GConf converter to
> libnm (and then we don't need to worry about where it will get its random bytes
> from).

True.

> pushed an updated branch with a forward-port of "libnm-util: Note that
> nm_utils_deinit() is a no-op", and a new commit removing
> nm_utils_rsa_key_encrypt() and nm_utils_rsa_key_encrypt_aes() from the public
> API.

I would like to just drop them.

> libnm-core: relax restrictions on input arguments for crypto_md5_hash()

Shouldn't crypto_md5_hash() at least right before "while (nkey > 0) {" do:

	g_return_if_fail (password && password_len);

because there's no point to calling this function without a password, and even if one is given, we should ensure that it is not "\0" for the password_len=-1 case.  Nothing will ever call it with a salt, but no password, because that's contrary to the purpose of the function.

(if this is done, then the 'ctx = g_checksum_new()" needs to move down as well to prevent a leak).

The rest looks good to me.

>> libnm-core: drop nm_utils_rsa_key_encrypt(), _encrypt_aes()

I would not drop the @cipher argument. It doesn't hardly simplify the code. Actually, it makes it more obvious what effects different ciphers have (for example, we might choose a different salt length, which is not-obvious).


(In reply to comment #9)
> > libnm-core: relax restrictions on input arguments for crypto_md5_hash()
> 
> Shouldn't crypto_md5_hash() at least right before "while (nkey > 0) {" do:
> 
>     g_return_if_fail (password && password_len);
> 
> because there's no point to calling this function without a password, and even
> if one is given, we should ensure that it is not "\0" for the password_len=-1
> case.  Nothing will ever call it with a salt, but no password, because that's
> contrary to the purpose of the function.
> 
> (if this is done, then the 'ctx = g_checksum_new()" needs to move down as well
> to prevent a leak).
> 
> The rest looks good to me.

As its name indicates, the purpose of the function is to compute an md5 hash. It should just operate happily on the valid input range for md5, including "".

This artificial limitation of the valid input range is unexpected (to me). It would require a code comment. Unit tests have to skip testing the obvious corner case "".
This limitation also propagates to nm_utils_uuid_generate_from_string("").

Any minimal password complexity must be enforced by the caller. And at that point the caller should also reject one-letter words, and so on.

(In reply to comment #10)
> >> libnm-core: drop nm_utils_rsa_key_encrypt(), _encrypt_aes()
> 
> I would not drop the @cipher argument.

@cipher was only an argument to the internal helper method used by the public APIs. Since the AES API is going away, there's no need to keep the helper separate from nm_utils_rsa_key_encrypt() any more, and merging it back in implies inlining the @cipher.


Agreed on allowing empty password.


pushed as ddcd6dda555697395cdd45adc3478df9f7cd5c88