Skip to content

Commit

Permalink
Added "--with-pkcs11-module" configure option
Browse files Browse the repository at this point in the history
  • Loading branch information
@mtrojnar
mtrojnar committed Jan 16, 2016
1 parent 60aa098 commit 0d0436e
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 12 deletions.
9 changes: 6 additions & 3 deletions NEWS
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
NEWS for Engine PKCS#11 -- History of user visible changes

New in 0.2.1; unreleased;
* Fixed OpenSSL engines directory autoselection (Michał Trojnara).
* Fixed PIN-less access to public keys (Mouse).
* Require new libp11 0.3.1 for PKCS11_enumerate_public_keys().
* Added "--with-pkcs11-module" configure option; addresses #36
(Michał Trojnara)
* Fixed OpenSSL engines directory autoselection (Michał Trojnara)
* Fixed PIN-less access to public keys (Mouse)
* Require new libp11 0.3.1 for PKCS11_enumerate_public_keys()
(Michał Trojnara)

New in 0.2.0; 2015-10-09; Nikos Mavrogiannopoulos
* Added support for ECDSA when compiled with libp11 0.3.0
Expand Down
22 changes: 13 additions & 9 deletions configure.ac
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,12 @@ AC_ARG_WITH(
fi]
)

AC_ARG_WITH(
[pkcs11-module],
[AS_HELP_STRING([--with-pkcs11-module], [default PKCS11 module])],
[pkcs11_module="${withval}"],
[pkcs11_module="`$PKG_CONFIG --variable=proxy_module --silence-errors p11-kit-1`"])

dnl Checks for programs.
AC_PROG_CPP
AC_PROG_INSTALL
Expand Down Expand Up @@ -124,14 +130,6 @@ AC_CHECK_HEADERS([ \
locale.h getopt.h dlfcn.h utmp.h \
])

PKG_CHECK_MODULES(
[P11KIT],
[p11-kit-1],
[proxy_module="`$PKG_CONFIG --variable=proxy_module p11-kit-1`"
AC_DEFINE_UNQUOTED([DEFAULT_PKCS11_MODULE], "${proxy_module}", [p11-kit proxy])],
[]
)

PKG_CHECK_MODULES([LIBP11], [libp11 >= 0.3.1],, [AC_MSG_ERROR([libp11 >= 0.3.1 is required])])

saved_LIBS=$LIBS
Expand Down Expand Up @@ -259,7 +257,12 @@ fi
#done
#LDFLAGS="${saved_LDFLAGS}"


if test -n "${pkcs11_module}"; then
AC_DEFINE_UNQUOTED(
[DEFAULT_PKCS11_MODULE],
"${pkcs11_module}",
[PKCS11 module])
fi
AC_SUBST([enginesdir])
AC_SUBST([ENGINE_LINK])
AC_SUBST([ENGINE_PKCS11_VERSION_MAJOR])
Expand Down Expand Up @@ -303,6 +306,7 @@ Linker flags: ${LDFLAGS}
Libraries: ${LIBS}

enginesdir ${enginesdir}
DEFAULT_PKCS11_MODULE ${pkcs11_module}

LIBP11_CFLAGS: ${LIBP11_CFLAGS}
LIBP11_LIBS: ${LIBP11_LIBS}
Expand Down

10 comments on commit 0d0436e

@nmav
Copy link
Contributor

@nmav nmav commented on 0d0436e Jan 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the change is not equivalent. The p11-kit proxy module is not just a default module. It is a system wide module that wraps all the available in the system PKCS#11 modules. With the new option the p11-kit module is not used even if available. A user would have to know about it.

@mtrojnar
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the change is not equivalent.

I certainly hope so. The previous code attempted to create an unnecessary direct dependency on p11-kit, whereas the new code simply requests libp11 to load the p11-kit-proxy module.

The p11-kit proxy module is not just a default module. It is a system wide module that wraps all the available in the system PKCS#11 modules.

I fail to understand the contradiction in your statement. p11-kit-proxy is both "just a default module" and "a system wide module that wraps all the available in the system PKCS#11 modules". For libp11 and engine_pkcs11 p11-kit-proxy is just a PKCS#11 module. Neither libp11 nor engine_pkcs11 directly invoke any functionality specific to p11-kit.

With the new option the p11-kit module is not used even if available. A user would have to know about it.

Why do you think it is not used? Have you tested it? Could you share the results of your tests?

@nmav
Copy link
Contributor

@nmav nmav commented on 0d0436e Jan 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the change is not equivalent.
I certainly hope so. The previous code attempted to create an unnecessary direct dependency on p11-kit, whereas the new code simply requests libp11 to load the p11-kit-proxy module.

The long term goal for the engine was to rely only on p11-kit (without libp11) for safety and pkcs11 module discovery. May not make much sense though as currently it would require a rewrite of the engine.

With the new option the p11-kit module is not used even if available. A user would have to know about it.
Why do you think it is not used? Have you tested it? Could you share the results of your tests?

That was a mistake of mine. I only checked the code and didn't realize that you kept using p11-kit by default.

@mtrojnar
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May not make much sense though as currently it would require a rewrite of the engine.

Indeed. The rewrite would require duplicating most (>80%) of the libp11 functionality into engine_pkcs11. I'm not sure what you mean by "safety", but neither libp11 nor engine_pkcs11 do any PKCS#11 module discovery: this feature already solely relies on p11-kit.

@nmav
Copy link
Contributor

@nmav nmav commented on 0d0436e Jan 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sharing issue with PKCS#11 is that you cannot share a module in a process with multiple consumers (e.g., engine_pkcs11 through a library dependency and direct usage of PKCS#11).

http://p11-glue.freedesktop.org/doc/p11-kit/sharing.html#sharing-problem

@mtrojnar
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. This was a very useful read.

Technically, we could replace the libp11 (not engine_pkcs11, because engine_pkcs11 only invokes PKCS#11 via libp11) code for loading PKCS#11 modules with p11-kit API (http://cgit.freedesktop.org/p11-glue/p11-kit/tree/p11-kit/p11-kit.h). p11-kit would basically replace the functionality currently implemented in the src/libpkcs11.c file (4% of the libp11 code) with its own, significantly more advanced implementation.

On the other hand, there are important drawbacks of this approach:

  • Another build dependency: libp11 currently only requires OpenSSL.
  • Limited portability: AFAIK p11-kit only builds/works on Unix-based operating systems, so making it a dependency would also limit the platforms supported by libp11. Conditional compilation (supporting the current code for loading PKCS#11 modules and the p11-kit API) would break the KISS principle.
  • Limited flexibility: Some end-users prefer simplicity for their (possibly embedded) hardware, over the advanced features of p11-kit.
  • No apparent benefits: We already achieved identical (please correct me if I'm wrong about it) functionality with p11-kit-proxy.

@dwmw2
Copy link
Contributor

@dwmw2 dwmw2 commented on 0d0436e Jan 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to have a discussion along those lines, what I would actually like to do is kill libp11 and engine_pkcs11 completely. OpenSSL should support PKCS#11 as a first-class citizen. The APIs which currently take filenames for certificates and keys should accept PKCS#11 URIs seamlessly in place of those filenames.

One potential way that might be achieved is perhaps:

  • Obtain permission to relicence libp11 under the OpenSSL licence
  • Add libp11 under openssl/crypto/pkcs11
  • Extend the other APIs (especially SSL APIs) to use it as appropriate, for full integration

The relicensing might be a pain point; it might end up being better to reimplement, or to re-use code from something with a more appropriate licence like Alon's pkcs11-helper. But I really want to see OpenSSL supporting PKCS#11 natively so that it isn't a bolt-on extra that all applications forget to support.

@mtrojnar
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the questions are:

  1. Will all libp11 contributors (authors) agree to relicense their code from the LGPL license to the OpenSSL license?
  2. Can we relicense engine_pkcs11 from its 2-clause BSD license to the OpenSSL license without a permission from the authors?
  3. Will OpenSC team leaders agree to give engine_pkcs11 and libp11 away to OpenSSL?
  4. Will OpenSSL agree to accept this present (agree to further support this code)?

I expect problems with (1) and (4). The technical stuff seems to be quite simple compared to getting all the approvals.

@dwmw2: In case you decide to implement your proposal: I hereby agree to relicense my contributions to engine_pkcs11 and libp11 to the OpenSSL license.

@nmav
Copy link
Contributor

@nmav nmav commented on 0d0436e Jan 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another build dependency: libp11 currently only requires OpenSSL.

Well how useful is libp11 as a generic library? I'd simply consider it as an internal part of engine_pkcs11. If engine_pkcs11 is used with openssl there is no reason to use libp11.

Limited portability: AFAIK p11-kit only builds/works on Unix-based operating systems, so making it a dependency would also limit the platforms supported by libp11. Conditional compilation (supporting the current code for loading PKCS#11 modules and the p11-kit API) would break the KISS principle.

gnutls uses p11-kit and that works in windows as well. Granted p11-kit's configuration in windows is manual (there is no automatic discovery of modules there), but it works.

Limited flexibility: Some end-users prefer simplicity for their (possibly embedded) hardware, over the advanced features of p11-kit.

That could be true, but p11-kit is quite small comparing to openssl overall. Embedded systems are tricky, and whatever you do you may not please them. If space is an issue they would most likely skip libp11 and engine_pkcs11 completely and use plain PKCS#11 calls.

No apparent benefits: We already achieved identical (please correct me if I'm wrong about it) functionality with p11-kit-proxy.

There are few disadvantages of the proxy module; it bundles all the available modules in the system as one, and places them into different slots. It also requires real-time function generation which doesn't work well with some selinux environments.

dwmn2: If we're going to have a discussion along those lines, what I would actually like to do is kill libp11 and engine_pkcs11 completely. OpenSSL should support PKCS#11 as a first-class citizen. The APIs which currently take filenames for certificates and keys should accept PKCS#11 URIs seamlessly in place of those filenames.

I am for it, and we should plan for that, but should keep that as an ultimate goal. engine_pkcs11 has to be transformed step by step, so that it can be added in openssl, and then propose to include it, and then enable it transparently.

@mtrojnar
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well how useful is libp11 as a generic library?

Ubuntu 15.10 (via apt-rdepends -r libp11-2) indicates libp11 is also used by:

  • libdigidocpp0
  • libpam-p11
  • qdigidoc
  • qesteidutil

I guess there are also other products, not packaged in Ubuntu.

There are few disadvantages of the proxy module; it bundles all the available modules in the system as one, and places them into different slots.

How does it affect engine_pkcs11?

It also requires real-time function generation which doesn't work well with some selinux environments.

How critical is this issue?

Please sign in to comment.