There's one issue, though. Keyfile is not a well settled standard and it's been changed repeatedly. It would be good to define it carefully before it gets part of the public API. Or is there a notion of an experimental API or something like that?

(In reply to Pavel Simerda from comment #1)
> There's one issue, though. Keyfile is not a well settled standard and it's
> been changed repeatedly. It would be good to define it carefully before it
> gets part of the public API. Or is there a notion of an experimental API or
> something like that?

what you mean with standard? It's only known to NetworkManager. No other application uses it (natively).

The implementation is the definition ;-) and there is `man nm-settings-keyfile`

We value backward compatibility very highly, a newer version will always be able to read keyfiles written by older versions.
The user should treat the generated keyfile as an opaque string (but human readable). Mangling the generated string will work for most practical purposes, but the way that is guaranteed to work is:
  c = NMConnection::from_keyfile(str)
  c['connection']['autoconnect'] = false
  str = c.to_keyfile()


If somebody sees the need to define it better, let's do that too (but what is wrong with `man nm-settings-keyfile`?).

there is no guarantee that an old reader can parse a connection written by a newer writer. But that is not avoidable, and there is a pretty good chance that it might work.

An alternative implementation would be to change the settings plugin interface so that plugins only need to depend on libnm(-core), not libNetworkManager, and then nmcli could just dlopen the plugins (and so then it could read/write ifcfg-rh files too, which is an alternate approach for part of bug 744700).

I don't think this would be that hard; the biggest problem is that the plugin connection types currently need to inherit from NMSettingsConnection. But it might be possible to just have the plugins export NMSimpleConnections, and NMSettings would take those connections and clone them into NMSettingsConnections itself.

(In reply to Dan Winship from comment #4)
> An alternative implementation would be to change the settings plugin
> interface so that plugins only need to depend on libnm(-core), not
> libNetworkManager, and then nmcli could just dlopen the plugins (and so then
> it could read/write ifcfg-rh files too, which is an alternate approach for
> part of bug 744700).

downside of that is, that the functionality is not readily available to libnm users, e.g. using gi/python. Every user has to workaround it (dlopen).

Also, there is no actual keyfile shared library. Keyfile plugin is statically linked.


> I don't think this would be that hard; the biggest problem is that the
> plugin connection types currently need to inherit from NMSettingsConnection.
> But it might be possible to just have the plugins export
> NMSimpleConnections, and NMSettings would take those connections and clone
> them into NMSettingsConnections itself.

we only need nm_keyfile_plugin_write_connection() and nm_keyfile_plugin_connection_from_file(). that code is already in reader.c, writer.c, and utils.c and independent of NMSettingsConnection/NMKeyfileConnection.

looking at it now, it seems very simple to do (which is great).

(In reply to Thomas Haller from comment #5)
> downside of that is, that the functionality is not readily available to
> libnm users, e.g. using gi/python. Every user has to workaround it (dlopen).

I was thinking we only needed it in nmcli. But we could put the code into libnm just as easily.

> Also, there is no actual keyfile shared library. Keyfile plugin is
> statically linked.

Yeah, but that wouldn't be hard to change

work in progress, at branch th/libnm-keyfile-bgo744699 . Not yet ready for review.

(In reply to Thomas Haller from comment #7)
> work in progress, at branch th/libnm-keyfile-bgo744699 . Not yet ready for
> review.

have a look at th/libnm-keyfile-bgo744699

the new functions nm_keyfile_read() and nm_keyfile_write() are intended to be powerful and extendible. Hence, it seems hard to use from language bindings.


I think, for bindings, we should expose two simple wrappers as public API, where the read handler does nothing, and the write handler returns all the certificates to the caller (as GVariant?)

(In reply to Thomas Haller from comment #8)
> (In reply to Thomas Haller from comment #7)

> I think, for bindings, we should expose two simple wrappers as public API,
> where the read handler does nothing, and the write handler returns all the
> certificates to the caller (as GVariant?)

added a commit so that keyfile writer in libnm embeds certificate blobs into the keyfile (as blob:// scheme). That way, the writer can serialize the entire connection (including blobs).


Missing: reader and writer should add a possibility to hide passwords. E.g. via a type "secret" to the caller(?).

> libnm: ensure zero terminated path for blob in 802.1x certificates

>    is not NULL terminated. It is not that grave, because verify_cert()

>+		/* check if the blob is NULL terminated. */

nitpick: "NULL" is a pointer. '\0' is "NUL" with only one "L" (which comes from ASCII, which uses 2 or 3 letter names for all the control characters).

>+		for (i = 0; i < length; i++) {
>+			if (data[i] == '\0')
>+				return NM_SETTING_802_1X_CK_SCHEME_PATH;

There shouldn't be any 0 bytes before the end. So I'd say

    if (memchr (data, '\0', length) == data + length)
        return NM_SETTING_802_1X_CK_SCHEME_PATH;



> libnm: add keyfile support to libnm-core

Rather than having a ReadInfo/WriteInfo struct passed as the last arg to every function, it would be more idiomatic to have an "NMKeyfileReader"/"NMKeyfileWriter" as the first arg (replacing the GKeyFile in many cases).

The "foo" vs "foo1" distinction needs better naming.

Without seeing how this is going to be used from clients, it's hard to say whether the API is right or if it's too generic.

>+#define NM_KEYFILE_READ_TYPE_WARN "warn"

Why is this a string? If it was an enum then you could just compare against it correctly. As it is now, the keyfile plugin code needs to do a strcmp.



> keyfile: support writing certificates as blob inside the keyfile

You should use the "data" URI type here rather than inventing a "blob" type. Just "data:;base64," and then the base64-encoded blob.

(In reply to Dan Winship from comment #10)
> > libnm: ensure zero terminated path for blob in 802.1x certificates
> 
> >    is not NULL terminated. It is not that grave, because verify_cert()
> 
> >+		/* check if the blob is NULL terminated. */

fixed

> nitpick: "NULL" is a pointer. '\0' is "NUL" with only one "L" (which comes
> from ASCII, which uses 2 or 3 letter names for all the control characters).
> 
> >+		for (i = 0; i < length; i++) {
> >+			if (data[i] == '\0')
> >+				return NM_SETTING_802_1X_CK_SCHEME_PATH;
> 
> There shouldn't be any 0 bytes before the end. So I'd say
> 
>     if (memchr (data, '\0', length) == data + length)
>         return NM_SETTING_802_1X_CK_SCHEME_PATH;

fixed


> > libnm: add keyfile support to libnm-core
> 
> Rather than having a ReadInfo/WriteInfo struct passed as the last arg to
> every function, it would be more idiomatic to have an
> "NMKeyfileReader"/"NMKeyfileWriter" as the first arg (replacing the GKeyFile
> in many cases).
> 
> The "foo" vs "foo1" distinction needs better naming.

fixed. Good now?


> Without seeing how this is going to be used from clients, it's hard to say
> whether the API is right or if it's too generic.



> >+#define NM_KEYFILE_READ_TYPE_WARN "warn"
> 
> Why is this a string? If it was an enum then you could just compare against
> it correctly. As it is now, the keyfile plugin code needs to do a strcmp.

changed to enum.


> > keyfile: support writing certificates as blob inside the keyfile
> 
> You should use the "data" URI type here rather than inventing a "blob" type.
> Just "data:;base64," and then the base64-encoded blob.

fixed.



Thanks, repushed.

> libnm: ensure zero terminated path for blob in 802.1x certificates

Won't this break compatibility with existing clients that may send paths without termination?

(In reply to Dan Williams from comment #12)
> > libnm: ensure zero terminated path for blob in 802.1x certificates
> 
> Won't this break compatibility with existing clients that may send paths
> without termination?

I think not. Note that verify_cert() checks for the file:// scheme and then enforces that the binary blob is NUL terminated (and UTF-8).

Even if some clients would sent non-NUL-terminated, non-UTF-8 paths, NM core would already now reject them as invalid.


Also, without this change there is no guarantee that the following doesn't crash:

      if (nm_setting_802_1x_get_ca_cert_scheme (s_8021x) == NM_SETTING_802_1X_CK_SCHEME_PATH)
          g_print ("path: %s", nm_setting_802_1x_get_ca_cert_path (s_8021x))


nm_setting_802_1x_get_ca_cert_path() might return an unterminated string.

regarding

>> keyfile: fix potential crash in keyfile reader get_bytes()

I think this change is correct, but I am a bit surprised that I never saw a crash due to this... maybe I missed something.

(In reply to Thomas Haller from comment #14)
> regarding
> 
> >> keyfile: fix potential crash in keyfile reader get_bytes()
> 
> I think this change is correct, but I am a bit surprised that I never saw a
> crash due to this... maybe I missed something.

Did you remove that commit from the branch? I don't see it there any more.

It looks like we currently don't crash because g_key_file_get_integer_list() sets *length to 0 on error. But we shouldn't rely on that.

> libnm: add keyfile support to libnm-core

>+typedef struct {
>+	KEYFILE_READER_INFO_MEMBERS;
>+	const char *group;
>+	NMSetting *setting;
>+} KeyfileReaderInfo;

You don't need two different struct types. The code only ever parses one group at a time, so just put all the fields into one struct, and have read_setting() set the group and setting fields appropriately on entry, and set them to NULL on exit.

(This and most of the other reader comments apply to the writer as well.)

>+_fmt_warn (const char *group, NMSetting *setting, const char *property_name, char *message)

as before, it's hard to say without seeing the client-side usage, but given that you already pass the group and property names to the handler, it might make more sense to just pass the raw message to the handler, and let the handler concatenate the property name to it as it sees fit.

>+#define handle_warn_base(arg_info_base, arg_group, arg_setting, arg_property_name, arg_severity, ...) \

>+#define handle_warn(arg_info, arg_property_name, arg_severity, ...) \

Why are these are macros rather than functions? Also, something like "keyfile_reader_warn()" would be a better name.

>+		             _("unexepected character '%c' for %s: '%s' (position %td)"),

typo "unexpected" (in three places)

>+ * For 'warn' type, the default action is doing nothing.

docs should be updated to refer to the enum

>+                                          GError **error,
>+                                          void *user_data);

hm... I would say that "GError always comes last" trumps "user_data always comes last" :-)

>+} NMKeyfileWarnSeverity;

Could we just use GLogLevelFlags?

>-	const char *privkey_pw_prop;

Should be a separate commit?



> libnm: add define for cert scheme prefix file:// for NMSetting8021x

This adds the blob scheme too. Which is weird since NMSetting8021x itself doesn't know anything about that...

(In reply to Thomas Haller from comment #8)
> I think, for bindings, we should expose two simple wrappers as public API,
> where the read handler does nothing, and the write handler returns all the
> certificates to the caller (as GVariant?)

You could get rid of read handlers entirely, and just have the reader return an array of notifications, where each notification contains level, group, key, and message. The keyfile plugin would iterate over that and nm_log each notification.

On the write side, I think the simple API should just always use the data: scheme for writing certificates. But you still need the more complicated version to let the plugin write backward-compatible key files...

Why are we trying to write stuff out as blob/data now?  We tried really hard to get away from that in the past, because there are so many certificate formats and parsing is a huge PITA.  I'd rather we didn't have a blob/data scheme at all, the only reason it's there is for backwards compatibility.  Honestly I don't think the keyfile writer should *ever* write data out as a blob, or at least we should never do that in the NM core at all.

If we don't want to use file paths somewhere, then we should be using the pkcs#11 URI to point to it instead of passing some arbitrary blob that may be a PCKS#8 file, PKCS#1 file, raw DER, PEM-encoded, PKCS#12 blob, whatever.  I'd rather have NM get out of the business of touching certificates/keys at all, even in binary form where it's just passthrough...

(In reply to Dan Winship from comment #15)
> (In reply to Thomas Haller from comment #14)
> > regarding
> > 
> > >> keyfile: fix potential crash in keyfile reader get_bytes()
> > 
> > I think this change is correct, but I am a bit surprised that I never saw a
> > crash due to this... maybe I missed something.
> 
> Did you remove that commit from the branch? I don't see it there any more.
> 
> It looks like we currently don't crash because g_key_file_get_integer_list()
> sets *length to 0 on error. But we shouldn't rely on that.

yeah, first I thought there is a problem there. Then I noticed about *length=0 
and I dropped the patch. Now I brought it back as
  >> keyfile: handle invalid integer list in keyfile reader get_bytes()


(In reply to Dan Winship from comment #16)
> > libnm: add keyfile support to libnm-core
> 
> >+typedef struct {
> >+	KEYFILE_READER_INFO_MEMBERS;
> >+	const char *group;
> >+	NMSetting *setting;
> >+} KeyfileReaderInfo;
> 
> You don't need two different struct types. The code only ever parses one
> group at a time, so just put all the fields into one struct, and have
> read_setting() set the group and setting fields appropriately on entry, and
> set them to NULL on exit.
> 
> (This and most of the other reader comments apply to the writer as well.)

done


> >+_fmt_warn (const char *group, NMSetting *setting, const char *property_name, char *message)
> 
> as before, it's hard to say without seeing the client-side usage, but given
> that you already pass the group and property names to the handler, it might
> make more sense to just pass the raw message to the handler, and let the
> handler concatenate the property name to it as it sees fit.

Agree. moved _fmt_warn() to keyfile plugin writer.


> >+#define handle_warn_base(arg_info_base, arg_group, arg_setting, arg_property_name, arg_severity, ...) \
> 
> >+#define handle_warn(arg_info, arg_property_name, arg_severity, ...) \
> 
> Why are these are macros rather than functions? Also, something like
> "keyfile_reader_warn()" would be a better name.

Because I only want to construct the message (g_strdup_printf (__VA_ARGS__)) if we actually have a handler. I moved part of the macro to a function.

I like the name handle_warn() because it means calling the "handler" callback to report a warning...


> >+		             _("unexepected character '%c' for %s: '%s' (position %td)"),
> 
> typo "unexpected" (in three places)

fixed


> >+ * For 'warn' type, the default action is doing nothing.
> 
> docs should be updated to refer to the enum

fixed


> >+                                          GError **error,
> >+                                          void *user_data);
> 
> hm... I would say that "GError always comes last" trumps "user_data always
> comes last" :-)

done


> >+} NMKeyfileWarnSeverity;
> 
> Could we just use GLogLevelFlags?

I think, our own custom enum gives us more flexibility. I am still undecided, whether nm_keyfile_read() should become public API (or if we just expose a simplified wrapper).
I tend to yes, and then I think our own enum gives us more flexibility to extend the functionality later on.

For example, the last commit defines an warning level "NM_KEYFILE_WARN_SEVERITY_INFO_MISSING_FILE" for warning when a file is missing. I could imagine that nmcli might want to swallow this warning.
I don't know yet, but our own enum gives more flexibility later.


> >-	const char *privkey_pw_prop;
> 
> Should be a separate commit?

done.


> > libnm: add define for cert scheme prefix file:// for NMSetting8021x
> 
> This adds the blob scheme too. Which is weird since NMSetting8021x itself
> doesn't know anything about that...

you are right. Now there is:

nm-setting-8021x.h:
#define NM_SETTING_802_1X_CERT_SCHEME_PREFIX_PATH "file://"

nm-keyfile.internal.h:
#define NM_KEYFILE_CERT_SCHEME_PREFIX_BLOB "data:;base64,"
#define NM_KEYFILE_CERT_SCHEME_PREFIX_PATH "file://"

(In reply to Dan Winship from comment #17)
> (In reply to Thomas Haller from comment #8)
> > I think, for bindings, we should expose two simple wrappers as public API,
> > where the read handler does nothing, and the write handler returns all the
> > certificates to the caller (as GVariant?)
> 
> You could get rid of read handlers entirely, and just have the reader return
> an array of notifications, where each notification contains level, group,
> key, and message. The keyfile plugin would iterate over that and nm_log each
> notification.
> 
> On the write side, I think the simple API should just always use the data:
> scheme for writing certificates. But you still need the more complicated
> version to let the plugin write backward-compatible key files...

I like the handlers for it's flexibility. They allow us to add hooks to extend writing/reading keyfiles whatever comes up.

Only downside is, that these functions are not well consumable via bindings (due to the semi-opaque @type_data argument). Maybe for bindings, we could expose a dumped down version (which also is less powerful and less extendible in the future). I can add simplified wrappers *in addition* to the complex ones with handlers.

I'm not yet fully decided on how the public API should look like, but I tend to go with nm-keyfile-internal.h as is -- at least from "C" it's well consumable.

(In reply to Dan Williams from comment #18)
> Why are we trying to write stuff out as blob/data now?  We tried really hard
> to get away from that in the past, because there are so many certificate
> formats and parsing is a huge PITA.  I'd rather we didn't have a blob/data
> scheme at all, the only reason it's there is for backwards compatibility. 
> Honestly I don't think the keyfile writer should *ever* write data out as a
> blob, or at least we should never do that in the NM core at all.

Keyfile writer does still not ever write keyfiles with blobs. Nothing changed there. It's not encouraging the use of blobs.

The only reason I add it now is backward compatibility.

> If we don't want to use file paths somewhere, then we should be using the
> pkcs#11 URI to point to it instead of passing some arbitrary blob that may
> be a PCKS#8 file, PKCS#1 file, raw DER, PEM-encoded, PKCS#12 blob, whatever.
> I'd rather have NM get out of the business of touching certificates/keys at
> all, even in binary form where it's just passthrough...

One goal is nmcli being able to import/export connections in keyfile format. Keyfile being "the" format to represent NM connections.
For that keyfile must be able to express every valid connection (address labels are missing too, btw).

If a connection contains (legacy) blobs, we still need a way to express them. If you type `nmcli connection export legacy-connection`, somehow these blobs must be represented. It would be very cumbersome if nmcli would write them to some files (mktemp??).
With this change, the default bahavior of nm_keyfile_write() in face of legacy blobs is to embed them in  keyfile as "data:;base64,". Which nm_keyfile_read() can read back to make a full round-trip.

keyfile plugin writer, which intercepts the default behavior of nm_keyfile_write(), still converts all blobs to files.

(In reply to Thomas Haller from comment #20)
> (In reply to Dan Winship from comment #17)
> > (In reply to Thomas Haller from comment #8)
> > > I think, for bindings, we should expose two simple wrappers as public API,
> > > where the read handler does nothing, and the write handler returns all the
> > > certificates to the caller (as GVariant?)
> > 
> > You could get rid of read handlers entirely, and just have the reader return
> > an array of notifications, where each notification contains level, group,
> > key, and message. The keyfile plugin would iterate over that and nm_log each
> > notification.
> > 
> > On the write side, I think the simple API should just always use the data:
> > scheme for writing certificates. But you still need the more complicated
> > version to let the plugin write backward-compatible key files...
> 
> I like the handlers for it's flexibility. They allow us to add hooks to
> extend writing/reading keyfiles whatever comes up.
> 
> Only downside is, that these functions are not well consumable via bindings
> (due to the semi-opaque @type_data argument). Maybe for bindings, we could
> expose a dumped down version (which also is less powerful and less
> extendible in the future). I can add simplified wrappers *in addition* to
> the complex ones with handlers.
> 
> I'm not yet fully decided on how the public API should look like, but I tend
> to go with nm-keyfile-internal.h as is -- at least from "C" it's well
> consumable.

One idea is that `nmcli con export` should hide passwords by default (or certificate blobs which may contain private keys).

Easily extendable by adding hocks to the write handler.

(In reply to Thomas Haller from comment #21)
> (In reply to Dan Williams from comment #18)
> > Why are we trying to write stuff out as blob/data now?  We tried really hard
> > to get away from that in the past, because there are so many certificate
> > formats and parsing is a huge PITA.  I'd rather we didn't have a blob/data
> > scheme at all, the only reason it's there is for backwards compatibility. 
> > Honestly I don't think the keyfile writer should *ever* write data out as a
> > blob, or at least we should never do that in the NM core at all.
> 
> Keyfile writer does still not ever write keyfiles with blobs. Nothing
> changed there. It's not encouraging the use of blobs.
> 
> The only reason I add it now is backward compatibility.
> 
> > If we don't want to use file paths somewhere, then we should be using the
> > pkcs#11 URI to point to it instead of passing some arbitrary blob that may
> > be a PCKS#8 file, PKCS#1 file, raw DER, PEM-encoded, PKCS#12 blob, whatever.
> > I'd rather have NM get out of the business of touching certificates/keys at
> > all, even in binary form where it's just passthrough...
> 
> One goal is nmcli being able to import/export connections in keyfile format.
> Keyfile being "the" format to represent NM connections.
> For that keyfile must be able to express every valid connection (address
> labels are missing too, btw).
> 
> If a connection contains (legacy) blobs, we still need a way to express
> them. If you type `nmcli connection export legacy-connection`, somehow these
> blobs must be represented. It would be very cumbersome if nmcli would write
> them to some files (mktemp??).
> With this change, the default bahavior of nm_keyfile_write() in face of
> legacy blobs is to embed them in  keyfile as "data:;base64,". Which
> nm_keyfile_read() can read back to make a full round-trip.

I'd almost rather always write them out as files like ifcfg-rh does.  We have the same problem when we start using PKCS#11 URIs too, where the certificate reference/URI won't be valid on any other machine.  That's fine; I don't think NM should be in the business of passing arbitrary certificate data around.  If you have certificates they are already files somewhere (which is how you got them into NM in the first place) and so you will have to copy them to the other machine too.

Perhaps instead keyfile should package all the things it can find (including certificates) up into a tar.gz.  Note that with certificates and a store you usually can't get the certs/private key out of the store anyway.

That doesn't mean I disagree with the code you've got here though :)  More review early next week...

(In reply to Dan Williams from comment #23)
> (In reply to Thomas Haller from comment #21)
> > (In reply to Dan Williams from comment #18)
> > > Why are we trying to write stuff out as blob/data now?  We tried really hard
> > > to get away from that in the past, because there are so many certificate
> > > formats and parsing is a huge PITA.  I'd rather we didn't have a blob/data
> > > scheme at all, the only reason it's there is for backwards compatibility. 
> > > Honestly I don't think the keyfile writer should *ever* write data out as a
> > > blob, or at least we should never do that in the NM core at all.
> > 
> > Keyfile writer does still not ever write keyfiles with blobs. Nothing
> > changed there. It's not encouraging the use of blobs.
> > 
> > The only reason I add it now is backward compatibility.
> > 
> > > If we don't want to use file paths somewhere, then we should be using the
> > > pkcs#11 URI to point to it instead of passing some arbitrary blob that may
> > > be a PCKS#8 file, PKCS#1 file, raw DER, PEM-encoded, PKCS#12 blob, whatever.
> > > I'd rather have NM get out of the business of touching certificates/keys at
> > > all, even in binary form where it's just passthrough...
> > 
> > One goal is nmcli being able to import/export connections in keyfile format.
> > Keyfile being "the" format to represent NM connections.
> > For that keyfile must be able to express every valid connection (address
> > labels are missing too, btw).
> > 
> > If a connection contains (legacy) blobs, we still need a way to express
> > them. If you type `nmcli connection export legacy-connection`, somehow these
> > blobs must be represented. It would be very cumbersome if nmcli would write
> > them to some files (mktemp??).
> > With this change, the default bahavior of nm_keyfile_write() in face of
> > legacy blobs is to embed them in  keyfile as "data:;base64,". Which
> > nm_keyfile_read() can read back to make a full round-trip.
> 
> I'd almost rather always write them out as files like ifcfg-rh does.  

The writer of keyfile-settings-plugin still converts blob certificates to files. It still doesn't write blobs as blobs. No change there.


> We
> have the same problem when we start using PKCS#11 URIs too, where the
> certificate reference/URI won't be valid on any other machine.  That's fine;
> I don't think NM should be in the business of passing arbitrary certificate
> data around.  If you have certificates they are already files somewhere
> (which is how you got them into NM in the first place) and so you will have
> to copy them to the other machine too.
> 
> Perhaps instead keyfile should package all the things it can find (including
> certificates) up into a tar.gz.  Note that with certificates and a store you
> usually can't get the certs/private key out of the store anyway.

Why do you say "can't get the certs/private key out of the store"? URIs are not converted to blobs. URIs (file://) are still read/written as URIs. If an URI references to a missing file/certificate, it's a configuration error but no problem.

You were talking about import/export of connection data and the keyfile's handling of blob vs path.  I was talking about how keyfile should handle blobs, but as you point out libnm-core doesn't actually write files yet, and there are hooks to handle the certificate stuff.  So ignore me.

> libnm: combine get_cert_scheme() and verify_cert() and ensure valid paths for NMSetting8021x

Could we have get_cert_scheme() pass back a GError explaining why the file:// item was invalid?  eg, if the scheme is file:// but the string wasn't UTF-8, then it would be helpful for the client to get a more descriptive error.  Maybe just pass back a const char ** out string that the g_error_set() just below in verify_cert() could append to the error if set?

> libnm: ensure valid blob for nm_setting_802_1x_set_*_cert()

Instead of having to call this new function right after crypto_load_and_verify_certificate(), how about just wrapping that function with ensure_valid_blob_loaded() and renaming it to _load_and_verify_certificate() or something like that?

> libnm: fix clearing memory in file_to_secure_bytes()

Doesn't libnm-util's file_to_byte_array() need the same fix?

> keyfile: make reader more strict in handle_as_path()

The code comment has a "[" where I think you want "]".

> libnm: add keyfile support to libnm-core

Why remove the boolean return from read_array_of_uint()?  If the array is invalid, shouldn't the code issue a handle_warn()?

Should get_one_int() g_assert that if one of info/property_name are set, that both are?

> keyfile: refactor to use reading and writing of keyfile from libnm-core

It's a bit odd that nm_keyfile_read() takes a keyfile and just parses it, while nm_keyfile_write() returns a keyfile.  Would it make more sense to just pass some string data to nm_keyfile_read() and have it create the GKeyFile and populate it from the data internally?  That would save a few LoC in the keyfile plugin and nmtst_create_connection_from_keyfile().  The callers just create the keyfile itself and throw it away, and never do anything with it.

> keyfile: support writing certificates as blob inside the keyfile

The 'goto' jump in handle_as_scheme() is pretty odd, can't we just do a 'break' to get out of the loop and test whether (i < data_len) to return the error?

Also 'make check' in keyfile plugin fails now with:

libnm-CRITICAL **: Did not see expected message WARNING **: *<warn>  keyfile: 802-1x.client-cert: certificate or key file '/home/dcbw/Desktop/certinfra/client.pem' does not exist

Because that file actually exists on my system :)  We should probably change that path to something unique or figure out what's actually going on there.

======

I don't have any general objections to this branch any more, just code comments.  Thanks!

(In reply to Dan Williams from comment #25)

> > libnm: combine get_cert_scheme() and verify_cert() and ensure valid paths for NMSetting8021x
> 
> Could we have get_cert_scheme() pass back a GError explaining why the
> file:// item was invalid?  eg, if the scheme is file:// but the string
> wasn't UTF-8, then it would be helpful for the client to get a more
> descriptive error.  Maybe just pass back a const char ** out string that the
> g_error_set() just below in verify_cert() could append to the error if set?

done.

> > libnm: ensure valid blob for nm_setting_802_1x_set_*_cert()
> 
> Instead of having to call this new function right after
> crypto_load_and_verify_certificate(), how about just wrapping that function
> with ensure_valid_blob_loaded() and renaming it to
> _load_and_verify_certificate() or something like that?

done

> > libnm: fix clearing memory in file_to_secure_bytes()
> 
> Doesn't libnm-util's file_to_byte_array() need the same fix?

libnm-util doesn't use GBytes and does not clear the memory for the passwords.


> > keyfile: make reader more strict in handle_as_path()
> 
> The code comment has a "[" where I think you want "]".

it's intentional. The [ indicates that the upper index is excluded.


> > libnm: add keyfile support to libnm-core
> 
> Why remove the boolean return from read_array_of_uint()?  If the array is
> invalid, shouldn't the code issue a handle_warn()?

read_array_of_uint() doesn't ever return FALSE -- except when the assertion fails, which would indicate a bug.


> Should get_one_int() g_assert that if one of info/property_name are set,
> that both are?

done

> > keyfile: refactor to use reading and writing of keyfile from libnm-core
> 
> It's a bit odd that nm_keyfile_read() takes a keyfile and just parses it,
> while nm_keyfile_write() returns a keyfile.  Would it make more sense to
> just pass some string data to nm_keyfile_read() and have it create the
> GKeyFile and populate it from the data internally?  That would save a few
> LoC in the keyfile plugin and nmtst_create_connection_from_keyfile().  The
> callers just create the keyfile itself and throw it away, and never do
> anything with it.

nm_keyfile_read() goes together with nm_keyfile_write() in that it reads/writes a GKeyFile. In that sense, their input/output type matches.


All 3 users of nm_keyfile_read() do something different:

- nmtst_create_connection_from_keyfile() wants to read from a string
- nm_keyfile_plugin_connection_from_file() from a file. 
- libnm-core/tests/test-keyfile.c want so have the keyfile to assert against group-key-value entries.

We can add a wrapper around that accepts a string. But as there is no public API yet, that can be added later.


> > keyfile: support writing certificates as blob inside the keyfile
> 
> The 'goto' jump in handle_as_scheme() is pretty odd, can't we just do a
> 'break' to get out of the loop and test whether (i < data_len) to return the
> error?

done


> Also 'make check' in keyfile plugin fails now with:
> 
> libnm-CRITICAL **: Did not see expected message WARNING **: *<warn> 
> keyfile: 802-1x.client-cert: certificate or key file
> '/home/dcbw/Desktop/certinfra/client.pem' does not exist
> 
> Because that file actually exists on my system :)  We should probably change
> that path to something unique or figure out what's actually going on there.

done



rebased and repushed to master.

(In reply to Thomas Haller from comment #26)
> (In reply to Dan Williams from comment #25)
> > > libnm: add keyfile support to libnm-core
> > 
> > Why remove the boolean return from read_array_of_uint()?  If the array is
> > invalid, shouldn't the code issue a handle_warn()?
> 
> read_array_of_uint() doesn't ever return FALSE -- except when the assertion
> fails, which would indicate a bug.

I was thinking about error cases (where tmp == NULL) but looking at it more, since read_one_setting_value() loops over all properties and tries to get them, this function will be called for values that don't exist, and thus should just ignore that (which it does) and clear out the value in the setting (which it does).  So OK.

> > > keyfile: refactor to use reading and writing of keyfile from libnm-core
> > 
> > It's a bit odd that nm_keyfile_read() takes a keyfile and just parses it,
> > while nm_keyfile_write() returns a keyfile.  Would it make more sense to
> > just pass some string data to nm_keyfile_read() and have it create the
> > GKeyFile and populate it from the data internally?  That would save a few
> > LoC in the keyfile plugin and nmtst_create_connection_from_keyfile().  The
> > callers just create the keyfile itself and throw it away, and never do
> > anything with it.
> 
> nm_keyfile_read() goes together with nm_keyfile_write() in that it
> reads/writes a GKeyFile. In that sense, their input/output type matches.

I don't really agree...  nm_keyfile_write() returns a keyfile object, but doesn't take one as an argument.  It could return anything, it could even return a 'char *' containing the data.  But we wouldn't have nm_keyfile_read() take a pre-allocated array as the first argument, which is essentially what passing a GKeyFile does.

> All 3 users of nm_keyfile_read() do something different:
> 
> - nmtst_create_connection_from_keyfile() wants to read from a string
> - nm_keyfile_plugin_connection_from_file() from a file. 
> - libnm-core/tests/test-keyfile.c want so have the keyfile to assert against
> group-key-value entries.
> 
> We can add a wrapper around that accepts a string. But as there is no public
> API yet, that can be added later.

So I did a whole patch to get rid of this, and it's less code, but I guess it doesn't really matter since this API isn't public.  I can attach that patch if you'd like.

But it did bring up a question about _keyfile_convert().  These two test are entire NOPs right?

		*keyfile = k;
	} else
		k = *keyfile;

     ...

		*con = c;
	} else
		c = *con;

	nmtst_assert_connection_equals (c, FALSE, *con, FALSE);
	_keyfile_equals (k, *keyfile);

since the pointer values of *con + c and k + *keyfile will be exactly the same, what's the point in testing them against each other?

Created attachment 299218 [details] [review]
libnm-keyfile-cleanup.patch

To be clear about nm_keyfile_read(), I now don't care either way.  Here's the patch I did though.

For nm_keyfile_read(), the only code that cares about getting a keyfile back is the test code, and in general I think we should make the test code more complex and keep the real code simpler.  But I guess in this case it's a difference of 10 LoC in the real code, so meh.

And to be even clearer, ACK on the branch as-is.

merged to master as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=59eb5312a5d64fd4dd91b2348ab035506bfb12a4

All is still internal API (nm-keyfile-internal.h)

Close this branch, and let first users (bug 744702 ?) nail down the public API.

(In reply to Thomas Haller from comment #31)

> Close this branch, and let first users (bug 744702 ?) nail down the public
> API.

... including providing a simpler API that operates on string instead of GKeyFile (as dcbw suggested).