Merge engine_pkcs11 into libp11 #50

mtrojnar · 2016-01-03T20:48:42Z

I hereby propose libp11 build to produce both libp11 and engine_pkcs11 libraries. If needed, building engine_pkcs11 could be made optional with a ./configure parameter.

The relationship between libp11 and engine_pkcs11 is similar to the relationship between libcrypto and libssl in OpenSSL. libp11 is usable without engine_pkcs11 (but not the other way around) just like libcrypto is usable without libssl (but not the other way around).

Merging engine_pkcs11 into libp11 would provide the following advantages:

libp11 testing scripts could use infrastructure provided by OpenSSL command-line tools via engine_pkcs11. Adding automated test cases to libp11 is essential to its further development.
Combined releases would automatically ensure compatibility between new features introduced in both libraries. It would no longer be required to detect libp11 version and features when building engine_pkcs11.
engine_pkcs11 would be easier to install for end-users. There would be simply less dependencies to download and compile.

I highly appreciate any comments, including any disadvantages I may have missed.

dengert · 2016-01-03T22:23:40Z

I agree. 

On 1/3/2016 2:48 PM, Michał Trojnara
  wrote:


  I hereby propose libp11 build to produce both libp11 and
    engine_pkcs11 libraries If needed, building engine_pkcs11 could
    be made optional with a /configure parameter
  The relationship between libp11 and engine_pkcs11 is similar to
    the relationship between libcrypto and libssl in OpenSSL libp11
    is usable without engine_pkcs11 (but not the other way around)
    just like libcrypto is usable without libssl (but not the other
    way around)
  Merging engine_pkcs11 into libp11 would provide the following
    advantages:

    libp11 testing scripts could use infrastructure provided by
      OpenSSL command-line tools via engine_pkcs11 Adding automated
      test cases to libp11 is essential to its further development
    Combined releases would automatically ensure compatibility
      between new features introduced in both libraries It would no
      longer be required to detect libp11 version and features when
      building engine_pkcs11
    engine_pkcs11 would be easier to install for end-users There
      would be simply less dependencies to download and compile

  I highly appreciate any comments, including any disadvantages I
    may have missed
  —
    Reply to this email directly or view it on
      GitHub.









--

Douglas E. Engert DEEngert@gmail.com

mouse07410 · 2016-01-04T00:47:26Z

I agree too. And if you could incorporate the engine_pkcs11 fixes for PIN-less access to public keys, and access to ECC keys - it would be outstanding.

Needless to say, I'm standing by, ready to test on PIV tokens. ;-)

nmav · 2016-01-05T12:31:17Z

That's a nice idea. However note that the license of both projects is different. That shouldn't be a problem in merging them - but should be a problem if engine_pkcs11 is to be included in openssl.

mtrojnar · 2016-01-05T13:11:40Z

Re-licensing engine_pkcs11 is indeed an important aspect of this change.

Including engine_pkcs11 without including libp11 would bring very little or no benefit to OpenSSL. I don't think the OpenSSL project has ever intended to include engine_pkcs11. They already have too much code to deal with rather than too little. 8-)

leobackes · 2016-01-20T13:11:59Z

Hi,

I use libp11 in a project of mine, but I don't use engine_pkcs11.
I see merging both projects can bring huge benefits to the overall usage, but please consider adding an option to enable/disable at build time.

mtrojnar · 2016-01-21T11:02:12Z

What would be the benefit of adding a ./configure option to disable building engine_pkcs11?

leobackes · 2016-01-21T12:09:35Z

It would reduce the build time, but maybe building the engine doesn't really takes that long at all. :)

mtrojnar · 2016-01-21T12:12:15Z

My point exactly...

mouse07410 · 2016-02-01T01:04:03Z

Currently out of four tests one fails and three succeed:

$ make check
Making check in src
Making check in doc
make[1]: Nothing to be done for `check'.
Making check in examples
make[1]: Nothing to be done for `check'.
Making check in tests
/Applications/Xcode.app/Contents/Developer/usr/bin/make  fork-test evp-sign \
      testpkcs11.softhsm testfork.softhsm testlistkeys.softhsm evp-sign.softhsm engines.cnf.in cert.der key.der pubkey.der
make[2]: `fork-test' is up to date.
make[2]: `evp-sign' is up to date.
make[2]: Nothing to be done for `testpkcs11.softhsm'.
make[2]: Nothing to be done for `testfork.softhsm'.
make[2]: Nothing to be done for `testlistkeys.softhsm'.
make[2]: Nothing to be done for `evp-sign.softhsm'.
make[2]: Nothing to be done for `engines.cnf.in'.
make[2]: Nothing to be done for `cert.der'.
make[2]: Nothing to be done for `key.der'.
make[2]: Nothing to be done for `pubkey.der'.
/Applications/Xcode.app/Contents/Developer/usr/bin/make  check-TESTS
PASS: testpkcs11.softhsm
PASS: testfork.softhsm
PASS: testlistkeys.softhsm
FAIL: evp-sign.softhsm
============================================================================
Testsuite summary for libp11 0.4.0_git
============================================================================
# TOTAL: 4
# PASS:  3
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0
============================================================================
See tests/test-suite.log
============================================================================
make[3]: *** [test-suite.log] Error 1
make[2]: *** [check-TESTS] Error 2
make[1]: *** [check-am] Error 2
make: *** [check-recursive] Error 1
$ cat tests/test-suite.log
============================================
   libp11 0.4.0_git: tests/test-suite.log
============================================

# TOTAL: 4
# PASS:  3
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: evp-sign.softhsm
======================

Current directory: /Users/uri/src/libp11/tests
Source directory: .
Output directory: output.16049
-n * Initializing smart card...
ok
Using slot 0 with a present token (0x0)
Using slot 0 with a present token (0x0)
Using slot 0 with a present token (0x0)
***************
Listing objects
***************
Using slot 0 with a present token (0x0)
Private Key Object; RSA
  label:      server-key
  ID:         00010203
  Usage:      decrypt, sign, unwrap
Public Key Object; RSA 2048 bits
  label:      server-key
  ID:         00010203
  Usage:      encrypt, verify, wrap
Certificate Object, type = X.509 cert
  label:      server-key
  ID:         00010203
At main.c:283:
- SSL error:FFFFFFFF8000B404:Vendor defined:PKCS11_rsa_verify:Not supported: p11_rsa.c:194
Basic PKCS #11 test, using ctrl failed
FAIL evp-sign.softhsm (exit status: 1)

But frankly, it concerns me less that the fact that ECDH is only half-done.

mouse07410 · 2016-02-01T15:35:54Z

Update
@mtrojnar It just hit me that the problem manifested here could be similar to the one with ECDH public key. The correct way to deal with asymmetric operations on a hardware (or simulated) token usually is:

request the token to perform private key operation, and
extract the public key to perform operation with it in software outside of the token.

It appears that here the code is attempting to perform a Verify operation (that requires public key) on the token itself. All the tokens I'm aware of implement/support only two operations: (a) sign, and (b) decrypt or derive (with the private key). Nothing else is supported. So here it seems that the code is trying to make the token to perform Verify, which the token (quite correctly) refuses.

Something similar appears to be happening with ECDH: before the patches the code was trying to extract the key form the token, which succeeded only for the public key. After @dengert patches, it seems to try to perform all the operations on the token, which succeeds only for the private key.

Does the above make sense to you?

dengert · 2016-02-01T16:40:29Z

That could be a problem. The engine_pkcs11 would get this correct, because the only key methods that are replaced are for private key oprations. OpenSSL will do everything else in software.

I have not looked at the tests, but if they are expecting libp11 to do all the public and private key operations that could the issue.

Is libp11 assuming that if the token can do a sign or derive, it can do a verify (CKF_VERIFY)
If it can not, then what does it do?

In regards to what a token can actually do, PKCS#11 has the CKF_HW attribute for mechanisms: "True if the mechanism is performed by the device; false if the mechanism is performed in software." OpenSC PKCS#11 is fairly good about this, but depends on the card driver settings.

Does libp11 look for CKF_HW? What does it do if the mechanism is not HW, or not available?
Mozilla's NSS can handle multiple PKCS#11 modules, and can select a module that can do what it needs.

mtrojnar · 2016-02-01T18:13:08Z

We should not override the rsa_verify method. This is what other OpenSSL engines do.

It does not make sense to implement public key operations on the engine.

mouse07410 · 2016-02-01T19:00:21Z

The engine_pkcs11 would get this correct, because the only key methods that are
replaced are for private key oprations. OpenSSL will do everything else in software.

But somehow this doesn't seem to happen with EC DERIVE (with the patches that make private key-based on-the-token derivation enabled and working). @dengert, your help here would be appreciated.

mouse07410 · 2016-02-01T21:43:19Z

Re. 4e35780. operation still fails, with a different error report.

$ make check
Making check in src
Making check in doc
make[1]: Nothing to be done for `check'.
Making check in examples
make[1]: Nothing to be done for `check'.
Making check in tests
/Applications/Xcode.app/Contents/Developer/usr/bin/make  fork-test evp-sign \
      testpkcs11.softhsm testfork.softhsm testlistkeys.softhsm evp-sign.softhsm engines.cnf.in cert.der key.der pubkey.der
make[2]: `fork-test' is up to date.
make[2]: `evp-sign' is up to date.
make[2]: Nothing to be done for `testpkcs11.softhsm'.
make[2]: Nothing to be done for `testfork.softhsm'.
make[2]: Nothing to be done for `testlistkeys.softhsm'.
make[2]: Nothing to be done for `evp-sign.softhsm'.
make[2]: Nothing to be done for `engines.cnf.in'.
make[2]: Nothing to be done for `cert.der'.
make[2]: Nothing to be done for `key.der'.
make[2]: Nothing to be done for `pubkey.der'.
/Applications/Xcode.app/Contents/Developer/usr/bin/make  check-TESTS
PASS: testpkcs11.softhsm
PASS: testfork.softhsm
PASS: testlistkeys.softhsm
FAIL: evp-sign.softhsm
============================================================================
Testsuite summary for libp11 0.4.0_git
============================================================================
# TOTAL: 4
# PASS:  3
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0
============================================================================
See tests/test-suite.log
============================================================================
make[3]: *** [test-suite.log] Error 1
make[2]: *** [check-TESTS] Error 2
make[1]: *** [check-am] Error 2
make: *** [check-recursive] Error 1
$ cat tests/test-suite.log 
============================================
   libp11 0.4.0_git: tests/test-suite.log
============================================

# TOTAL: 4
# PASS:  3
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: evp-sign.softhsm
======================

Current directory: /Users/ur20980/src/libp11/tests
Source directory: .
Output directory: output.24244
-n * Initializing smart card... 
ok
Using slot 0 with a present token (0x0)
Using slot 0 with a present token (0x0)
Using slot 0 with a present token (0x0)
***************
Listing objects
***************
Using slot 0 with a present token (0x0)
Private Key Object; RSA 
  label:      server-key
  ID:         00010203
  Usage:      decrypt, sign, unwrap
Public Key Object; RSA 2048 bits
  label:      server-key
  ID:         00010203
  Usage:      encrypt, verify, wrap
Certificate Object, type = X.509 cert
  label:      server-key
  ID:         00010203
./evp-sign.softhsm: line 31: 24256 Segmentation fault: 11  ./evp-sign ctrl false "${outdir}/engines.cnf" ${PRIVATE_KEY} ${PUBLIC_KEY} ${ADDITIONAL_PARAM}
Basic PKCS #11 test, using ctrl failed
FAIL evp-sign.softhsm (exit status: 1)

$

mtrojnar · 2016-02-01T22:18:38Z

I checked pkcs11_rsa_verify() against the OpenSSL source back to version 0.9.7. Most of this function is just a hack to fix the problem created by introducing this function in the first place. pkcs11_rsa_verify() also fails unless the RSA_FLAG_SIGN_VER flag is set, which apparently causes the problem reported by @mouse07410. I have no idea why it could possibly be useful.

Maybe (unlikely) it made sense with even more obsolete versions of OpenSSL, but it doesn't seem to make sense nowadays. I simply removed the code: 4e35780

mtrojnar · 2016-02-01T22:21:26Z

@mouse07410 Most likely you need opendnssec/SoftHSMv2#184

mouse07410 · 2016-02-01T22:31:41Z

@mouse07410 Most likely you need opendnssec/SoftHSMv2#184

Bingo!

$ make check
/Applications/Xcode.app/Contents/Developer/usr/bin/make  fork-test evp-sign \
      testpkcs11.softhsm testfork.softhsm testlistkeys.softhsm evp-sign.softhsm engines.cnf.in cert.der key.der pubkey.der
make[1]: `fork-test' is up to date.
make[1]: `evp-sign' is up to date.
make[1]: Nothing to be done for `testpkcs11.softhsm'.
make[1]: Nothing to be done for `testfork.softhsm'.
make[1]: Nothing to be done for `testlistkeys.softhsm'.
make[1]: Nothing to be done for `evp-sign.softhsm'.
make[1]: Nothing to be done for `engines.cnf.in'.
make[1]: Nothing to be done for `cert.der'.
make[1]: Nothing to be done for `key.der'.
make[1]: Nothing to be done for `pubkey.der'.
/Applications/Xcode.app/Contents/Developer/usr/bin/make  check-TESTS
PASS: testpkcs11.softhsm
PASS: testfork.softhsm
PASS: testlistkeys.softhsm
PASS: evp-sign.softhsm
============================================================================
Testsuite summary for libp11 0.4.0_git
============================================================================
# TOTAL: 4
# PASS:  4
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================
$

Now we can again concentrate on why ECDH-DERIVE doesn't work with both private and public keys. ;)

dengert · 2016-02-01T23:09:19Z

ECDH-DERIVE is always a private key operation. But It also takes as a parameter the public key (the EC_POINT) of the other party.

So both parties do an ECDH-DERIVE using their private key, with the input of the other parties EC_POINT.

The application may be creating an ephemeral ECDH key in software, so only one the private keys is on a token.

NIST 800-56A goes in to all the possible ways to use ECDH. Much of this is also done by the openssl cms command.

mouse07410 · 2016-02-01T23:25:13Z

@dengert, you're absolutely correct (as usually :). The issue is whether the private key in question is on the token, or in a file (aka software-loadable).

An application (or a user) may want to:

a. Derive ECDH key on the token - which did not work until you fixed it, thanks! or

b. Derive ECDH key from the ephemeral private key it received or generated and the public key on the token.

Here's the example of (a) (which I'm very grateful for fixing!):

$ openssl pkeyutl -engine pkcs11 -keyform engine -derive -inkey id_03 -peerform DER -peerkey ephempub.der -hexdump
engine "pkcs11" set.
PKCS#11 token PIN: 
0000 - 22 d8 ad 9f aa 8f f9 62-c0 6e 63 a4 8b 3c 64 ca   "......b.nc..<d.
0010 - 60 fe bb 80 d1 99 63 5e-a9 60 87 12 e2 0f 40 eb   `.....c^.`....@.
0020 - 8f a6 cc 56 31 83 1b c8-d4 e5 45 6b cd 2a fd 89   ...V1.....Ek.*..
$

Here's the example of (b) (which got broken, probably because currently the code can't distinguish between where the private key for this operation is):

$ openssl pkeyutl -engine pkcs11 -peerform engine -derive -inkey ephempriv.pem -peerkey id_03 -hexdump
engine "pkcs11" set.
Public Key operation error
$

The problem is - once the token gets involved/mentioned, the code automatically assumes that that's where the private key is going to be. Or at least tries to perform the operation as if that's where the private key was.

With RSA, it does not make this mistake: when Decrypt is requested it goes to the token, but I can request Encrypt - in which case it just takes the public key from the token and uses it in software.

@dengert, what is your opinion regarding how to address the above? Do we need two DERIVE methods instead of one? Do we need to do something different within the method if it's -peerform engine as opposed to -keyform engine? I suspect that we should avoid setting up ecdh_method when -peerform engine, but not sure how to change the code to accomplish it.

mtrojnar · 2016-02-03T16:16:16Z

086c06b concludes this issue.

mouse07410 · 2016-02-07T20:34:00Z

@mtrojnar there are a few questions related to this merge that I can't figure the answer to.

As this issue is closed, the repo OpenSC/engine_pkcs11 is obsolete and should not be used any more?
@dengert created a set of patches that enable ECDH for OpenSSL-1.0.2. Are you planning to integrate them yourself into the master? In that case I recommend making them conditional not on -DBUILD_ECDH_102, but simply on OpenSSL version being equal to 1.0.2.
Since so far OpenSSL Dev Team hasn't been keen exposing the necessary interfaces, I recommend adding ech_locl.h file to the libp11, and include it if and only if OpenSSL version is 1.0.2.

Comments please?

dengert · 2016-02-07T21:06:48Z

As for 2 and 3. There maybe copyright reasons not to copy the
OpenSSL internal header file into the OpenSC distribution.
OpenSSL wanted the file  ech_locl.h to be internal, copying it to
libp11 violates their intentions. 

libp11 is built by many distros and anything that looks like a
copyright problem or problem with the intentions of one package vs
another 
could cause problems. 

That is why I did not copy it, but left it up to the user to provide
the header file, leaving it up to the local user or site to decide
if it was OK to use the internal header file. 

@mtrojnar its up to you.

On 2/7/2016 2:34 PM, Mouse wrote:


  @mtrojnar there are a few questions
    related to this merge that I can't figure the answer to.


      As this issue is closed, the repo OpenSC/engine_pkcs11
        is obsolete and should not be used any more?


      @dengert created a set of patches
        that enable ECDH for OpenSSL-1.0.2. Are you planning to
        integrate them yourself into the master? In that case I
        recommend making them conditional not on -DBUILD_ECDH_102,
        but simply on OpenSSL version being equal to 1.0.2.


      Since so far OpenSSL Dev Team hasn't been keen exposing the
        necessary interfaces, I recommend adding ech_locl.h
        file to the libp11, and include it if and only
        if OpenSSL version is 1.0.2.


  Comments please?
  —
    Reply to this email directly or view
      it on GitHub.









--

Douglas E. Engert DEEngert@gmail.com

mtrojnar · 2016-02-07T22:17:02Z

Ad 1. At least not until libp11 0.4.0 is released (in a few weeks). The old https://github.com/OpenSC/engine_pkcs11 repository is still useful with the https://github.com/OpenSC/libp11/tree/libp11-0_3_x branch.

Ad 2. I have already answered this question in the proper thread: #49 (comment)

Ad 3. I still haven't decided about the the details. I'm working on a solution right now. I am fully aware of the licensing issues. Either including or requiring the original header (or parts of it) are not the only ways to achieve binary compatibility with an existing data structure.

mouse07410 · 2016-02-07T22:22:50Z

[mtrojnar] Ad 2. I have already answered this question in the proper thread: #49 (comment)

OK, according to #49 you are going to integrate ECDH-DERIVE (after you finish the cleanup). Great, thanks!

[dengert] There maybe copyright reasons not to copy the OpenSSL internal header file into
the OpenSC distribution.

Below is the copyright statement from ech_locl.h. I see nothing there even remotely suggesting the concerns you mentioned. To me it essentially says "you may use it or derive from it, as long as you acknowledge authorship, retain the acknowledgment stated below, and don't claim to be OpenSSL".

/* ====================================================================
 * Copyright (c) 2000-2005 The OpenSSL Project.  All rights reserved.
 *
 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions
 * are met:
 *
 * 1. Redistributions of source code must retain the above copyright
 *    notice, this list of conditions and the following disclaimer.
 *
 * 2. Redistributions in binary form must reproduce the above copyright
 *    notice, this list of conditions and the following disclaimer in
 *    the documentation and/or other materials provided with the
 *    distribution.
 *
 * 3. All advertising materials mentioning features or use of this
 *    software must display the following acknowledgment:
 *    "This product includes software developed by the OpenSSL Project
 *    for use in the OpenSSL Toolkit. (http://www.OpenSSL.org/)"
 *
 * 4. The names "OpenSSL Toolkit" and "OpenSSL Project" must not be used to
 *    endorse or promote products derived from this software without
 *    prior written permission. For written permission, please contact
 *    licensing@OpenSSL.org.
 *
 * 5. Products derived from this software may not be called "OpenSSL"
 *    nor may "OpenSSL" appear in their names without prior written
 *    permission of the OpenSSL Project.
 *
 * 6. Redistributions of any form whatsoever must retain the following
 *    acknowledgment:
 *    "This product includes software developed by the OpenSSL Project
 *    for use in the OpenSSL Toolkit (http://www.OpenSSL.org/)"
 *
 * THIS SOFTWARE IS PROVIDED BY THE OpenSSL PROJECT ``AS IS'' AND ANY
 * EXPRESSED OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
 * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
 * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE OpenSSL PROJECT OR
 * ITS CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
 * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
 * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
 * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
 * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
 * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
 * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
 * OF THE POSSIBILITY OF SUCH DAMAGE.
 * ====================================================================
 *
 * This product includes cryptographic software written by Eric Young
 * (eay@cryptsoft.com).  This product includes software written by Tim
 * Hudson (tjh@cryptsoft.com).
 *
 */

OpenSSL wanted the file ech_locl.h to be internal, copying it to libp11 violates their intentions.

If that was their intention - they did not state it in writing. Perhaps they did not intend for somebody to actually have a working ECDH with a token? :)
In any case, this doesn't seems to be something to be concerned of.

mtrojnar added the enhancement label Jan 3, 2016

mtrojnar self-assigned this Jan 3, 2016

mtrojnar added this to the 0.4.x milestone Jan 3, 2016

mtrojnar mentioned this issue Jan 15, 2016

Support OpenSSL 1.1 #52

Closed

mtrojnar added a commit that referenced this issue Feb 1, 2016

Stop overriding the rsa_verify method; see #50

4e35780

It does not make sense to implement public key operations on the engine.

mtrojnar closed this as completed Feb 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge engine_pkcs11 into libp11 #50

Merge engine_pkcs11 into libp11 #50

mtrojnar commented Jan 3, 2016

dengert commented Jan 3, 2016

mouse07410 commented Jan 4, 2016

nmav commented Jan 5, 2016

mtrojnar commented Jan 5, 2016

leobackes commented Jan 20, 2016

mtrojnar commented Jan 21, 2016

leobackes commented Jan 21, 2016

mtrojnar commented Jan 21, 2016

mouse07410 commented Feb 1, 2016

mouse07410 commented Feb 1, 2016

dengert commented Feb 1, 2016

mtrojnar commented Feb 1, 2016

mouse07410 commented Feb 1, 2016

mouse07410 commented Feb 1, 2016

mtrojnar commented Feb 1, 2016

mtrojnar commented Feb 1, 2016

mouse07410 commented Feb 1, 2016

dengert commented Feb 1, 2016

mouse07410 commented Feb 1, 2016

mtrojnar commented Feb 3, 2016

mouse07410 commented Feb 7, 2016

dengert commented Feb 7, 2016

mtrojnar commented Feb 7, 2016

mouse07410 commented Feb 7, 2016