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

Add support for certificate links #29

Merged
merged 7 commits into from
Jan 25, 2024
Merged

Conversation

mennodegraaf
Copy link
Collaborator

  • Support updating of certificate links
  • Adjust get_key_pair to return both single leaf and chain (if found)

@AssemblyJohn
Copy link
Collaborator

In review, will need a bit of time since it modified the 'get_key_pair' function that is used in many places.

@AssemblyJohn
Copy link
Collaborator

AssemblyJohn commented Jan 19, 2024

@mennodegraaf I have a question, why is both the chain and certificate, required (when returned from the 'get_key_pair') result? Because the full chain can be found in either the root CA or the LeafCA, it is certain that the full chain is retrieved when we request both.

Plus if we modify the interface (get_key_pair) we need also to change in everest core not to break the compatibility.

@AssemblyJohn
Copy link
Collaborator

AssemblyJohn commented Jan 19, 2024

@mennodegraaf another modification was made, the get_key_pair function return had it's members renamed so that it doesn't break compatibility with existing function usage. Except that, as long as the new symlinks don't break any compatibility with existing code, I'd consider it good to do.

Menno de Graaf and others added 6 commits January 22, 2024 10:39
Separated single leaf and full chain in get_key_pair() to achieve that

Signed-off-by: Menno de Graaf <[email protected]>
Signed-off-by: Menno de Graaf <[email protected]>
Signed-off-by: Menno de Graaf <[email protected]>
@@ -116,8 +116,8 @@ struct OCSPRequestDataList {
};
struct KeyPair {
fs::path key; ///< The path of the PEM or DER encoded private key
fs::path certificate; ///< The path of the PEM or DER encoded certificate
fs::path chain; ///< The path of the PEM or DER encoded certificate chain
fs::path certificate; ///< The path of the PEM or DER encoded certificate chain
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps this can be renamed to certificate_chain to more clearly distinguish between certificate_single. This has the advantage that you will have to go through all code where certificate was used before, where you can verify that the chain is indeed really desired there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Being part of the public API, I am unsure where it might be used (maybe in non-public repos), so while I do agree with you from the clarity of the code, I would incline to keep it as-is.

@AssemblyJohn AssemblyJohn self-requested a review January 24, 2024 07:40
Signed-off-by: Menno de Graaf <[email protected]>
@AssemblyJohn AssemblyJohn merged commit 1604c8b into main Jan 25, 2024
2 checks passed
@AssemblyJohn AssemblyJohn deleted the mg-certificate-links branch January 25, 2024 08:46
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