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.

Sign up for GitHub

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

Jump to bottom

feat: add TPM2 support for EvseV2G TLS server private key #1021

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: add TPM2 support for EvseV2G TLS server private key #1021

wants to merge 4 commits into from

Conversation

james-ctc
Copy link
Contributor

Describe your changes

feat: add TPM2 support for EvseV2G TLS server private key
Note arbitrary sign/verify is not currently supported in the OpenSSL utility layer (openssl_util.cpp)

TPM2 support requires -DUSING_TPM2 to cmake

Issue ticket number and link

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • I read the contribution documentation and made sure that my changes meet its requirements

@james-ctc james-ctc force-pushed the feat/v2g-tpm-support branch from d23e00d to e926621 Compare January 22, 2025 16:14
SebaLukas
SebaLukas approved these changes Jan 23, 2025
View reviewed changes
Copy link
Contributor

@SebaLukas SebaLukas left a comment

Choose a reason for hiding this comment

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

Looks good 👍
Do we have some documentation or example where -DUSING_TPM2 is already used? If not, then we should make a note of how to activate TPM support.

lib/staging/tls/openssl_util.cpp Outdated Show resolved Hide resolved
lib/staging/tls/tls.cpp Show resolved Hide resolved
@james-ctc
Copy link
Contributor Author

Looks good 👍 Do we have some documentation or example where -DUSING_TPM2 is already used? If not, then we should make a note of how to activate TPM support.

-DUSING_TPM2 is a livevse-security flag. It should be already documented there.
It is only used here to include or exclude the TMP2 unit tests. (same as libocpp).

@james-ctc james-ctc force-pushed the feat/v2g-tpm-support branch from c307ee4 to d3be06f Compare January 23, 2025 09:11
lategoodbye
lategoodbye reviewed Jan 23, 2025
View reviewed changes
lib/staging/tls/tests/tls_connection_test.hpp
// ref1.certificate_chain_file = "alt_server_chain.pem";
// ref1.private_key_file = "alt_server_priv.pem";
// ref1.trust_anchor_file = "alt_server_root_cert.pem";
// ref1.ocsp_response_files = {"ocsp_response.der", "ocsp_response.der"};
Copy link

Choose a reason for hiding this comment

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

Is it intended to submit all these commented out stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link

Choose a reason for hiding this comment

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

Since this is not self-explaining, could you please add a comment about the intention of this?
Personally i don't think such dead code, because there is no CI coverage in case of refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is test code, not part of the deployed code.
Those lines will be uncommented when the tests that use them are added.
It saves having to remember what the changes are.

@james-ctc james-ctc force-pushed the feat/v2g-tpm-support branch from d3be06f to 85802c0 Compare January 24, 2025 14:06
@james-ctc
feat: add TPM2 support for EvseV2G TLS server private key
034ef32 
Note arbitrary sign/verify is not currently supported in the
OpenSSL utility layer (openssl_util.cpp)

TPM2 support requires -DUSING_TPM2 to cmake

Signed-off-by: James Chapman <james.chapman@pionix.de>
@james-ctc
fix: added comment to explain resetting the global provider settings
c3b451b 
Signed-off-by: James Chapman <james.chapman@pionix.de>
@james-ctc james-ctc force-pushed the feat/v2g-tpm-support branch from 85802c0 to c3b451b Compare February 3, 2025 13:05
@SebaLukas SebaLukas added the post-release Tag that this PR should not go into the current merge window for the upcoming release label Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lategoodbye lategoodbye lategoodbye left review comments

@SebaLukas SebaLukas SebaLukas approved these changes

@AssemblyJohn AssemblyJohn Awaiting requested review from AssemblyJohn AssemblyJohn is a code owner

@corneliusclaussen corneliusclaussen Awaiting requested review from corneliusclaussen corneliusclaussen is a code owner

Assignees

@SebaLukas SebaLukas

Labels
post-release Tag that this PR should not go into the current merge window for the upcoming release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@james-ctc @lategoodbye @AssemblyJohn @SebaLukas