Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PROTON-2594: Add OpenSSL PKCS#11 PROVIDER support to enable HSM use #430

Closed
wants to merge 3 commits into from

Conversation

a3f
Copy link
Contributor

@a3f a3f commented Jul 10, 2024

Hardware Security Modules (HSMs) are physical computing devices that
safeguard secrets and allow performing cryptographic operations like
encryption and signing without necessarily divulging the private key
material.

PKCS#11 is a platform-independent API for cryptographic tokens like
HSMs. It defines a scheme for pkcs11: URIs that describe objects on the
token as well as an API to interact with them.

This commit adds support for transparent use of PKCS#11 URIs: Whenever a
certificate or private key path is prefixed with pkcs11: it will be
interpreted as PKCS#11 URI instead of a file path and OpenSSL will do
all necessary communication with the HSM behind the scenes.

For programs like proton that use OpenSSL, this could have been
realized in two ways:

  • OpenSSL ENGINE: Introduced in OpenSSL 0.9.6 and deprecated in 3.0
  • OpenSSL PROVIDER: Introduced in OpenSSL 3.0 to replace ENGINE

While both are supported in recent OpenSSL versions, it's more future
proof to use the PROVIDER API, even if this comes at the cost of having
to add an #ifdef to the code.

This has been tested on Linux/x86_64 with softhsm and Linux/arm32 with
OP-TEE, both with v0.5 of https://github.com/latchset/pkcs11-provider.

As everything is loaded dynamically, we do not link against any
PKCS#11-related shared libraries. It's expected that PKCS#11 provider
will already be loaded via OPENSSL_CONF if it's to be used.

@astitcher
Copy link
Member

This looks good to me - is there any way of testing this functionality in the CI job? (Rather than just testing that nothing regressed!)

@a3f
Copy link
Contributor Author

a3f commented Jul 12, 2024

is there any way of testing this functionality in the CI job? (Rather than just testing that nothing regressed!)

How about we apply this patch and then run the test twice once with PEM files and once with PKCS#11 or would you prefer I duplicate the relevant parts into a new test?

PKCS#11 test would use SoftHSM like done here, but I haven't attempted to do this in CI yet.

using namespace proton;

// Hack to write strings with embedded '"' and newlines
#define RAW_STRING(...) #__VA_ARGS__
Copy link
Member

Choose a reason for hiding this comment

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

We currently require C++17 or above and so can now use native C++11 raw strings.
I know you just copied this from a previous c++ test so it's not a required change, more a note to myself that we can improve the test code a bit now!

@a3f
Copy link
Contributor Author

a3f commented Oct 29, 2024

I added a pkc11_test that receives pkcs11: URIs for both server and client certificates and keys via environment variables. A script executed by CI adds the PEM files available in tree to a SoftHSM and exports said environment variables.

The test is skipped without being marked as failure whenever the environment variables are missing as not break other users running the test suite, but lacking the prerequisites.

This works fine locally, but unfortunately fails in CI and I am not sure why.

@astitcher How would you go about debugging issues that only fail in CI? I tried https://github.com/nektos/act, but I don't have a docker image that looks sufficiently enough like the Github runner VM images, so it's able to execute the tests as-is.

Copy link
Member

@astitcher astitcher left a comment

Choose a reason for hiding this comment

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

In general the tests in the cpp subdirectory are used to test the C++ API binding rather than the underlying proton-c functionality. This is because building the C++ binding is optional and so putting underlying proton-c testing here risks not testing fully.
It is true that the unit testing of proton-c does actually also use a C++ test harness but it is in the c subdirectory. If at all possible I'd really like to maintain this distinction for these tests too.
There are ssl tests there that I think you copy in the same way that you've copied a previous test for this test. It probably won't be as straightforward as this though - really sorry about that - we should probably add some more testing infrastructure to the C side to make it simpler.

@astitcher
Copy link
Member

I added a pkc11_test that receives pkcs11: URIs for both server and client certificates and keys via environment variables. A script executed by CI adds the PEM files available in tree to a SoftHSM and exports said environment variables.

The test is skipped without being marked as failure whenever the environment variables are missing as not break other users running the test suite, but lacking the prerequisites.

This works fine locally, but unfortunately fails in CI and I am not sure why.

@astitcher How would you go about debugging issues that only fail in CI? I tried https://github.com/nektos/act, but I don't have a docker image that looks sufficiently enough like the Github runner VM images, so it's able to execute the tests as-is.

I'm not aware of anything specific. I guess adding extra debugging output until you can figure out enough to make it fail in your local environment too. I know that's not very helpful - sorry :-(

@astitcher
Copy link
Member

Hmm, have you made sure that the CI image actually has the relevant openssl modules for the softhsm? Maybe you need to apt install something to make the test work in the CI envioronment.

@a3f
Copy link
Contributor Author

a3f commented Oct 30, 2024

Hmm, have you made sure that the CI image actually has the relevant openssl modules for the softhsm? Maybe you need to apt install something to make the test work in the CI envioronment.

Yes, that's what the apt install pkcs11-provider was for. I managed to reproduce in a ubuntu-24.04 container. The pkcs11-provider shipped there was the culprit. I now build the pkcs11-provider manually and the tests pass finally: https://github.com/a3f/qpid-proton/actions/runs/11591599162.

If at all possible I'd really like to maintain this distinction for these tests too.

I can understand that, but most users will likely not run the PKCS#11 tests anyway, because there are more prerequisites than the C++ compiler (Linux, softhsm2, pkcs11-provider, OpenSSL > 3.0).
Unfortunately, this side quest with debugging the CI has more than depleted the time I have alotted for this, so I won't be able to rewrite the test myself anytime soon.

@astitcher
Copy link
Member

Thanks for the work of adding the test.
Can you squash together the last 4 commits into 1 commit leaving just 3 commits overall please. I don't think those commits standalone really - they are just part of adding the test.

@astitcher
Copy link
Member

I've asked @cliffjansen to take a look at this. He has more experience with the openssl code.

@cliffjansen
Copy link
Contributor

This PR has some problems for which I have some proposed fixes. The main ones being compilation failures on older versions of OpenSSL and a memory leak.

If you can confirm my changes do not break anything in your environment with a real hardware security module, I will check this in and add a documentation update. My proposed changes are here:

https://github.com/cliffjansen/qpid-proton/tree/pn2594_wip

I will revert the github action back to ubuntu-latest once that advances to ubuntu-24.04 in a few weeks.

It is unfortunate the software emulation bits of PKCS11 support are sufficiently buggy/immature that you had to take extraordinary steps to build a custom package and configuration just to get the test to run. Hopefully the tests can be altered to use regular distro packages in time.

Hardware Security Modules (HSMs) are physical computing devices that
safeguard secrets and allow performing cryptographic operations like
encryption and signing without necessarily divulging the private key
material.

PKCS#11 is a platform-independent API for cryptographic tokens like
HSMs. It defines a scheme for pkcs11: URIs that describe objects on the
token as well as an API to interact with them.

This commit adds support for transparent use of PKCS#11 URIs: Whenever a
certificate or private key path is prefixed with pkcs11: it will be
interpreted as PKCS#11 URI instead of a file path and OpenSSL will do
all necessary communication with the HSM behind the scenes.

For programs like proton that use OpenSSL, this could have been
realized in two ways:

  - OpenSSL ENGINE:   Introduced in OpenSSL 0.9.6 and deprecated in 3.0
  - OpenSSL PROVIDER: Introduced in OpenSSL 3.0 to replace ENGINE

While both are supported in recent OpenSSL versions, it's more future
proof to use the PROVIDER API, even if this comes at the cost of having
to add an #ifdef to the code.

This has been tested on Linux/x86_64 with softhsm and Linux/arm32 with
OP-TEE, both with v0.5 of https://github.com/latchset/pkcs11-provider.

As everything is loaded dynamically, we do not link against any
PKCS#11-related shared libraries. It's expected that PKCS#11 provider
will already be loaded via OPENSSL_CONF if it's to be used.
…nto header

The test_handler implementation in connect_config_test can be useful for
further tests as well such as the incoming pkcs11_test.

We don't want to add the PKCS#11 test here, because the test is not
applicable to all platforms and it would overly complicate the existing
tests, so let's share code via a common header.
Existing tests hardcode paths to PEM files. For easily testing PKCS#11 usage
for client certificates on the target, we want to pass in dynamically
PKCS#11 URIs identifying the certificates and keys to use without
requiring recompilation.

Enable doing that by consulting a set of new environment variables:

	PKCS11_CLIENT_CERT: URI of client certificate
	PKCS11_CLIENT_KEY:  URI of client private key
	PKCS11_SERVER_CERT: URI of server certificate
	PKCS11_SERVER_KEY:  URI of server private key
	PKCS11_CA_CERT:     URI of CA certificate

These variables are populated and exported by sourcing the new
scripts/prep-pkcs11_test.sh script prior to executing the test.

The script uses SoftHSM, which is an implementation of a cryptographic store
accessible through a PKCS apache#11 interface without requiring an actual
Hardware Security Module (HSM).

We load into the SoftHSM both client and server keys and certificates.
As the server key exists only in encrypted form, we decrypt
server-private-key-lh.pem, so we need not handle passphrase input when
the PEM file is processed by pkcs11-tool.

When the script is not sourced, none of the environment variables will
be set and the test will be skipped without being marked as error.
@a3f
Copy link
Contributor Author

a3f commented Nov 14, 2024

This PR has some problems for which I have some proposed fixes. The main ones being compilation failures on older versions of OpenSSL and a memory leak.

Thanks and sorry for missing those. I now squashed your fixes directly into the commits.

If you can confirm my changes do not break anything in your environment with a real hardware security module, I will check this in and add a documentation update. My proposed changes are here:

Your changes look fine to me. Let me know if you have any further questions and feel free to push any commits you want to add on top of this PR. Let me k

I will revert the github action back to ubuntu-latest once that advances to ubuntu-24.04 in a few weeks.

Sounds good.

It is unfortunate the software emulation bits of PKCS11 support are sufficiently buggy/immature that you had to take extraordinary steps to build a custom package and configuration just to get the test to run. Hopefully the tests can be altered to use regular distro packages in time.

This is mostly because of the OpenSSL switch from ENGINE to PROVIDER. The PKCS#11 Engine is shipped universally, but is now deprecated. The provider is getting there.

Thanks again,

@cliffjansen
Copy link
Contributor

Now on main. Rebased to af0124e

@a3f
Copy link
Contributor Author

a3f commented Nov 15, 2024

Thanks a lot @astitcher & @cliffjansen!

@a3f a3f closed this Nov 15, 2024
@a3f a3f deleted the proton-2594-pkcs11 branch November 15, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants