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

Support hashing a certificate directory #96

Merged
merged 7 commits into from
Nov 26, 2024

Conversation

jannejy
Copy link
Contributor

@jannejy jannejy commented Oct 30, 2024

Describe your changes

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

@jannejy
Copy link
Contributor Author

jannejy commented Oct 31, 2024

PR in libocpp: EVerest/libocpp#852

@AssemblyJohn
Copy link
Collaborator

This doesn't look good to me. The behavior has been changed to only return the directory in which the bundle of roots is located. This will break all existing usage that depend on the file that contains the latest valid root, since not everyone is using directories.

If one requires the directory in which the bundle is located, it can be extracted from the returned path, for example:
std::filesystem::path directory_of_bundle = std::filesystem::path(certificate_single).parent_path();

Modifying code from existing functionality is never advised.

However, if a separate function that is clear and returns the location instead the bundle file is required, then it's better to create a new one in order to preserve backwards compatibility.

@jannejy
Copy link
Contributor Author

jannejy commented Nov 18, 2024

This doesn't look good to me. The behavior has been changed to only return the directory in which the bundle of roots is located. This will break all existing usage that depend on the file that contains the latest valid root, since not everyone is using directories.

Actually, not only directory, that is also checked in the tests here.

Modifying code from existing functionality is never advised.

However, if a separate function that is clear and returns the location instead the bundle file is required, then it's better to create a new one in order to preserve backwards compatibility.

Anyway, I agree, that the changing of the interface is anot good idea. I'll add the implementaion for the location (as well as files as directories).

@jannejy jannejy force-pushed the support_folder_of_root_certificates branch 2 times, most recently from cf7e810 to d8d064a Compare November 18, 2024 16:31
@AssemblyJohn
Copy link
Collaborator

Now it looks good, however make sure that in libocpp code, you are handling both the case of a directory and of a bundle file.

@jannejy jannejy force-pushed the support_folder_of_root_certificates branch from e4f9236 to 0b01822 Compare November 19, 2024 10:00
@AssemblyJohn
Copy link
Collaborator

Looks good, see only that the tests are passing, and I'll approve.

@jannejy jannejy force-pushed the support_folder_of_root_certificates branch from e69a3c3 to cd61c04 Compare November 26, 2024 09:39
@jannejy
Copy link
Contributor Author

jannejy commented Nov 26, 2024

The build fails not because of my changes, something is wrong after gmock installation

@AssemblyJohn
Copy link
Collaborator

The build fails not because of my changes, something is wrong after gmock installation

Does it work locally?

@jannejy
Copy link
Contributor Author

jannejy commented Nov 26, 2024

Does it work locally?

Yes

@AssemblyJohn
Copy link
Collaborator

When trying to build locally with: cmake -DBUILD_TESTING=ON -DCMAKE_BUILD_TYPE=Debug -DCMAKE_INSTALL_PREFIX=./dist .. I get a similar error. Looking into this.

@AssemblyJohn
Copy link
Collaborator

Rebase and try again, since we had a recent merge.

@jannejy jannejy force-pushed the support_folder_of_root_certificates branch from 41e3e01 to 4166a30 Compare November 26, 2024 12:00
Copy link
Collaborator

@AssemblyJohn AssemblyJohn 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. One note, in the header function get_verify_location, also mention that the rehash is called each time this function is invoked.

Signed-off-by: Ivan Rogach <[email protected]>
@jannejy jannejy changed the title Return folder with verified certificates Support hashing a certificate directory Nov 26, 2024
@jannejy jannejy merged commit 9ba21d4 into main Nov 26, 2024
7 of 8 checks passed
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