Added OpenSSL server that supports TLS certificate status request #677

james-ctc · 2024-05-08T08:33:27Z

Describe your changes

feat: added optional OpenSSL server
feat: integration of OpenSSL into EvseV2G
feat: added cmake integration with optional OpenSSL
feat: added OpenSSL sha256 and ECDSA functions + ISO test vectors
feat: added and tested EXI signatures
feat: added base64 encode/decode
feat: OpenSSL or MbedTLS for EvseV2G

cmake -DEVEREST_CORE_BUILD_TESTING=ON -USING_MBED_TLS=OFF GNinja .. default is to use MBED TLS

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

SebaLukas · 2024-05-08T12:48:15Z

@FaHaGit Would be great if you could also take a look

barsnick

Nice PR! Valuable refactoring, and long-awaited switch to OpenSSL.

I didn't yet get a chance to review the complete PR, let alone test it. see the individual preliminary comments.

Open questions (possibly answered if I read properly):

Can OpenSSL also provide logging (and UDP-insertion) of CLIENT_RANDOM for Wireshark TLS decryption?
Can or should some of this be in libevse-security? D20Evse (branch testing/iso15118-20) could also use the same procedures. TLS and certificate handling is required is several EVerest modules - albeit as a server in EvseV2G/D20Evse, and as client in OCPP/OCPP201. Forks of the System module could also make use of it.

modules/EvseManager/tests/CMakeLists.txt

modules/EvseManager/tests/EvseManagerStub.hpp

modules/EvseV2G/tests/ISO15118_chargetImplStub.hpp

modules/EvseV2G/tests/evse_secrutyIntfStub.hpp

modules/EvseV2G/CMakeLists.txt

james-ctc · 2024-05-24T15:57:36Z

UDP with session key would be a good addition - the aim is to add this as a subsequent PR.
The aim at this point is to check that the OpenSSL refactor is a working option

james-ctc · 2024-05-24T16:02:23Z

Can or should some of this be in libevse-security?

Worthy of consideration - my take is to have something working in the first instance and then look to have common functionality in libevse-security. Aspects of libevse-security aren't abstracted to make unit testing easy especially since it is currently focused on being a TLS client rather than server.

config/config-sil-dc.yaml

SebaLukas

First batch 😅

modules/EvseV2G/CMakeLists.txt

modules/EvseV2G/EvseV2G.cpp

modules/EvseV2G/tests/tls_main.cpp

modules/EvseV2G/connection/tls_connection.cpp

SebaLukas

These changes could be separated and moved to a separate PR.

modules/EvseManager/tests/CMakeLists.txt

modules/EvseManager/tests/EvseManagerStub.hpp

tests/README.md

tests/include/ModuleAdapterStub.hpp

james-ctc · 2024-06-24T08:45:32Z

These changes could be separated and moved to a separate PR.
They can be moved but this PR will then depend on that one being approved.

SebaLukas · 2024-06-24T08:51:16Z

Yes, this is clear that there is a dependency. But the few changes in the extra PR can be merged more quickly than this large PR. I generally want to separate things so that small changes are not "hidden" in large PRs.

james-ctc · 2024-06-24T08:57:57Z

I'll move them out, but there can be no further updates to this PR until it is approved and merged as this one will no longer compile.
#739

james-ctc · 2024-06-27T13:52:51Z

rebase/merged against main with the new libcbv2g.
addressed many merge issues.

lib/staging/tls/tests/README.md

SebaLukas

Some comments

modules/EvseV2G/charger/ISO15118_chargerImpl.hpp

lib/staging/tls/tests/tls_main.cpp

lib/staging/tls/openssl-patch.md

lib/staging/tls/openssl_util.cpp

AssemblyJohn

For my part the openssl part looks ok, but with further possible improvements if we require to share more code (with the security lib), instead of duplicating it. In that way the code is also safer since we don't need to test for security vulnerabilities in two places, at least where's common functionality. The PR is very large, and I'd be against doing those modifications now.

SebaLukas

Last comment before testing PnC and debugging some tests with my setup

lib/staging/tls/tls.cpp

SebaLukas

Great job 👍

corneliusclaussen

I think we should make openssl the default setting when compiling. We need some more real world EV testing, but I'm afraid we will not get enough testing if it is not enabled by default. Since it is merged after the June release we have some time until the next stable release to fix potential issues

james-ctc · 2024-07-10T08:18:23Z

Fixed integration test failure.

item	note
underlying cause	Evse Security could not find certificates and key for ISO 15118 TLS server causing the module to exit
why was it passing previously	The original code didn't attempt SSL configuration until there was an incoming connection (which there isn't in any of the tests)
simple fix	provide certificate chain and key as part of the CI pipeline
actual fix	allow module to start with missing configuration. Missing SSL configuration requested when there is an incoming connection
Codacy	mostly false positives - not sure how to correctly tag that the issue has been investigated

feat: added optional OpenSSL server feat: integration of OpenSSL into EvseV2G feat: added cmake integration with optional OpenSSL feat: added OpenSSL sha256 and ECDSA functions + ISO test vectors feat: added and tested EXI signatures feat: added base64 encode/decode feat: OpenSSL or MbedTLS for EvseV2G cmake -DEVEREST_CORE_BUILD_TESTING=ON -DUSING_MBED_TLS=OFF -GNinja .. default is to use MBED TLS fix: clang format missmatch fix: missing dependencies fix: link issue fix: updated to support new error logging framework fix: updated to support new libevse-security intrefaces feat: remove use of mbedtls_base64_encode and mbedtls_base64_decode when building for OpenSSL OpenSSL base64 routines used when USING_MBED_TLS=OFF mbedTLS base64 routines used when USING_MBED_TLS=ON fix: remove duplicate include fix: removed typo in filename fix: SIL testing fixes fix: corrected typo in filename fix: add structure to EvseV2G use subdirectories to help collect common functionality and provide structure to the module. OpenSSL TLS moved to common area since it is not tied to EvseV2G. fix: addressing PR review comments fix: moved EvseManager changes to separate PR fix: rebase against main with new libcbv2g fix: remove TLS tests from CI - fail for unknown reason fix: Update README.md fix: corrected unit test executable names fix: updated code owners for the tls directory fix: removed debugging output to cout fix: added log handler to remove dependency on EVLOG fix: updates to try and get tests to run in CI fix: some codacy issues Signed-off-by: James Chapman <[email protected]> Adding missing <algorithm> Signed-off-by: Sebastian Lukas <[email protected]>

Signed-off-by: James Chapman <[email protected]>

Originally information was fetched from libevse-security every connection. The OpenSSL addition moved away from that so that configuration was checked at module start. The problem occurs when there isn't valid configuration. This change splits config into two sets: 1. config that must exist at module start 2. config that can be obtained at 1st connection TCP socket information is covered in 1. SSL certificates, keys and OCSP resonses are in 2. Signed-off-by: James Chapman <[email protected]>

james-ctc force-pushed the feat/v2g-optional-openssl branch 2 times, most recently from 9b04b45 to 910d42e Compare May 8, 2024 10:06

james-ctc marked this pull request as ready for review May 8, 2024 11:33

james-ctc requested review from corneliusclaussen, SebaLukas, Pietfried and hikinggrass as code owners May 8, 2024 11:33

SebaLukas self-assigned this May 8, 2024

SebaLukas requested a review from FaHaGit May 8, 2024 12:47

barsnick reviewed May 24, 2024

View reviewed changes

james-ctc force-pushed the feat/v2g-optional-openssl branch 3 times, most recently from 44c955e to d000f0a Compare June 6, 2024 07:28

james-ctc force-pushed the feat/v2g-optional-openssl branch 4 times, most recently from 90c7333 to c4c1366 Compare June 19, 2024 08:13

SebaLukas reviewed Jun 20, 2024

View reviewed changes

config/config-sil-dc.yaml Outdated Show resolved Hide resolved

james-ctc force-pushed the feat/v2g-optional-openssl branch 3 times, most recently from 63ac52b to d469080 Compare June 20, 2024 13:01

SebaLukas reviewed Jun 21, 2024

View reviewed changes

modules/EvseV2G/CMakeLists.txt Outdated Show resolved Hide resolved

modules/EvseV2G/EvseV2G.cpp Outdated Show resolved Hide resolved

modules/EvseV2G/tests/tls_main.cpp Outdated Show resolved Hide resolved

modules/EvseV2G/connection/tls_connection.cpp Outdated Show resolved Hide resolved

SebaLukas reviewed Jun 24, 2024

View reviewed changes

modules/EvseManager/tests/CMakeLists.txt Outdated Show resolved Hide resolved

modules/EvseManager/tests/EvseManagerStub.hpp Outdated Show resolved Hide resolved

tests/README.md Outdated Show resolved Hide resolved

tests/include/ModuleAdapterStub.hpp Outdated Show resolved Hide resolved

james-ctc mentioned this pull request Jun 24, 2024

feat: move test stub to common directory so other modules can use it #739

Merged

3 tasks

hikinggrass added the post-release Tag that this PR should not go into the current merge window for the upcoming release label Jun 25, 2024

james-ctc force-pushed the feat/v2g-optional-openssl branch from 4d1bad2 to 50ac7a7 Compare June 27, 2024 13:51

SebaLukas reviewed Jul 2, 2024

View reviewed changes

lib/staging/tls/tests/README.md Outdated Show resolved Hide resolved

SebaLukas reviewed Jul 2, 2024

View reviewed changes

modules/EvseV2G/charger/ISO15118_chargerImpl.hpp Show resolved Hide resolved

lib/staging/tls/tests/tls_main.cpp Show resolved Hide resolved

lib/staging/tls/tests/tls_main.cpp Show resolved Hide resolved

AssemblyJohn reviewed Jul 2, 2024

View reviewed changes

lib/staging/tls/openssl-patch.md Outdated Show resolved Hide resolved

AssemblyJohn reviewed Jul 2, 2024

View reviewed changes

lib/staging/tls/openssl_util.cpp Show resolved Hide resolved

AssemblyJohn approved these changes Jul 2, 2024

View reviewed changes

james-ctc force-pushed the feat/v2g-optional-openssl branch from 1582cec to f1cd4ca Compare July 4, 2024 08:41

SebaLukas reviewed Jul 8, 2024

View reviewed changes

lib/staging/tls/tls.cpp Outdated Show resolved Hide resolved

james-ctc force-pushed the feat/v2g-optional-openssl branch 2 times, most recently from 7cd6d8c to c9d2f70 Compare July 8, 2024 12:52

SebaLukas approved these changes Jul 9, 2024

View reviewed changes

james-ctc force-pushed the feat/v2g-optional-openssl branch from 3199c3b to 7c6298e Compare July 9, 2024 07:11

corneliusclaussen reviewed Jul 9, 2024

View reviewed changes

corneliusclaussen approved these changes Jul 9, 2024

View reviewed changes

james-ctc force-pushed the feat/v2g-optional-openssl branch 3 times, most recently from 7284fdb to 3c33600 Compare July 10, 2024 08:10

james-ctc requested review from SebaLukas and corneliusclaussen July 10, 2024 08:19

james-ctc force-pushed the feat/v2g-optional-openssl branch from 3c33600 to 58bb7e3 Compare July 10, 2024 08:57

james-ctc added 3 commits July 11, 2024 11:13

feat: make OpenSSL the default for 15118 communication with the PEV

e3faca4

Signed-off-by: James Chapman <[email protected]>

james-ctc force-pushed the feat/v2g-optional-openssl branch from 58bb7e3 to a7aa4d0 Compare July 11, 2024 10:13

james-ctc merged commit b2ea2fe into main Jul 11, 2024
7 of 8 checks passed

james-ctc deleted the feat/v2g-optional-openssl branch July 11, 2024 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added OpenSSL server that supports TLS certificate status request #677

Added OpenSSL server that supports TLS certificate status request #677

james-ctc commented May 8, 2024

SebaLukas commented May 8, 2024

barsnick left a comment

james-ctc commented May 24, 2024

james-ctc commented May 24, 2024 •

edited

Loading

SebaLukas left a comment

SebaLukas left a comment

james-ctc commented Jun 24, 2024

SebaLukas commented Jun 24, 2024 •

edited

Loading

james-ctc commented Jun 24, 2024 •

edited

Loading

james-ctc commented Jun 27, 2024

SebaLukas left a comment

AssemblyJohn left a comment •

edited

Loading

SebaLukas left a comment

SebaLukas left a comment

corneliusclaussen left a comment

james-ctc commented Jul 10, 2024

Added OpenSSL server that supports TLS certificate status request #677

Added OpenSSL server that supports TLS certificate status request #677

Conversation

james-ctc commented May 8, 2024

Describe your changes

Issue ticket number and link

Checklist before requesting a review

SebaLukas commented May 8, 2024

barsnick left a comment

Choose a reason for hiding this comment

james-ctc commented May 24, 2024

james-ctc commented May 24, 2024 • edited Loading

SebaLukas left a comment

Choose a reason for hiding this comment

SebaLukas left a comment

Choose a reason for hiding this comment

james-ctc commented Jun 24, 2024

SebaLukas commented Jun 24, 2024 • edited Loading

james-ctc commented Jun 24, 2024 • edited Loading

james-ctc commented Jun 27, 2024

SebaLukas left a comment

Choose a reason for hiding this comment

AssemblyJohn left a comment • edited Loading

Choose a reason for hiding this comment

SebaLukas left a comment

Choose a reason for hiding this comment

SebaLukas left a comment

Choose a reason for hiding this comment

corneliusclaussen left a comment

Choose a reason for hiding this comment

james-ctc commented Jul 10, 2024

james-ctc commented May 24, 2024 •

edited

Loading

SebaLukas commented Jun 24, 2024 •

edited

Loading

james-ctc commented Jun 24, 2024 •

edited

Loading

AssemblyJohn left a comment •

edited

Loading