Secret key support

[fabled]

This implements better Secret key support in OpenSC, and so far allows uploading secret keys on MyEID cards. Many of the first commits are ready for review and merging, but some comments have FIXME notes in the commit message. Consider this as requesting feedback on how to fix those issues properly.

[fabled]

@frankmorgner @viktorTarasov @dengert Care to take a look at this?

The CI fails due to the overloaded use store_key() in the last three commits, and it's marked on them with a FIXME. I would be interested to know if it'd be more appropriate to make a separate store_skey() or just change the current store_key()'s last argument to be void* or union that can contain either of the key types presentation.

[dengert]

Should _sc_card_add_generic be called something like _sc_card_add_secret?
"generic" is a type of secret key, but you are adding multiple types of secret keys.

You use the term secrkey in a few places and parameters. Would secretkey be a better name?

[fabled]

@dengert We could rename it to _sc_card_add_secret but it also accepts non-secret key algorithms. The point was to add an utility function that does not require any algorithm specific extra data.

For secrkey the commit explains: that user visible string is exposed in pkcs11-tool, and I used the same string in pkcs15-init to keep it harmonious. I have no strong opinion on what it should be.

[frankmorgner]

looks OK at first glance. Is there someone who can verify this functionality with myeid?

If you want to use _sc_card_add_generic, please do so only within card.c, but expose algorith specific functions (e.g. _sc_card_add_aes_alg) in card.h.

Regarding store_key, I'd suggest to use void * as parameter and to use the function for both, secret and private key import.

[fabled]

I modified sc_pkcs15_prkey to be able to contain an sc_pkcs15_skey. This allows existing store_key abstraction to be used nicely and type safely. If casting to void* is preferred, I can do that instead, but the cruft for changing all pkcs15init card drivers to accommodate that is rather large.

I'd rather not add separate _add_aes_alg and _add_des_alg functions that do exactly the same job. That's why I called it generic instead, because it can and will be used with multiple different symmetric cipher algorithms.

[dengert]

There is one part of OpenSC that depends on secret keys today, ECDH derive. With all the proposed changes to support secret key, do you have a card with EC keys to test ECDH?

It can be tested using pkcs11-tool if you have two cards each with a different EC key. Giving each card the public key of the other, both cards will derive the same secret key. Today the derived secret key is generic, and stored in memory. Part of your changes could also be to allow this key to be stored on the card.

I would hope you would test to make sure any of the proposed changes do not break the ECDH derive code.

[fabled]

With all the proposed changes to support secret key, do you have a card with EC keys to test ECDH?

Yes.

I would hope you would test to make sure any of the proposed changes do not break the ECDH derive code.

That code is isolated currently in pkcs11 module. It is not touched at all (except one mechanical variable type change), and it handles the ECDH derive in isolated way - the pkcs15 code is not involved in it. If and when I get to adding support for supporting on-board secret keys via pkcs11, this needs more careful attention. But I am aware of how it works, and have hardware to test it with.

[dengert]

In https://github.com/OpenSC/OpenSC/files/926015/1025-bad.log.txt Problem appears to be in after Line:11591 the session is then closed.
0x7fffc84873c0 12:07:30.450 [opensc-pkcs11] framework-pkcs15.c:3336:pkcs15_prkey_get_attribute: pkcs15_prkey_get_attribute() called
0x7fffc84873c0 12:07:30.450 [opensc-pkcs11] pkcs11-session.c:158:C_CloseSession: C_CloseSession(0x7fb1a65125b0)

In good line:11259

0x7fffc84873c0 12:12:47.231 [opensc-pkcs11] framework-pkcs15.c:3316:pkcs15_prkey_get_attribute: pkcs15_prkey_get_attribute() called
0x7fffc84873c0 12:12:47.231 [opensc-pkcs11] framework-pkcs15.c:3316:pkcs15_prkey_get_attribute: pkcs15_prkey_get_attribute() called

Then when the secret key in memory is being created:
0x7fffc84873c0 12:12:47.231 [opensc-pkcs11] pkcs11-object.c:99:sc_create_object_int: called

It looks like in framework-pkcs15.c, this is failing:

2063 /* TODO Only Session secret key objects are supported for now
2064 * Sesison objects have CKA_TOKEN=false
2065 * This is used by the C_DeriveKey with ECDH to hold the
2066 * key, and the calling application can then retrieve tha attributes as needed.
2067 * TODO If a card can support secret key objects on the card, this
2068 * code will need to be expanded.
2069 */
2070 static CK_RV
2071 pkcs15_create_secret_key(struct sc_pkcs11_slot *slot, struct sc_profile *profile,
2072 CK_ATTRIBUTE_PTR pTemplate, CK_ULONG ulCount,
2073 CK_OBJECT_HANDLE_PTR phObject)
2074 {

@fabled Have you looked at pkcs15_create_secret_key?

[mouse07410]

...Many of the first commits are ready for review and merging...

I don't think so.

This implements better Secret key support in OpenSC...

It broke ECDH, which I consider pretty fundamental/important for all the EC-capable cards, not just MyEID.

P.S. On a separate topic, it looks like the only thing CI tests is whether the submitted code compiles. While it is important - we're missing a crucial part: does this code actually work? I propose addition of self-tests that would use SoftHSMv2 as a software-based token emulator, to actually test whether the command-line tools do what they're supposed to. Without that we're risking catching serious problems too late down the pipe. I realize the risks of relying on somebody else's package for testing - but the current state is hardly better.

[fabled]

@mouse07410

Here's the log: 1025-bad.log.txt
And the good log: pre-1025-good.log.txt

The logs indicate that there was other changes in addition to this change PR (many of the logs line numbers that should match, do not match). Could describe the exact versions you tested? Did you do a full install of opensc? If the OpenSC pkcs11 module and the pkcs15 library in system do not match, things will break.

If you could bisect the offending commit, it would be even more helpful. I am suspicious because a) I am not touching PKCS#11 code that would break this, and b) I tested the PKCS#11 module with MyEID and EC Derive works (see below).

@dengert

Have you looked at pkcs15_create_secret_key [of PKCS#11]?

Yes. I am not touching it. This PR implements only PKCS#15 side improvements to maintain SKDF and to upload secret keys. It is potential future work to improve PKCS#11 module to support onboard secret key objects. The only change to PKCS#11 module was a mechanical change in commit e57b581. Additionally PKCS#11 module does not call to PCKS#15 code for the stub secret key object.

@mouse07410

...Many of the first commits are ready for review and merging...
I don't think so.

Why? I think they are ready for review. Yes, if something is broken, it needs to be merged before the PR in entirety is merged. However, if one late commit has a problem does not mean the first ones are bad.

This implements better Secret key support in OpenSC...
It broke ECDH, which I consider pretty fundamental/important for all the EC-capable cards, not just MyEID.

I am not fully convinced yet that this breaks it. You compare is not clean, but includes other changes too. When I tried to reproduce the issue with pkcs11-tool, MyEID EC derive works just fine even with the PR applied:

$ ./src/tools/pkcs11-tool --module ./src/pkcs11/.libs/opensc-pkcs11.so -vvv  --derive --login --id "fa151a1502df69549579f5bc3ca753786eabaaf1" --input-file ec-pub.der
Using slot 0 with a present token (0x0)
Logging in to "MyEID".
Please enter User PIN: 
Using derive algorithm 0x00001051 ECDH1-COFACTOR-DERIVE
��s��oೲVE���8g${Ã�p�����Ֆ���C��po���kg�H(�cr@x�cA~���ޘ��R��

P.S. On a separate topic, it looks like the only thing CI tests is whether the submitted code compiles. While it is important - we're missing a crucial part: does this code actually work? I propose addition of self-tests that would use SoftHSMv2 as a software-based token emulator, to actually test whether the command-line tools do what they're supposed to. Without that we're risking catching serious problems too late down the pipe. I realize the risks of relying on somebody else's package for testing - but the current state is hardly better.

Yes, agreed. Test suite could be improved. That is worth a separate issue / PR.

[dengert]

I have tested this pull request on Ubuntu using NIST demo card 4 and 15 with commands like this where the /tmp/derive.2580.other.pubkey.der is the pubkey of the other card:

pkcs11-tool --slot 0 --module /opt/ossl-1.0.2/lib/opensc-pkcs11.so -l --derive -m ECDH1-COFACTOR-DERIVE -O -d 03 -i /tmp/derive.2580.other.pubkey.der

The derive operations work and the two cards produce the same generic secret (Anyone else using the NIST demo cards 4 aND 15 should get the same result.):
Secret Key Object; Generic secret length 32
VALUE: 4a13df485f9c4c132c94cd0f1ef3a37b0316729e0b288cfe55a7d691eae754d1
label: ��
Usage: encrypt, decrypt, wrap, unwrap

@mouse07410 I don't see where the code has broken ECDH. There may be something else going on in your environment. As I said above, in you opensc-debug logs it looks like pkcs15_create_secret_key is failing, but there is not enough information in the logs.

You are using OpenSSL commands via libp11 and the engine. I am using pkcs11-tool.

You are using a Yubico, I am using NIST PIV demo cards. (With the keyUsage modes that allows Yubico devices to bypass NIST PIV policies, make sure the certificate for the key MAN key has keyUsage = keyAgreement when the key is ECC. Or has no keyUsage that would then default ti the NIST policies.)

SPY output might be helpful too.

The only other thing I can suggest is to use a debugger and step through pkcs15_create_secret_key.
I build OpenSSL, OpenSC, libp11 with the CFLAGS=-g and LDFLAGS=-g to get the debugging symbols. I use gcc and gdb on Ubuntu. I don't use MacOS, but would expect there is some compiler, loader and a debugger you could use.

[fabled]

@mouse07410

(it is weird that the pubkey has usage "None" instead of "derive", but hopefully the "new" OpenSC takes what's in the certificate in this case)

If the key has 'none' usage, it will cause derive operation to fail. The cause is probably of the same root cause. Please ensure that pkcs11 and pkcs15 libraries match -- that is that opensc is fully installed to system after applying the PR, and not just the other library.

I am genuinely interested on not breaking things. pkcs11-tool is used as it's the simplest way to test things, and triggers same code paths as openssl engine. But as we have two hardware items working, it suggest something specific to your environment or hardware. I will look at the logs you provided a bit later in detail. Thanks.

[dengert]

You said earlier: "(it is weird that the pubkey has usage "None" instead of "derive", but hopefully the "new" OpenSC takes what's in the certificate in this case)"

Today you said:
"For some reason with this PR, Usage is seen as "none" for both prkey and pubkey. WIthout it - only pubkey has Usage: none, and the prkey shows the correct Usage: derive."

In pkcs15-piv.c, the keyUsage is checked at lines 768 to 892, and for EC and keyAgreement lines 867 and 868. the derive chould have been set for both pub and private, and it does not make sense that private key has derive and public key has none.

The ckis[i].cert_keyUsage is an unsigned long long, and this could be a compiler problem.
if(ckis[i].cert_keyUsage & 0x10u) { /* keyAgreement */
but looks more like something else.

@mouse07410 I need to have you do some debugging.

[fabled]

@mouse07410

I suspect that 67ea96d is broken, at least on Mac.

That commit has nothing to do with this pull request. I would like to get clarification on which builds you are comparing? What is the commit id of the build that works? Can you confirm if the current git master works for you or not? If current opensc git master does not work for you, all this should go to a new / separate bug.

Please, use git bisect to determine which change breaks the usage for you.

[frankmorgner]

@mouse07410 for your feedback to be helpful, please make sure to

• checkout exactly this pull request
• install OpenSC locally instead of using the pkg (which drops the debug symbols), for example by using ./configure --prefix=/tmp/ && make install && env DYLD_LIBRARY_PATH=${DYLD_LIBRARY_PATH}:/tmp/lib /tmp/bin/pkcs11-tool
• Use lldb to find out where the problem is exactly (lldb has a ncurses based gui)
• Point to a specific change (in this PR) that you think needs work

If I read the above thread correctly, ECDH key derivation still works as expected for NIST's PIV test cards and the myeid card.

Yubikey's PIV ECDH key derivation seems to be broken due to a different (prior) commit. If so, you should indeed analyse this problem with git bisect in a seperate issue...

[mouse07410]

Situation worse than it seemed (#1029 revealed more problems than reported there). I will need to git bisect.

[dengert]

IMHO, @mouse07410 has a git problem, Mac problem configuration problem, compiler problem or changes he may have in his version of OpenSC are causing strange problems. Until #1029 is resolved, I am not looking at 67ea96d

[dengert]

Its not the "New English" See this comment in card-piv.c has:

2139 /*
2140 * If the object can not be present on the card, because the History
2141 * object is not present or the History object says its not present,
2142 * return 1. If object may be present return 0.
2143 * Cuts down on overhead, by not showing non existent objects to pkcs11
2144 * The path for the object is passed in and the first 2 bytes are used.
2145 * Note: If the History or Discovery object is not found the
2146 * PIV_OBJ_CACHE_NOT_PRESENT is set, as older cards do not have these.
2147 * pkcs15-piv.c calls this via cardctl.
2148 */

[dengert]

@mouse07410

You says things like: " I've added a ton of debug-logging there, and none of it appears in the log."
But you don't show what you added.
This sounds like you are linking/loading against the wrong libraries which don't have your changes. You could look the trace and see if the line number of all the entries in the trace match the line numbers in your modified code.

in https://github.com/OpenSC/OpenSC/files/931707/1025-bad.txt two of the highest pkcs15-piv.c trace entries are is:
pkcs15-piv.c:1167:sc_pkcs15emu_piv_init: USAGE: cert_keyUsage_present:1 usage:0x00000022
[opensc-pkcs11] pkcs15-piv.c:1174:sc_pkcs15emu_piv_init: returning with: 0 (Success)
Do these line number match?
They match the git version I have and my HEAD is at c4d4c42

We are talking about ECDH, but in the last output of pkcs11-tool -O -l all your certs are RSA!

You cant find out how to use OpenSC SPY on your own because you forgot? Google is your friend
Google for: OpenSC SPY

You have local mods in your git and can not build a clean copy. This is a requirement to do any testing.
Does the MacOS require a make depend?

You need to lean how to use a debugger and other tools like @frankmorgner pointed out.
Add gitk to that list too.

[mouse07410]

@fabled After #1030 was taken care of, I've tested your PR for ECDH key derivation, and it worked fine:

$ /tmp/OpenSC/bin/pkcs11-tool --module /tmp/OpenSC/lib/opensc-pkcs11.so -l --derive --id 03 -i ec256pub.der -o sec-derived.dat
Using slot 0 with a present token (0x0)
Logging in to "PIV Card Holder pin (PIV_II)".
Please enter User PIN: 
Using derive algorithm 0x00001051 ECDH1-COFACTOR-DERIVE
$ openssl pkeyutl -engine pkcs11 -peerform engine -derive -inkey ec256priv.pem -peerkey "pkcs11:manufacturer=piv_II;object=KEY%20MAN%20pubkey;type=public" -out sec-derived2.dat
engine "pkcs11" set.
$ cmp sec-derived.dat sec-derived2.dat
$ 

[fabled]

Rebased on top of current git master.
@hhonkanen Care to look at this for the MyEID parts as you seem to be working with it?

[frankmorgner]

please be more concise and move the problem about keyusage to an other thread.

[mouse07410]

please be more concise and move the problem about keyusage to an other thread.

Done. It's #1030

[hhonkanen]

I looked at the changes in card-myeid.c and pkcs15-myeid.c. Basically they look good and I have no objection against any of them, but I haven't tested the code yet. myeid_init is cleaner after your refactoring. What kind of testing have you already done? I guess the basic RSA and ECC functionality should be tested.

You removed key size validation from myeid_create_key, for example:

•   	if (sc_card_find_ec_alg(p15card->card, keybits, NULL) == NULL) 
  

•   		LOG_TEST_RET(ctx, SC_ERROR_INVALID_ARGUMENTS, "Unsupported EC key size"); 
  

Is it guaranteed that the key size has been checked elsewhere before calling this function?

[fabled]

You removed key size validation from myeid_create_key, for example:

Is it guaranteed that the key size has been checked elsewhere before calling this function?

Only pkcs15-lib.c calls ops->create_key(). In all use cases it uses check_keygen_params_consistency() and/or check_key_compatibility() to verify that the request is sane.

[fabled]

The only problem left is what I described earlier:

It seems that TokenInfo is not written after creating a secret key that adds the supportedAlgorithms.
what would be the way to signal sc_pkcs15init_unbind to update TokenInfo regardless of profile->pkcs15.do_last_update value when the TokenInfo contents are "dirty"?

Any suggestions how to do this?

After the TokenInfo on card is updated properly, this pull request should be ready for merge unless additional issues are found.

[frankmorgner]

Unfortunately, I cannot help with this point. Maybe @CardContact can look a this, since SC-HSM could also benefit from secret key support (think of sc-hsm's "device key encryption key").

[fabled]

Added the code to update TokenInfo structure when needed.

[frankmorgner]

Only the /* fall through */ comments are missing. Have you used valgrind to check for memory issues?

[fabled]

@frankmorgner Where would fallthrough comment be required?

To me the only place my patch adds is myeid_new_file() and myeid_store_key() but in there it's just grouping same action for multiple cases. It is standard across codebase to not add this comment in this case. The only placse current code base has fallthrough comment is in src/libopensc/aux-data.c and src/tools/pkcs15-tool.c where the first case does things and then falls through to second case's code.

[frankmorgner]

please add some documentation in doc/tools/pkcs15-init.1.xml for the new options

[frankmorgner]

coverity should not report these (http:https://security.coverity.com/blog/2013/Sep/gimme-a-break.html), no need to add comments.

[frankmorgner]

If you want to have the changes in the upcoming release, please hurry up adding the documentation, thanks!

[fabled]

I am busy this weekend, but looking at this on Monday. Hope it's good enough.

[frankmorgner]

OK, thanks!

[fabled]

Added documentation.

[frankmorgner]

thanks