PKCS#11 support for Touch and PIN policies
kevinjacobs opened this issue · 15 comments
Hi. I'm working on a library that exposes 2FA functionality through PKCS#11 with certain hardware tokens.
We're in the process of adding YubiKey support and we'd like to be able to set touch and PIN policy through C_GenerateKeyPair. This used to be supported through the CKA_VENDOR_DEFINED attribute [0] but it was removed in [1]. I don't see any PR or explanation for its removal.
Would Yubico be receptive to a PR adding this functionality? If so, are there known, blocking issues with the old approach that would need to be addressed in the new PR? It appears that the old implementation lacked support for querying policy through C_GetAttributeValue, but that can be added (via ykpiv_get_metadata) relatively easily.
FWIW, I've implemented the basic functionality and confirmed that it works on a recent YubiKey 5.
Thanks in advance!
[0] https://github.com/Yubico/yubico-piv-tool/blob/yubico-piv-tool-1.7.0/ykcs11/token_vendors.c#L162
[1] fd2b585
We'd welcome a PR, preferably rebased on master of course. As you might notice there are quite a few PRs in flight at the moment, so it might take a little bit of time to get around to it, as well as the possibility that we might request another rebase after those are merged. But now is the time, a new release is not too far off into the future..
Sounds good and sorry for the delay.. The code is ready to go but I'm awaiting a couple of employer approvals before opening the PR. It should be up sometime next week at the latest, hopefully in time for your upcoming release.
How would you feel about adding the CKA_ALWAYS_AUTHENTICATE as an additional attribute for PIN policy ? It only has two possible values according to the standard. But we could support it as an alternative way of specifying YKPIV_PINPOLICY_ONCE or YKPIV_PINPOLICY_ALWAYS. Neither of the attributes would mean YKPIV_PINPOLICY_DEFAULT.
Yeah, I should have thought to wire that in. It's a simple addition.
Using a CKA_ALWAYS_AUTHENTICATE value of CK_FALSE to specify YKPIV_PINPOLICY_ONCE seems a bit counterintuitive, given that the PIV applet defines per-slot defaults and the PKCS#11 spec only discusses behavior when that attribute is CK_TRUE.
The alternative would be to only allow specifying a value of CK_TRUE (as YKPIV_PINPOLICY_ALWAYS). That said, PIN_ONCE is a safer configuration and from what I can tell this would only matter on slot 9e. I'll go that route as long as you're comfortable with the behavior.
I have merged the first PR now, thx for the contribution. I am working on keeping the policy values around in the slot info, like we do for the 'local' attribute already, from when the first session is opened on a slot. That logic uses attestation first, since we want the attestation certificate anyway, and also that is is available for older YubiKeys. If that fails it uses metadata, which will also be the case for imported keys. This way getting those attributes via C_GetAttributeValue will not need to touch the PIV applet.
#354 has now been merged to master. Let me know if you have any issues with how this works
I've run into an issue after rebasing my import patch. 31d1219 dropped a check in test_privkey_policy - specifically, after the application passes a policy of "default" for key generation, the C_GetAttributeValue return should reflect whatever default policy was applied by the token. It should not report "YKPIV_*POLICY_DEFAULT".
The new mechanism fails this check even though the attestation cert appears to contain the applied policy. The below diff will cause a failure for both touch and PIN:
diff --git a/ykcs11/tests/ykcs11_tests_util.c b/ykcs11/tests/ykcs11_tests_util.c
index 23bee96..8acf586 100644
--- a/ykcs11/tests/ykcs11_tests_util.c
+++ b/ykcs11/tests/ykcs11_tests_util.c
@@ -1506,9 +1506,19 @@ void test_privkey_policy(CK_FUNCTION_LIST_PTR funcs, CK_SESSION_HANDLE session,
else if (pin_attr_val == YKPIV_PINPOLICY_DEFAULT && always_auth_val)
pin_attr_val = YKPIV_PINPOLICY_ALWAYS;
- asrt(touch_pol, touch_attr_val, "TOUCH POLICY");
- asrt(pin_pol, pin_attr_val, "PIN POLICY");
asrt(always_auth, always_auth_val, "ALWAYS AUTH");
+
+ if (touch_attr_val != YKPIV_TOUCHPOLICY_DEFAULT) {
+ asrt(touch_attr_val, touch_pol, "TOUCH POLICY");
+ } else {
+ // Else the slot default should be reported, never YKPIV_*POLICY_DEFAULT.
+ asrt(touch_pol != YKPIV_TOUCHPOLICY_DEFAULT ? 1 : 0, 1, "DEFAULT TOUCH POLICY");
+ }
+ if (pin_attr_val != YKPIV_PINPOLICY_DEFAULT) {
+ asrt(pin_pol, pin_attr_val, "PIN POLICY");
+ } else {
+ asrt(pin_pol != YKPIV_PINPOLICY_DEFAULT ? 1 : 0, 1, "DEFAULT PIN POLICY");
+ }
}
void test_privkey_attributes_ec(CK_FUNCTION_LIST_PTR funcs, CK_SESSION_HANDLE session,
TEST START: test_generate_eccp256_with_policy()
.../yubico-piv-tool/ykcs11/tests/ykcs11_tests_util.c.1516: <DEFAULT TOUCH POLICY> check failed with value 0 (0x0), expected 1 (0x1)
Please let me know if this is expected.
It's expected but maybe not ideal - As it now only reads the attestation / metadata when you open the first session on a slot. I adjusted the tests accordingly. If you just implement the import like I did the key generation, i.e. setting the pin and touch policy values to the specified ones the existing tests (with my adjustments) should work.
We should probably create an attestation or fetch metadata when generating or importing keys as well, to keep that info up to date. But in the mean time, maybe you could just do as described in my previous note here.
OK, I have added code to parse the attestation upon key generation, and to get metadata when importing. So the tests fail again now, I just have to fix that and we should be fine. I still suggest you assign the specified policies on import (like I do in key generation) so that we have the best we can in case metadata is not supported by the device. Checking this in soon.
Thanks! See #357
Thx for the pull requests, nice work. I have now merged the second one. I will add guards around the tests for devices that don't support attestation (<4.30) and metadata (<5.30) in a separate PR.
And thank you for the reviews! We're glad to be able to use this functionality.