Cryptsetup pkcs11 - #2776

This does not compile, please run make distcheck locally. Also, please have a look at the CODING_STYLE file to bring your code in line with the project defaults.

Hmm, what's the strategy here regarding the smartcard being required at early boot, but the smartcard reader not having been probed that early yet? This sounds racy, what's you strategy to handle that?

zongue,poettering: Thank you for your quick responses!

zonque: I've updated the PR with a new commit that adds the missing header file to Makefile.am. I will take a look at the coding guidelines tomorrow.

poettering:
cryptsetup-generator adds "Wants=pcscd" to the volume's unit file. This works on my system.
There's of course the possibility to have a keyboard input password as well in your LUKS header as a backup/rescue option.
I realise that my strategy here is naive. I do not know systemd, I am new to using it, so if you can suggest a better way of making sure the smartcard reader is probed before cryptsetup then please do so :)
I did consider forking the process to be able to probe for smartcards while still asking for a keyboard-entry password, but that seemed like the bad choice.

Isn't pcscd a late boot service? cryptsetup is an early-boot and initrd service, i.e. it runs in a very limited, very early environment, and are you sure pcscd is available that early?

Also, note that Wants=pcscd.service pulls in that service, but defines no ordering against it. if you want to order your service after it you have to add After=pcscd.service too. But of course, that's going to break if pcscd is a late-boot service (which I think it is)...

Moreover just ordering pcscd before the cryptsetup is unlikely to be enough... The thing is that hardware (in particular USB hardware) can take any time it wants to be probed. As we boot up, and as the cryptsetup logic is run, hardware is still popping up, and the smartcard reader device might not have shown up yet. Thus the hdd/cryptsetup logic will race against the smartcard probing: if the smartcard reader is quicker and is probed first, and the hdd shows up later, and thus cryptsetup is started later than all is good. However, if the reverse happens, and the hdd is probed first and the smartcard reader only shows up later, then we might end up using the smartcard API before the device is actually available, and the operation will fail. Given that hardware initialization times are effectively random the smartcard stuff hence might work sometimes, but other times it will fail, depending on probing times.

To make this reliable I figure it's necessary to actively wait for smartcard devices to show up by watching udev, if the smartcard setting is set in crypttab...

That all said, I have no experience with smartcards, and I don't know pcscd... Maybe pcscd is actually ready to be run in early boot, and maybe it already has provisions for waiting for devices, but my guess it is and does neither, hence my question regarding the strategy with this.

Note that the patch doesn't follow the coding style we adopted for systemd, already superficially when it comes to indenting and placing brackets. Please have a look at the CODING_STYLE file and realign the patch accordingly.

I will look into waiting for a smartcard reader to be probed and a smartcard to be inserted. Ideally I would like the user to be able to cancel this if they have a keyboard-input LUKS key as fallback, but I'm not sure how askpassword would handle that. Otherwise I'm thinking a configurable timeout, what would be the appropriate way to do that?

As for the notes on the coding_style, I still have to read through the entire file but I want to ask, are you referring to the changes in cryptsetup.c and cryptsetup-generator.c or are you referring to pkcs11.h? The header file is borrowed from scute (www.scute.org) and I have not made any changes to it except for expanding tabs to spaces. What is the systemd way of handling these kinds of headers? Would you rather I rewrite it in the systemd code style? (benefit being that it conforms to the coding style, with the obvious disadvantage that there won't be an easy way to update if the source (scute) makes fixes and/or improvements)

pkcd11.h is packaged (e.g. pkcs11-helper-devel.rpm on Fedora). So this file should not be copied into systemd.

Instead, do something like this:

• add to configure.ac:

# ------------------------------------------------------------------------------
have_pkcs11=no
AC_ARG_ENABLE(pkcs11, AS_HELP_STRING([--disable-pkcs11], [disable support for PKCS11]))
AS_IF([test "x$enable_pkcs11" != "xno"], [
        PKG_CHECK_MODULES(PKCS11, [libpkcs11-helper-1],
                [AC_DEFINE(HAVE_PKCS11, 1, [Define if pkcs11-helper-1 library is available]) have_pkcs11=yes],
                [have_pkcs11=no])
        AS_IF([test "x$have_pkcs11" = "xno" -a "x$enable_pkcs11" = "xyes"],
                [AC_MSG_ERROR([*** libpkcs11-hepler-1 support requested but libraries not found])])])
AM_CONDITIONAL(HAVE_PKCS11, [test "$have_pkcs11" = "yes"])

and then add one line at the bottom of configure.ac which prints the status.

• make the code conditional on HAVE_PKCS11.

Another issue is that the code must be OOM-safe. I.e. every operation which allocate memory, e.g. cescape, must be checked for failure, and it must be handled by cleanly returning an error.

I don't understand why pk11 is global. It seems to be constructed and destroyed in the same function.

I will look into waiting for a smartcard reader to be probed and a smartcard to be inserted

I think this should be done in pcscd. I.e. when systemd detects that pcscd should be used, it should launch pcscd.service, and that daemon should watch udev and react to any devices that pop up.

I have just pushed a few commits with mostly cleanup of the code but also making sure that there's an option (waitforpkcs11) that makes systemd-cryptsetup wait for smartcard readers and smartcards for DEFAULT_TIMEOUT_MSEC.
I realize there may still be style errors, please point them out if you find any.
Also, I've tried to be better about freeing, but I think there may still be some places where I should free but don't, so please comment on that as well if you see anything I've missed.

Looking forward to your comments :)

Please always rebase/squash your patches.

Thank you for your feedbak keszybs, I have squashed and rebased now, as well as fixed the error that you pointed out.

Let's not talk about style or implementation details until we have the high level details worked out.

In what circumstances would you want to set pkcs11key= but not waitforpkcs11? It would seem that you always want to wait if pkcd11key is specified. If this is supposed to be used to decrypt disks during bootup, than waiting is the only reliable option.

Unfortunately I don't think we can accept the implementation of arg_pkcs11_waitfor as it currently looks. It does a busy loop (or almost, in 100ms intervals), until the hardware shows up. That's just too ugly and inefficient to put in systemd. The only reasonable way is to watch for udev events and reacts as the devices pop up. I don't know if the pkc11 library provides that, but it really should.

Also, I don't think there's any reason to use dynamic loading. Why not link to -lpkcs11-helper in the usual way? (autoconf sets PKCS11_LIBS and PKCS11_CFLAGS).

This functionality seems very useful, but the amount of code to add it is rather small, but if it lands in systemd the implementation must be reasonable. So no busy waits, sorry.

Thank you for your comments keszybz :)
The reason you'd want to have pkcs11key but not waitforpkcs11 is in the case where your LUKS header is set up with one key from pkcs11 and one key that is a traditional keyboard input passphrase. This is a cornercase that shouldn't have to be used, so I'm thinking we'll remove this option.

And it'd be great if this removed the need for a timeout, but I don't think it does. I think that in the case of a device that's allowed to fail, there needs to be either a one-shot try at getting the key or a sensible timeout, no?

Ideally the user would be presented with the choice to either insert a pkcs11 device or skip unlocking this volume, I'm not sure if this kind of support should/could be included in the password agents?

I'm not really a fan of the busy-wait loop myself. It came from the need of a timeout and the fact that pkcs11 provides only a nonblocking WaitForSlotEvent and an infinite blocking one. Using the infinitely blocking one would require a fork() in order to provide a timeout, but perhaps that's preferable? If so, please do point me to an example of how it's done the 'systemd-way'.
Also, reinitializing the pkcs11 was necessary for my system to recognize the smartcard reader being plugged in, so I'm not sure how the blocking version of WaitForSlotEvent deals with that. I will investigate this.
Generally, I don't think there's a way to use udev events to watch for pkcs11 hardware since pkcs11 is a platform-independent library and there's no way to know if a device represents a pkcs11 reader/token or not.

I will look into static linking instead of dynamic loading, the loading is an artifact from early development of this when I thought I'd potentially need to probe several cryptoki implementations. However, now I'm thinking we'll stick to only opensc right now as that seems to be what is used. Please correct me if I'm wrong in this :)

It seems that since this commit (OpenSC/OpenSC@5a23069) only GetFunctionList is exported in opensc-pkcs11.so, and that static linking is not generally how pkcs11/cryptoki is done.
If anyone has more experience with pkcs11/cryptoki, please feel free to jump in here and educate me, I only started looking at this a week ago or so :)

EDIT: It also seems that all functions are exported again in OpenSC since this commit (OpenSC/OpenSC@52b6505) following the discussion of issue #183 (OpenSC/OpenSC#183). However, the version included in my distribution (debian testing) does not export all symbols.

Would static linking be prefered even if the function references are loaded through C_GetFunctionList?

The reason you'd want to have pkcs11key but not waitforpkcs11 is in the case where your LUKS header is set up with one key from pkcs11 and one key that is a traditional keyboard input passphrase.

I still don't see how that would obviate the need to wait. If you have pkcs11key configured, even if in the end you decide not to use it and type in the password instead, the machine has no way of knowing your intent, and has to present you with a valid pkcs11 prompt, so it has to wait for the hardware. Otherwise the case where you do want to use pkcs11 would not work reliably.

And it'd be great if this removed the need for a timeout, but I don't think it does. I think that in the case of a device that's allowed to fail, there needs to be either a one-shot try at getting the key or a sensible timeout, no?

Yes, a sensible timeout. (Probably there should be an option to specify the timeout, but that's something that can easily be added later). There should always be a timeout, even if the device is not allowed to fail. In that case we want to fall back to emergency.target or do other error handling.

commit (OpenSC/OpenSC@5a23069)

It seems that this commit must have been reverted at a later point. https://github.com/OpenSC/OpenSC/blob/master/src/pkcs11/pkcs11.exports has a long list of functions.

static linking

Static linking is even worse than dlopen ;) Normal "shared" linking only please.

Ideally the user would be presented with the choice to either insert a pkcs11 device or skip unlocking this volume, I'm not sure if this kind of support should/could be included in the password agents?

There's always the option of simply entering an empty password a few times. I don't know if our password agents currently have an option to cancel. It might be useful, but either way, it's not strictly related to this pull request.

pkcs11 provides only a nonblocking WaitForSlotEvent and an infinite blocking one. Using the infinitely blocking one would require a fork() in order to provide a timeout, but perhaps that's preferable? If so, please do point me to an example of how it's done the 'systemd-way'.

Then it should be done as a separate process. Basically cryptsetup would "announce" the need for a pkcs11 password (similarly how it "announces" the need for a normal password), maybe start an external service, and wait for a reply until some timeout.

This circles back to the topic of "binary passwords" that was discussed before (https://lists.freedesktop.org/archives/systemd-devel/2014-July/020880.html) without reaching a clear resolution. We are now using the keyring, but I don't know if this changes much, and if the kernel keyring would be useful here.

Actually, wouldn't the normal password agent logic be appropriate here? If binary passwords were supported, then cryptsetup could emit a question that contains all the information for the pkcs11 agent, and the agent could query for the PIN and send back the answer.

This has two issues:

• binary password protocol: I guess the patches from David Härdeman could be revived [1]. Back then it seemed that kdbus might become a thing, so it made sense to delay. But right now there's no clear plan for that, so this should be revisited.
• the pkcs11 agent: the agent would basically contain the loop that you wrote, or something similar. If the pkcs11 library does not provide a better API, it could do the loop itself in a smarter way by waiting for udev events and doing the reinitialization step if a new device pops up.

[1] https://lists.freedesktop.org/archives/systemd-devel/2014-June/020616.html

Right now I'm feeling kind of discouraged about getting to a good solution here. sigh

There's two major setbacks right now:

1. opensc doesn't support blocking waitforslotevent (https://github.com/OpenSC/OpenSC/blob/master/src/pkcs11/pkcs11-global.c#L666) so if we want to keep using opensc there will have to be a busy-wait loop. :(
2. On my distribution the opensc package doesn't provide a .pc file for pkg-config, so dynamically linking to this won't be pretty. :(

I'm going to research if there's a better pkcs11 implementation to link against than opensc but if not, I don't see how this is going to be good enough to be a part of systemd, which is kind of sad because it works pretty well for me locally by now and I do think the feature would be appreciated.

The pkg-config file is just a nicety that makes it easy to detect if the library is installed. Linking with pkcs11-helper is not much more than adding -lpkcs11-helper to the link flags.

With a bit of udev magic, the "busy loop" could be made quite smart. If you add a filter that corresponds to some superset of the devices that could be used, and react to new devices showing up, restarting would only have to happen when new devices are detected. After the initial phase of hardware discovery, there would be no more looping.

The pkg-config file is just a nicety that makes it easy to detect if the library is installed. Linking with pkcs11-helper is not much more than adding -lpkcs11-helper to the link flags.

Yes, the thing is however that I'm not interested in linking against pkcs11-helper (the dependency on pkcs11-helper was added following a comment that the pkcs11.h was packaged in pkcs11-helper. The only file used from that package is the header file, which btw. is also packaged with icedove, iceweasel, libgnutls and others). The library I need to link against in order to not do dynamic loading is opensc-pkcs11.so (and probably other libraries on other platforms), this would be done by adding -l:opensc.pkcs11.so. This begs the question of what platforms/distributions are the target of systemd?
I'm asking because I don't know. I know Solaris provides a PKCS11 provider, as well as a soft token provider. Red Hat claims support of multiple providers tracker through "secmod.db". I tried looking at secmod.db on a Fedora box but that seemed broken. I don't know Red Hat so I don't know if this is a feature that's supported and/or used. On other platforms/distributions there's potentially other pkcs11 providers and/or discovery mechanisms.

With a bit of udev magic, the "busy loop" could be made quite smart.

No. I'm sorry, but udev magic won't help us here. I earlier stated that PKCS11/cryptoki has a blocking WaitForSlotEvent function. While that is true, the standard specifies such a function, the opensc implementation of pkcs11/cryptoki will always return an error on calling the blocking version of that function. So in reality, when using opensc, there's no blocking version of WaitForSlotEvent and thus a busy loop is required.
As for udev magic in order to not have to reinitialize, pkcs11/cryptoki is a platform independent standard. While I could make an educated guess as to what devices could possibly be pkcs11 slot providers, there's no way of knowing for sure. I know that some usb tokens have a built in pkcs11 "reader" as a sub-device, as well as that there are serial-attached and PCMCIA smart card readers.
Also, I think udev would only be helpful in determining when a new reader is attached, not whether a card is inserted into a reader or not.

I will look further into providing pkcs11 as a password agent, if this is deemed a more reasonable place to have a busy-wait loop.

This begs the question of what platforms/distributions are the target of systemd?

At least that's easy. Linux only.

I will look further into providing pkcs11 as a password agent

Yes, this seems to be the best direction.

A note on the last commit:
This refactoring is (so far) untested since I'm away from home and don't have the hardware. It make distchecks though and I would like to receive your comments on it.
The way I see it, it's either this way or trying to get PKCS#11 support into the "real" cryptsetup, which I think could be quite cumbersome.
Note that I reverted to dynamic loading after noticing that PKCS#11 URI:s has a "module-path" attribute. It also makes sense to let the user decide which PKCS#11 provider to use.
Now non-blocking polling is used only as a last resort if the provider returns CKR_FUNCTION_NOT_SUPPORTED when trying a blocking wait.

I will update when I get home and can test this properly.
Looking forward to your comments :)

I've ironed out a few kinks. This works as expected under a running system.
In an initrd environment I'm experiencing some problems with pcscd, but not with this patch.
I would ask you to reconsider taking this route, as the alternatvies seem worse.
I've looked into the password-agent protocol, but that doesn't really support this behaviour, so implementing this as a password-agent would mean rewriting the password-agent protocol.
I've also looked at including it in cryptsetup, but there seems to be no support for that on the mailing list, and I'm thinking it would be too low-level. It's probably better to go that way than the password-agent route though.

I've now tested this on Fedora 23 with dracut initrd. If you include pcscd and the relevant reader drivers in the initrd, as well as make sure pcscd is compiled without polkit support (since polkit isn't available at initrd-time) it works well. I can do a writeup of the procedure I followed if that helps.

As for the discussion on wether to include pkcs11 support here or in cryptsetup (dmcrypt) please see the following discussion with their developers: http://article.gmane.org/gmane.linux.kernel.device-mapper.dm-crypt/8548 ("in new version of LUKS we plan to add some kind of token
support")

Sidenote: I think the autopkgtest-systems were broken during my last commit, at least make distcheck runs for me.

Thanks for the update. We're getting ready for a release, so review might be a bit delayed.

I get the "a bit delayed" part but now it's been three months? Any update on this keszybz?

This PR is 6 months old, stale, and got rejected in its present form. If this is still relevant, can you please fix and rebase to current master, and otherwise close this? Thanks!

@martinpitt This is stale since @keszybz said that review might be "a bit delayed" since you were getting ready for a release? This was in May, and there seems to have been releases of systemd since?
I will rebase this and retest (against Fedora 24 I'm guessing) if someone can confirm that this actually has chance of being merged at some point.

Since v245 of systemd we actually support a different method of hooking up PKCS#11 with cryptsetup. Instead of storing anything on the pkcs#11 key we decrypt an encrypted version of the LUKS unlocking key with a RSA key from the PKCS#11 device. Thus we can re-use keys that exist on the device anyway, and can unlock as many volumes with the same key as we want.

I think this is a good alternative to what is proposed in this PR, hence let's close this one.

Sorry this took so long. And sorry for not reviewing this PR properly early on!