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

Feature/get ocsp cache data #64

Closed
wants to merge 9 commits into from
Closed

Conversation

AssemblyJohn
Copy link
Collaborator

@AssemblyJohn AssemblyJohn commented Apr 12, 2024

Describe your changes

  • Fixed a certificate hierarchy potential issue when creating the OCSP cache
  • Added possibility to retrieve the OCSP cache
  • Added base64 utilities

Issue ticket number and link

EVerest/libocpp#596

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

@Pietfried Pietfried self-assigned this Apr 16, 2024
@AssemblyJohn AssemblyJohn force-pushed the feature/get_ocsp_cache_data branch from 0d3d2b8 to da844ca Compare April 16, 2024 09:38
Copy link
Contributor

@Pietfried Pietfried left a comment

Choose a reason for hiding this comment

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

lgtm except for the OCSP update and retrieval.

My idea would be:
In update_ocsp_cache we can simply choose a random file name for the OCSP response and store it in some directory that is dedicated for OCSP responses.

For retrieve_ocsp_cache we should not search for the ocsp response by filename, but rather iterate over the files in the OCSP directory, parse the OCSP data and check if the certificate hash data matches.

What do you think?

lib/evse_security/evse_security.cpp Outdated Show resolved Hide resolved
@AssemblyJohn
Copy link
Collaborator Author

lgtm except for the OCSP update and retrieval.

My idea would be: In update_ocsp_cache we can simply choose a random file name for the OCSP response and store it in some directory that is dedicated for OCSP responses.

For retrieve_ocsp_cache we should not search for the ocsp response by filename, but rather iterate over the files in the OCSP directory, parse the OCSP data and check if the certificate hash data matches.

What do you think?

I would require some hint in extracting the certificate hash data from the OCSP response. How is that possible? Also it will incur a certain performance cost, since it will imply parsing of all the data all the time, however it seems more stable than the current version.

@Pietfried
Copy link
Contributor

lgtm except for the OCSP update and retrieval.
My idea would be: In update_ocsp_cache we can simply choose a random file name for the OCSP response and store it in some directory that is dedicated for OCSP responses.
For retrieve_ocsp_cache we should not search for the ocsp response by filename, but rather iterate over the files in the OCSP directory, parse the OCSP data and check if the certificate hash data matches.
What do you think?

I would require some hint in extracting the certificate hash data from the OCSP response. How is that possible? Also it will incur a certain performance cost, since it will imply parsing of all the data all the time, however it seems more stable than the current version.

I was hoping openssl provides functionality to decode and parse the certificate hash data from the DER encoded ocsp response, but I dont know if that is the case.

It would be interesting to know how the OCSP response is loaded during the TLS handshake, because this is our targeted use case. Maybe @james-ctc already has some insights about this?

@AssemblyJohn
Copy link
Collaborator Author

Implemented header/interface refactor, request feedback before proceeding.

@AssemblyJohn
Copy link
Collaborator Author

AssemblyJohn commented Apr 22, 2024

There's also a requirement to integrate this with the garbage collection:

  • delete ocsp data for expired/garbage collected certificates
  • when ocsp data is updated if we already contain that OCSP hash, just over-write the data instead to write it again with a new random filename

@AssemblyJohn AssemblyJohn force-pushed the feature/get_ocsp_cache_data branch 2 times, most recently from 7a9cf64 to 8b69ebb Compare April 24, 2024 09:51
@AssemblyJohn AssemblyJohn marked this pull request as draft April 24, 2024 09:52
@AssemblyJohn AssemblyJohn force-pushed the feature/get_ocsp_cache_data branch 6 times, most recently from ae5b642 to 4104017 Compare April 26, 2024 11:54
@AssemblyJohn
Copy link
Collaborator Author

Comments have been implemented, OCSP relevant test has been implemented.

One more test update is required, for garbage collection related to deleted certificates.

@AssemblyJohn AssemblyJohn force-pushed the feature/get_ocsp_cache_data branch 2 times, most recently from 0ecb727 to 77d21e4 Compare April 29, 2024 10:08
@AssemblyJohn AssemblyJohn marked this pull request as ready for review April 29, 2024 10:08
@AssemblyJohn
Copy link
Collaborator Author

Relevant issues and comments have been solved.

@AssemblyJohn AssemblyJohn force-pushed the feature/get_ocsp_cache_data branch 2 times, most recently from add1dc1 to 57ba46d Compare April 29, 2024 10:16
- Refactored get_key_pair for extra info

Signed-off-by: AssemblyJohn <[email protected]>
Signed-off-by: AssemblyJohn <[email protected]>
Fixed code for tests

Signed-off-by: AssemblyJohn <[email protected]>
@AssemblyJohn AssemblyJohn force-pushed the feature/get_ocsp_cache_data branch from d6e71cf to 5d5a545 Compare May 1, 2024 07:42
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.

2 participants