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

Reliability improvements for OpenSSL tpm2 provider on an embedded system #463

Merged
merged 3 commits into from
Feb 16, 2024

Conversation

james-ctc
Copy link
Contributor

related to libevse-security PR42

Background

The existing implementation was occasionally failing especially when there was a migration from using non-TPM keys to supporting TPM protected keys (TSS2). Issues were observed during the loading and unloading of providers. There was also some concern that another call could impact which providers were loaded and their property strings.

There were significant improvements once the the tpm2-abrmd daemon was included and running. Generating keys, CSRs etc. didn't appear to need tpm2-abrmd however once TLS was being used tpm2-abrmd became essential.

It also became clear that the provider configuration depends on the key type. Hence the first line of the PEM key file is read so that the provider can be configured before actually loading the key.
Updates

Note: the tpm2-abrmd daemon needs to be running for tpm2 where TLS is being used.

Functionality change

The meaning of UseTPM has been clarified to support interworking and upgrades from non-TPM keys to TPM protected keys.
For TLS the provider chosen is based on the PEM file and not the UseTPM OCPP configuration variable.
UseTPM is used to determine how a new key is to be generated and not how an existing key is used.

Changes

The code has been updated to use the new OpenSSL provider implementation in libevse-security. It provides a suitable implementation for:

  • OpenSSL v3
    • TPM protected keys
    • non-TPM protected keys
  • OpenSSL v1
    • non-TPM protected keys

@AssemblyJohn
Copy link
Contributor

One note related to the OpenSSL provider. At the moment we seem to use a global OpenSSL context using the OpenSSLProvider. I am not sure what side effects this could have. Would it be better to re-create the openssl context upon each connection?

@james-ctc james-ctc force-pushed the openssl1-openssl3-and-tpm2 branch from 5837295 to c112411 Compare February 14, 2024 08:25
@james-ctc
Copy link
Contributor Author

… Would it be better to re-create the openssl context upon each connection?
Loading providers on demand was causing unreliability (in one instance there was a lockup while attempting to load a provider). The approach is based on loading providers as would be done in /etc/ssl/openssl.cfg; without needing to change the global configuration. Loading a provider is loading a DLL and running some initialisation and setup code. The SSL context for the connection doesn't change any of that, it just uses the loaded provider.

@AssemblyJohn
Copy link
Contributor

… Would it be better to re-create the openssl context upon each connection?
Loading providers on demand was causing unreliability (in one instance there was a lockup while attempting to load a provider). The approach is based on loading providers as would be done in /etc/ssl/openssl.cfg; without needing to change the global configuration. Loading a provider is loading a DLL and running some initialisation and setup code. The SSL context for the connection doesn't change any of that, it just uses the loaded provider.

I understand, probably that's why the old problems could arise, since loading/unloading the DLL can cause issues it seems. If there are checks so that concurrency problems do not arise (I've noticed the mutex) and also that there is no mis-calling of the wrong function (using the TPM instead of the normal openssl) then it's fine on my side.

fix: TPM & TLS working
feat: support OpenSSL versions 1 and 3
feat: support libevse-security TPM2 updates

Signed-off-by: James Chapman <[email protected]>
@james-ctc james-ctc force-pushed the openssl1-openssl3-and-tpm2 branch from c112411 to c709af2 Compare February 16, 2024 08:30
@james-ctc james-ctc self-assigned this Feb 16, 2024
Signed-off-by: James Chapman <[email protected]>
@james-ctc james-ctc marked this pull request as ready for review February 16, 2024 08:54
@james-ctc james-ctc requested review from corneliusclaussen and removed request for AssemblyJohn February 16, 2024 10:41
lib/ocpp/common/websocket/websocket_tls_tpm.cpp Outdated Show resolved Hide resolved
lib/ocpp/common/websocket/websocket_tls_tpm.cpp Outdated Show resolved Hide resolved
lib/ocpp/common/websocket/websocket_tls_tpm.cpp Outdated Show resolved Hide resolved
lib/ocpp/common/websocket/websocket_tls_tpm.cpp Outdated Show resolved Hide resolved
lib/ocpp/common/websocket/websocket_tls_tpm.cpp Outdated Show resolved Hide resolved
lib/ocpp/common/websocket/websocket_tls_tpm.cpp Outdated Show resolved Hide resolved
"Connecting with security profile 3 but no client side certificate is present or valid"));
}

path_chain = std::string(certificate_key_pair.value().certificate_path.c_str());
Copy link
Contributor

@AssemblyJohn AssemblyJohn Feb 16, 2024

Choose a reason for hiding this comment

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

There is a possible error here. The get_key_pair can return 2 values for a valid certificate, one is certificate_path and another is 'certificate_single_path'. if the certificate_path there should be a fallback to the 'certificate_single_path'.

Example:

// certificate_single_path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make the change.
However get_key_pair() could be updated so that this isn't required.
What if both certificate and certificate_single have values?
Is there a use case where certificate_single is needed on its own?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, there is a case with symlinks in the library. Because most of the times both will be present (chain & single) it is better to have this as both, and one can be empty. If both are non-null then the usage depends on the certificate setup.

Signed-off-by: James Chapman <[email protected]>
@james-ctc james-ctc merged commit df2400b into main Feb 16, 2024
4 checks passed
@james-ctc james-ctc deleted the openssl1-openssl3-and-tpm2 branch February 16, 2024 15:10
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