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

Improvements in DiskSigner and DiskVerifier and refactors for the use of signing certificate chains. #235

Merged

Conversation

maxlambrecht
Copy link
Contributor

@maxlambrecht maxlambrecht commented Jun 21, 2023

Pull request check list

  • Proper tests/regressions included?
  • Documentation updated?

Description of change

This PR introduces several enhancements to the DiskSigner and DiskVerifier. Key changes include the addition of support for intermediate CAs and the signing certificate chain. Two new configuration properties have been added to the DiskSigner: trust_bundle_path which is required when the ca_cert_path contains a non root CA, and signing_cert_ttl which allows to configure the TTL of the signing certificate to align it with the expiration time of the SPIRE bundle.

Test coverage has been expanded to ensure these new features perform as expected.

The API has also been updated to accurately reflect that the signing certificate is now a certificate chain, ensuring consistency between the implementation and the APIs.

Corresponding updates have also been made to the DB schema names storing the signing certificate chain.

Finally, the related documentation has been updated.

Which issue this pull requests fixes

@maxlambrecht maxlambrecht changed the title Improvements in DiskSigner and DiskVerifier and refactors for signing certificate chain. Improvements in DiskSigner and DiskVerifier and refactors for the use of signing certificate chains. Jun 21, 2023
@maxlambrecht maxlambrecht changed the title Improvements in DiskSigner and DiskVerifier and refactors for the use of signing certificate chains. [DO NOT MERGE] Improvements in DiskSigner and DiskVerifier and refactors for the use of signing certificate chains. Jun 21, 2023
Copy link
Collaborator

@Victorblsilveira Victorblsilveira left a comment

Choose a reason for hiding this comment

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

@maxlambrecht good work here, nice abstraction and good config explanations, just some minors to improve code's readability

pkg/harvester/catalog/catalog_test.go Outdated Show resolved Hide resolved
pkg/harvester/integrity/disk.go Show resolved Hide resolved
pkg/harvester/integrity/disk.go Outdated Show resolved Hide resolved
@maxlambrecht maxlambrecht changed the title [DO NOT MERGE] Improvements in DiskSigner and DiskVerifier and refactors for the use of signing certificate chains. Improvements in DiskSigner and DiskVerifier and refactors for the use of signing certificate chains. Jul 13, 2023
@maxlambrecht maxlambrecht force-pushed the use-chain-in-disk-signer branch from baa222b to 3d68af9 Compare July 13, 2023 15:09
Signed-off-by: Max Lambrecht <[email protected]>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Collaborator

@Victorblsilveira Victorblsilveira left a comment

Choose a reason for hiding this comment

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

Well, I really don't know why you are so reluctant in changing a name to meet the real implementation. Again, even if you plan to change in the future, the current implementation is using SHA256, so make it clear. The implementation itself will not change, just the name, the flexibility that you mention will not be affected by the name, nor any of the encapsulation and functionality.

@maxlambrecht
Copy link
Contributor Author

Well, I really don't know why you are so reluctant in changing a name to meet the real implementation. Again, even if you plan to change in the future, the current implementation is using SHA256, so make it clear. The implementation itself will not change, just the name, the flexibility that you mention will not be affected by the name, nor any of the encapsulation and functionality.

My reluctance to change the name to reflect the current implementation is due to the following reasons:

  • This DiskSigner struct is not being introduced in this PR; it already exists in the codebase. Modifying the name will also impact all dependencies that use this struct, which increases the scope and complexity of this PR.

  • We will expand the functionality of this struct, specifically to enable configuration of both the hashing and signing algorithms. Changing the name now to reflect the current implementation, only to revert it back when the updates are implemented, would introduce unnecessary changes.

  • The proposed name DiskSignerSHA256 only reflects the hashing algorithm, but does not account for the signing algorithm. This is a partial representation of the real implementation.

I hope this helps in understanding my standpoint.

@maxlambrecht maxlambrecht merged commit bad228f into HewlettPackard:main Jul 18, 2023
@maxlambrecht maxlambrecht deleted the use-chain-in-disk-signer branch July 18, 2023 15:47
maxlambrecht added a commit that referenced this pull request Jul 18, 2023
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