search

OpenSC / pkcs11-helper

Library that simplifies the interaction with PKCS#11 providers for end-user applications using a simple API and optional OpenSSL engine
Other
66 stars 43 forks source link

Migrate serialization format to standard PKCS#11 URIs compliant with RFC7512 #4

Open dwmw2 opened 9 years ago

dwmw2 commented 9 years ago

This migrates the serialization format to conform to the PKCS#11 URI scheme as described in RFC7512.

The old form is still recognised for compatibility, but standard PKCS#11 URIs are now generated and accepted. Testing with OpenVPN now gives me the following output:

$ openvpn --show-pkcs11-ids /usr/lib64/pkcs11/gnome-keyring-pkcs11.so 

The following objects are available for use.
Each object shown below may be used as parameter to
--pkcs11-id option please remember to use single quote mark.

Certificate
       DN:             DC=com, DC=intel, DC=corp, DC=ger, OU=Workers, CN=Woodhouse, David, emailAddress=david.woodhouse@intel.com
       Serial:         1EB2ECCF00030058F375
       Serialized id:  pkcs11:model=1.0;manufacturer=Gnome%20Keyring;serial=1%3aUSER%3aDEFAULT;token=Gnome2%20Key%20Storage;id=%59%ae%17%70%af%e8%af%9f%5b%94%fb%c6%89%f6%f1%4c%11%5c%36%0e

... and the PKCS#11 URI thus generated is also usable with the --pkcs11-id option.

dwmw2 commented 9 years ago

Updated push: helps if I NUL-terminate the serialised strings.

alonbl commented 9 years ago

I am unsure what is the cutoff between these two patches, can you please explain?

dwmw2 commented 9 years ago

I think this should address the feedback. I've made incremental commits for easy of review but obviously we can collapse this into the original commit(s) before merging if that's preferred.

I've made it use _pkcs11h_util_hexToBinary() as you suggest, for which I had to stop that function requiring that the string be NUL-terminated after the length it was asked to parse. Which was simple enough; there was only one other caller and I just made sure it checked for itself.

The loop parsing URI attributes no longer contains continue. Not quite sure what the problem is with that, but it's easy enough to remove it as you observed.

I've cleaned up the code parsing the legacy token/certificate IDs to be more symmetric with the URI parsing instead of duplicating the return paths.

I think you were asking why we keep pkcs11h_token_deserializeTokenId() if it's no longer used? It's an exported function. We no longer use it as a helper from pkcs11h_certificate_deserializeCertificateId() but we can't actually kill it.

I've made a table of the token ID fields which can now be used in three places — the parsing code, and twice during the serialization, which (following the lead of the existing code) was processing the fields twice: once for calculating the length of the string required, and again for actually generating the string. I've cleaned up some of the other code paths and duplication in the latter too.

I've prefixed static symbols with __ although I'm a little confused about that request. These really are static symbols within the C unit, not functions which are visible to other C units within the library, but intended to be private to the library. My understanding is that even if you're building without a decent linker that has visibility controls and symbols maps, a symbol which is declared as 'static' within a C file is really not going to be seen elsewhere. So you can call it absolutely anything you like... with the caveat that you should not use symbols starting with two underscores, because those are reserved for the system. But OK :)

I've defined URI_SCHEME and used it instead of the bare "pkcs11:" where appropriate.

dwmw2 commented 9 years ago

Now that the PKCS#11 URI format has been published as RFC7512 it would be very good to get this merged... should I collapse the above fixes into the original commits and resubmit? After this length of time there doesn't seem much point in keeping them incremental any more...

nmav commented 9 years ago

The pkcs11: URI format is a standard (RFC7512) since this April. It would be nice if pkcs11-helper would support that format.

dwmw2 commented 9 years ago

Updated patch with minor fix from https://bugzilla.redhat.com/show_bug.cgi?id=1264645

dwmw2 commented 8 years ago

Ping. It would be really good to get this merged...

alonbl commented 8 years ago

Hi, As I wrote, your implementation is much more complex than it should, it won't get merged. I need to rework this into something simpler, but this is extremely low priority for me. Thanks, Alon

On 23 August 2016 at 16:47, dwmw2 notifications@github.com wrote:

Ping. It would be really good to get this merged...

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/OpenSC/pkcs11-helper/pull/4#issuecomment-241736063, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNIrSZPebF7EBGQmup94jOCvJftFw2Jks5qivncgaJpZM4DGws9 .

dwmw2 commented 8 years ago

It could be massively simplified if it were to just use libp11-kit for the URI parsing. Other than that, I'm not sure how it could be simpler.

emmanuel-deloget commented 7 years ago

Hi,

RFC7512 is now two years old. Could this patch (or any equivalent patch) be merged into pkcs11-helper ?

Best regards,

-- Emmanuel Deloget

dwmw2 commented 6 years ago

Updated with bug fix from https://bugzilla.redhat.com/show_bug.cgi?id=1516474

celesteking commented 6 years ago

Can we get this crap merged finally so openvpn actualy works with smartcards properly via NetworkManager? Thanks.

boomer41 commented 5 years ago

Any updates on this one?

alonbl commented 5 years ago

As I wrote, this patch is way to complex to achieve the desired task, and is partial as it supports only full pkcs11 url which as far as I understand is the opposite of reaching portability between applications that support the notation. I also got reports that the windows version of openvpn in which this patch is applied is failing to parse urls.

I am unsure how it is that important to network manager and am unsure why the openvpn management interface cannot be used to detect the available ids and apply id as opaque. And I keep reminding people that at daemon context openvpn should not have access to user's devices anyway, and the proper solution is to remove the entire pkcs11 support from the daemon and make sure the management interface is capable of delegating all private key operation to the user interface program, however, it seems like the openvpn project is on freeze.

dwmw2 commented 4 years ago

Updated with fix for https://bugzilla.redhat.com/show_bug.cgi?id=1825496

As I wrote, this patch is way to complex to achieve the desired task,

I'm not quite sure how it could be simpler. I suppose it could take a library like libp11-kit as a dependency instead of doing the serialisation/deserialisation for itself?

and is partial as it supports only full pkcs11 url which as far as I understand is the opposite of reaching portability between applications that support the notation.

That doesn't seem to make much sense to me; can you explain what you mean? It will output the full URI when serialising, when listing all available objects. But obviously accepts only a partial URI (i.e. not redundantly specifying all attributes) when identifying an object. In that respect, it behaves like everything else does, and should.

I also got reports that the windows version of openvpn in which this patch is applied is failing to parse urls.

A specific bug report would be very much appreciated, if there is misbehaviour.

I am unsure how it is that important to network manager and am unsure why the openvpn management interface cannot be used to detect the available ids and apply id as opaque.

There is work on displaying the available objects through a UI (like in Seahorse) and that widget will emit the RFC7512 ID identifying the chosen object. For that, we need to be using the standard.

And I keep reminding people that at daemon context openvpn should not have access to user's devices anyway, and the proper solution is to remove the entire pkcs11 support from the daemon and make sure the management interface is capable of delegating all private key operation to the user interface program, however, it seems like the openvpn project is on freeze.

Yeah, it should do proxying from something in the user's session. But that's slightly orthogonal to the fact that we want to use a standard form to identify PKCS#11 objects.

becm commented 4 years ago

OpenVPN uses an out-of-date patch set which is bound to fail when maxing out certain token attribute entries, 16-byte serials being the most common case.

This was my first problem as well, then something else cropped up. Sadly neither seems to have made it in time for OpenVPN 2.4.9 release.

williamcroberts commented 4 years ago

Literally without this patch, things are broken. Fedora patches their stuff with this. I just bumped into this, can we somehow get this merged or get a clear path forward?

becm commented 4 years ago

The menioned problems with OpenVPN should be fixed by updated patch used in current builds for Windows. Update of included pkcs11-helper version also addresses EC support detection problem and adds support for PSS padding.

saper commented 1 month ago

Seems like this change might break access to some HSMs

https://bugzilla.redhat.com/show_bug.cgi?id=2298882 https://github.com/alonbl/gnupg-pkcs11-scd/issues/63

saper commented 1 month ago

your implementation is much more complex than it should

https://github.com/OpenSC/pkcs11-helper/pull/69 is my attempt to see if this can be done simpler. For now, this branch can only use URL encoding instead of \x20.

dwmw2 commented 1 month ago

Updated with fix for https://bugzilla.redhat.com/show_bug.cgi?id=2298882 (untested). Discussion in https://github.com/alonbl/gnupg-pkcs11-scd/issues/63