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

feat: eliminate crypto.SignerOpts from Sign/Verify #2636

Closed
wants to merge 1 commit into from

Conversation

ibukanov
Copy link
Collaborator

@ibukanov ibukanov commented Aug 7, 2024

Eliminate io.Reader and crypto.SignerOpts arguments from the Signator/Verifier interfaces to avoid the need to pass crypto.hash(0) in multiple places.

For symmetry with httpsignature.Ed25519PubKey add Ed25519PrivKey wrapper for ed25519.PrivateKey that implements the updated Verifier interface. In addition the wrapper provides Public() and PublicHex() methods to simplify the common operations.

Close #2635

Summary

Type of Change

  • Product feature
  • Bug fix
  • Performance improvement
  • [x ] Refactor
  • Other

Tested Environments

  • [ x] Development
  • Staging
  • Production

Before Requesting Review

  • [ x] Does your code build cleanly without any errors or warnings?
  • Have you used auto closing keywords?
  • Have you added tests for new functionality?
  • Have validated query efficiency for new database queries?
  • Have documented new functionality in README or in comments?
  • Have you squashed all intermediate commits?
  • Is there a clear title that explains what the PR does?
  • Have you used intuitive function, variable and other naming?
  • Have you requested security and/or privacy review if needed
  • Have you performed a self review of this PR?

Manual Test Plan

@ibukanov ibukanov requested a review from clD11 August 7, 2024 16:06
Copy link

github-actions bot commented Aug 7, 2024

[puLL-Merge] - brave-intl/bat-go@2636

Description

This PR refactors the httpsignature package and its usage across the codebase. It introduces new types and methods for handling Ed25519 keys and signatures, and updates the interfaces for signing and verification. The changes aim to improve type safety and reduce the reliance on the crypto package.

Changes

Changes

  1. libs/httpsignature/ed25519.go:

    • Introduced Ed25519PrivKey type
    • Added Ed25519Signator interface
    • Updated Verify method to VerifySignature
    • Added SignMessage, Public, and PublicHex methods for Ed25519PrivKey
  2. libs/httpsignature/hmac.go:

    • Updated Sign to SignMessage
    • Updated Verify to VerifySignature
  3. libs/httpsignature/httpsignature.go:

    • Updated Signator and Verifier interfaces
    • Removed Opts field from ParameterizedSignator and ParameterizedKeystoreVerifier
    • Updated Sign to SignRequest
    • Updated Verify to VerifyRequest
  4. Various test files:

    • Updated test cases to use new method signatures and types
  5. services/wallet, tools/settlement, tools/vault, tools/wallet:

    • Updated usage of httpsignature package to use new types and methods

Possible Issues

  1. The changes to the interfaces and method signatures might require updates in external packages that depend on this code.
  2. There might be some performance implications due to the changes in signature verification logic, especially in the HMAC implementation.

Security Hotspots

  1. The ErrBadSignature error is now used to indicate signature mismatch. Ensure that this error is properly handled in all places where signature verification is performed to avoid potential security vulnerabilities.

  2. The removal of the crypto.SignerOpts parameter from signing and verification methods might impact the flexibility of the signing process. Ensure that this change doesn't limit the ability to use different hashing algorithms if required in the future.

  3. The PublicHex method in Ed25519PrivKey exposes the public key. Ensure that this method is used carefully and doesn't accidentally expose sensitive information.

Overall, the changes appear to improve type safety and consistency in the signature handling process. However, careful testing and review of all affected components is necessary to ensure that the security properties of the system are maintained.

@ibukanov ibukanov force-pushed the igor/2635/no-ops branch 4 times, most recently from be94f3a to 2d42339 Compare August 7, 2024 20:36
Eliminate io.Reader and crypto.SignerOpts arguments from the
Signator/Verifier interfaces to avoid the need to pass crypto.hash(0) in
multiple places.

For symmetry with httpsignature.Ed25519PubKey add Ed25519PrivKey wrapper
for ed25519.PrivateKey that implements the updated Verifier interface.
In addition the wrapper provides Public() and PublicHex() methods to
simplify the common operations.

Close #2635
@ibukanov ibukanov closed this Nov 5, 2024
@ibukanov ibukanov deleted the igor/2635/no-ops branch November 5, 2024 08:06
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.

Eliminate redundant crypto.SignerOpts argument
1 participant