GNOME Bugzilla – Bug 776268
[review] lr/pkcs11: allow use of pkcs11: URIs with the supplicant
Last modified: 2017-01-06 15:17:56 UTC
https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/pkcs11 Limited usefulness for now -- the openssl engine is wpa_supplicant seems hopelessly broken when using more tokens and its parsing of pin-value is wrong too. Also, the path for asking for secrets is missing. Those will be subject of future work. Still, it works when you only use a key from a token, which is you'll end up doing when using TPM anyway (since it doesn't seem to be capable of anything more than that).
> libnm-core/8021x: add pkcs11: scheme for certificates and keys nm_setting_802_1x_get_ca_cert_uri() et al must ensure that they return a NUL terminated char-string. Basically, fix nm_setting_802_1x_check_cert_scheme() to do the checks same checks on the blob as done for file:// schemes. Thereby, also check for utf8 encoding too(?). For file paths we also check for utf8 encoding, which one might consider a bug, since filenames on Linux don't have an encoding *sigh*. But as we are already so strict for file names, we should be also so strict with pkcs11 URIs. nm_setting_802_1x_get_phase2_ca_cert_uri() et al. should be called *_uri(). The scheme is called "*_PKCS11", thus the getter for such a scheme should be called nm_setting_802_1x_get_phase2_ca_cert_pkcs11(). > cli: support printing the pkcs11: URI scheme let's convert the "if()" to "switch (nm_setting_802_1x_get_ca_cert_scheme (s_8021X))". That is nice because if you omit the default label, there will be a compiler warning about unhandled enum values. While at it, is there a way to reduce the code-duplication with the getter/setters? Maybe not... > keyfile: add support for pkcs11: URI scheme Your extension to handle_as_scheme() does not verify that the resulting binary is a valid pkcs11 scheme. It should call nm_setting_802_1x_check_cert_scheme() which is the central validation function for scheme. See also previous point to extend validation of the URI. Basically, do the same as for file:// cert_writer_default() must also be extended. Likewise cert_writer().
(In reply to Thomas Haller from comment #1) > > libnm-core/8021x: add pkcs11: scheme for certificates and keys > > nm_setting_802_1x_get_ca_cert_uri() et al must ensure that they return a NUL > terminated char-string. > > Basically, fix nm_setting_802_1x_check_cert_scheme() to do the checks same > checks on the blob as done for file:// schemes. Done. > Thereby, also check for utf8 encoding too(?). For file paths we also check > for utf8 encoding, which one might consider a bug, since filenames on Linux > don't have an encoding *sigh*. But as we are already so strict for file > names, we should be also so strict with pkcs11 URIs. Yes. The PKCS#11 contain UTF-8 strings and blobs that are %-escaped into US-ASCII subset. Thus it makes sense to validate they're UTF-8 in any case. > nm_setting_802_1x_get_phase2_ca_cert_uri() et al. should be called *_uri(). > The scheme is called "*_PKCS11", thus the getter for such a scheme should be > called nm_setting_802_1x_get_phase2_ca_cert_pkcs11(). That's intentional. There's nothing PKCS#11 specific in the getter (well, the assert, that is there so that we, God forbid, don't use the getter accidentally). If we ever add more schemes that would be accepted by supplicant as they are, it makes sense to me to use the same URI getter. > > cli: support printing the pkcs11: URI scheme > > let's convert the "if()" to "switch (nm_setting_802_1x_get_ca_cert_scheme > (s_8021X))". That is nice because if you omit the default label, there will > be a compiler warning about unhandled enum values. Done. I've also made the uri getter default, since the assumption that whatever is there that is not a blob or a file name would be an URI makes sense to me. That way the older client could deal with new schemes added, at the expense of making it difficult extend it with a scheme that is not an URI scheme. I don't think we would want to do it anyway. (Did this mostly because the compiler insisted on dealing with all possible enum values, but default-casing this makes sense to me.) > While at it, is there a way to reduce the code-duplication with the > getter/setters? Maybe not... I thought the same, but couldn't figure that out :( > > keyfile: add support for pkcs11: URI scheme > > Your extension to handle_as_scheme() does not verify that the resulting > binary is a valid pkcs11 scheme. It should call > nm_setting_802_1x_check_cert_scheme() which is the central validation > function for scheme. See also previous point to extend validation of the URI. > Basically, do the same as for file:// > > cert_writer_default() must also be extended. Likewise cert_writer(). Done. Streamlined handle_as_scheme() by replacing the redundant string compares with use of scheme returned from nm_setting_802_1x_check_cert_scheme(). Did not give it much testing yet, I'm going to test it now. Updated the branch.
The new API needs NM_AVAILABLE_IN_1_6. It also need GTK-doc with "Since: 1.6" tags. nm_setting_802_1x_check_cert_scheme() is good now. As a follow-up (new) feature... currently, there is no URI scheme for blob for the settings property. Note that keyfile reader/writer does support such a scheme, and it makes sure to qualify binary blobs with a data with "data:;base64,". That means, you cannot store a blob that starts with "file://". Ok, maybe that isn't gonna happen, right? It still feels wrong to have arbitrary restrictions on which blobs we support. How about to add a URI scheme "binary:" that is handled accordingly? Basically: #define NM_SETTING_802_1X_CERT_SCHEME_PREFIX_BINARY "binary:" the difficulty is that in that case nm_setting_802_1x_get_ca_cert_blob() cannot directly return priv->ca_cert. So, it would have to create and cache an internal clone. Note that this is not added to libnm-util, which is fine. But we should ensure that legacy libraries still behave sensible in face of pkcs11 URIs. I guess, they are just treated as blobs, which is wrong, but probably OK. I just mean: have an eye on that (preferably test it once). > > While at it, is there a way to reduce the code-duplication with the > > getter/setters? Maybe not... > I thought the same, but couldn't figure that out :( the change in handle_as_scheme() is wrong. You can only use nm_setting_802_1x_check_cert_scheme() on the blobs in the NMSettings8021x properties (like NM_SETTING_802_1X_CA_CERT). But "GBytes *bytes" in handle_as_scheme is the value as read by keyfile's get_bytes(). They are not the same. E.g. keyfile encoding supports a prefix NM_KEYFILE_CERT_SCHEME_PREFIX_BLOB, which nm_setting_802_1x_check_cert_scheme() does not. Note that there are some similarities. E.g. in previous code, we do call to nm_setting_802_1x_check_cert_scheme() after verifying that the binary keyfile blob starts with "file://". That is, for encoding for a PATH it identical. But not for binaries. Other example: keyfile's NM_KEYFILE_CERT_SCHEME_PREFIX_BLOB is base64 encoded. settings' NM_SETTING_802_1X_CK_SCHEME_BLOB is a binary blob. You cannot mix them.
(In reply to Thomas Haller from comment #3) > The new API needs NM_AVAILABLE_IN_1_6. It also need GTK-doc with "Since: > 1.6" tags. Added. > As a follow-up (new) feature... currently, there is no URI scheme for blob > for the settings property. Note that keyfile reader/writer does support such > a scheme, and it makes sure to qualify binary blobs with a data with > "data:;base64,". > That means, you cannot store a blob that starts with "file://". Ok, maybe > that isn't gonna happen, right? It still feels wrong to have arbitrary > restrictions on which blobs we support. > How about to add a URI scheme "binary:" that is handled accordingly? > > Basically: > > #define NM_SETTING_802_1X_CERT_SCHEME_PREFIX_BINARY "binary:" > > the difficulty is that in that case nm_setting_802_1x_get_ca_cert_blob() > cannot directly return priv->ca_cert. So, it would have to create and cache > an internal clone. Agreed. > Note that this is not added to libnm-util, which is fine. But we should > ensure that legacy libraries still behave sensible in face of pkcs11 URIs. I > guess, they are just treated as blobs, which is wrong, but probably OK. I > just mean: have an eye on that (preferably test it once). I did test this with older libnm based nmclient; it would indeed just use the blobs. I assume the libnm-util behavior is the same. I don't feel like spending too much time on this, since it probably works and is not very straightforward to test. > the change in handle_as_scheme() is wrong. > You can only use nm_setting_802_1x_check_cert_scheme() on the blobs in the > NMSettings8021x properties (like NM_SETTING_802_1X_CA_CERT). > But "GBytes *bytes" in handle_as_scheme is the value as read by keyfile's > get_bytes(). > They are not the same. E.g. keyfile encoding supports a prefix > NM_KEYFILE_CERT_SCHEME_PREFIX_BLOB, which > nm_setting_802_1x_check_cert_scheme() does not. > Note that there are some similarities. E.g. in previous code, we do call to > nm_setting_802_1x_check_cert_scheme() after verifying that the binary > keyfile blob starts with "file://". That is, for encoding for a PATH it > identical. > But not for binaries. > Other example: > keyfile's NM_KEYFILE_CERT_SCHEME_PREFIX_BLOB is base64 encoded. > settings' NM_SETTING_802_1X_CK_SCHEME_BLOB is a binary blob. > You cannot mix them. Ew. Okay. Backed it off, solved diffetently. Updated the branch.
> libnm-core/8021x: add pkcs11: scheme for certificates and keys Can you also add the new scheme to the documentation of nm_setting_802_1x_get_*_scheme() functions? > libnm-core: make cert/key setters handle pkcs11 URI scheme static GBytes * -path_to_scheme_value (const char *path) +value_with_scheme (const char *path, const char *scheme) { The function body should be updated to handle the new argument. The rest LGTM.
(In reply to Beniamino Galvani from comment #5) > > libnm-core/8021x: add pkcs11: scheme for certificates and keys > > Can you also add the new scheme to the documentation of > nm_setting_802_1x_get_*_scheme() functions? > > > libnm-core: make cert/key setters handle pkcs11 URI scheme > > static GBytes * > -path_to_scheme_value (const char *path) > +value_with_scheme (const char *path, const char *scheme) > { > > The function body should be updated to handle the new argument. > > The rest LGTM. Both fixed & merged.