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

SD-1822: Use certificates rather than public keys #240

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

nt-gt
Copy link
Collaborator

@nt-gt nt-gt commented Dec 6, 2024

User description

As a side-effect of this, we can now support EC keys instead of only RSA keys (without having to manually parse and understand the DER/ASN.1 format).


PR Type

enhancement


Description

  • Enhanced the PayloadSignerFactory to support EC keys in addition to RSA keys, allowing for more flexible cryptographic operations.
  • Implemented functionality to generate self-signed certificates, replacing the previous reliance on public keys.
  • Updated the RSAPayloadSigner to handle X509 certificates.
  • Added methods for parsing EC private keys and encoding certificates in PEM format.

Changes walkthrough 📝

Relevant files
Enhancement
PayloadSignerFactory.java
Add support for EC keys and certificate handling                 

ebl/src/main/java/org/dcsa/conformance/standards/ebl/crypto/PayloadSignerFactory.java

  • Added support for EC keys alongside RSA keys.
  • Introduced methods to parse and handle EC private keys.
  • Implemented self-signed certificate generation.
  • Replaced public key handling with certificate handling.
  • +110/-65
    RSAPayloadSigner.java
    Use X509 certificates instead of RSA public keys                 

    ebl/src/main/java/org/dcsa/conformance/standards/ebl/crypto/impl/RSAPayloadSigner.java

  • Replaced RSA public key with X509 certificate handling.
  • Updated method to return certificate in PEM format.
  • +6/-6     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @nt-gt nt-gt force-pushed the SD-1822-use-cert-rather-than-public-keys branch from f9b5f21 to 93f5b21 Compare December 6, 2024 13:31

    This comment was marked as outdated.

    @nt-gt nt-gt force-pushed the SD-1822-use-cert-rather-than-public-keys branch from 93f5b21 to e986168 Compare December 6, 2024 13:48
    @nt-gt nt-gt marked this pull request as ready for review December 6, 2024 14:05
    Copy link

    qodo-merge-pro bot commented Dec 6, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 Security concerns

    Certificate validation:
    The code does not appear to implement certificate chain validation or revocation checking when verifying signatures. This could potentially allow the use of compromised or revoked certificates. Consider adding proper certificate validation mechanisms.

    ⚡ Recommended focus areas for review

    Security Configuration
    The self-signed certificate generation uses hardcoded validity period of 2500 days which may be too long for security best practices. Consider making this configurable or using a shorter period.

    Error Handling
    Generic exception handling in verifierFromPemEncodedPublicKey() may mask specific errors. Consider handling different types of exceptions separately.

    Code Design
    The RSAPayloadSigner class name is now misleading since it handles both RSA and EC keys. Consider renaming to reflect its more general purpose.

    This comment was marked as outdated.

    @nt-gt nt-gt force-pushed the SD-1822-use-cert-rather-than-public-keys branch 2 times, most recently from 51e6c7b to df60ec7 Compare December 6, 2024 14:15
    As a side-effect of this, we can now support EC keys instead of only
    RSA keys (without having to manually parse and understand the
    DER/ASN.1 format).
    @nt-gt nt-gt force-pushed the SD-1822-use-cert-rather-than-public-keys branch from df60ec7 to affe678 Compare December 6, 2024 14:16
    Copy link
    Collaborator

    @jkosternl jkosternl left a comment

    Choose a reason for hiding this comment

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

    Thanks for your improvements NT! While looking at the code, it is hard to judge if it actually works. I guess you've verified this yourself. Would/can you consider adding some unit tests, to verify this keeps working in the future as well? Also when somebody else needs to change something.

    @@ -92,47 +113,21 @@ public class PayloadSignerFactory {
    -----END PRIVATE KEY-----
    """;

    @SuppressWarnings("secrets:S6706")
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    Can you add a comment behind it, of what you're actually suppressing? I can guess, but I'd rather read it.

    @nt-gt
    Copy link
    Collaborator Author

    nt-gt commented Dec 9, 2024

    Thanks for your improvements NT! While looking at the code, it is hard to judge if it actually works. I guess you've verified this yourself. Would/can you consider adding some unit tests, to verify this keeps working in the future as well? Also when somebody else needs to change something.

    You cannot start any of the PINT scenarios without this code working, so there are already integration tests for this.

    @nt-gt nt-gt merged commit 3cf9f4e into dev Dec 9, 2024
    1 check passed
    @nt-gt nt-gt deleted the SD-1822-use-cert-rather-than-public-keys branch December 9, 2024 12:12
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants