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

Fix RSAPublicKey hierarchy and hidden virtual functions #6673

Open
2 tasks
maxtropets opened this issue Dec 2, 2024 · 2 comments
Open
2 tasks

Fix RSAPublicKey hierarchy and hidden virtual functions #6673

maxtropets opened this issue Dec 2, 2024 · 2 comments

Comments

@maxtropets
Copy link
Collaborator

maxtropets commented Dec 2, 2024

List of problems to solve while refactoring the interface for signature verification

  • RSAPublicKey and RSAKeyPair both define verify(signature_args..., md_type, salt_length), is it even legit?..
  • RSAKeyPair_OpenSSL implements verify(signature_args..., md_type, salt_length), BUT PublicKey_OpenSSL implements verify(signature_args..., md_type, hash_bytes&). Here's why JWT authentication uses the wrong verification impl (check comment).
  • salt_length is size_t, therefore it's not possible to pass options like RSA_PSS_SALTLEN_AUTO == -2 or other predefined constants. Not sure what's the best way to do it in the interface, it's TBD how different paddings and salt work as per documentation first, but this has to be fixed.
  • this and this
  • Found out RSAPublicKey_OpenSSL doesn't throw when created with EC DER, silently passes, I think all ctor-s need revision and proper unit testing
@maxtropets
Copy link
Collaborator Author

maxtropets commented Dec 3, 2024

A sketch partially displaying the current state.

I'm confused about a real purpose of having verifying methods in (RSA)KeyPair interface, while we could have KeyPair as a child of PublicKey, so we won't copy verification interface, as well as public_key_der(), public_key_pem, etc.

Image

@maxtropets
Copy link
Collaborator Author

Suspected #6405 to hide the verify(args..., md) overloads by adding salt_size, but ruled that out, because it turns out the PublicKey::verify(args, md) is not virtual (🤦).

Instead, it has an implementation, which computes a hash of the contents and them calls virtual verify(args, md, hash_out)

However, salt_size shall also be removed, as it doesn't fit the common interface. I'm thinking of some sort of separate params struct/variant to pass depending on the underlying key type, and leave signature and contents as the only interface.

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

No branches or pull requests

1 participant