ring icon indicating copy to clipboard operation
ring copied to clipboard

RSA padding trait methods are public, and take `BitLength` arguments, but `BitLength` isn't public

Open indygreg opened this issue 2 years ago • 2 comments

I'm attempting to perform PKCS#1 padding using ring's APIs for doing so. (I need to write the result over the wire to a YubiKey, whose signature creation API wants you to feed in the PKCS#1 padded value: https://github.com/Yubico/yubico-piv-tool/blob/89bd496167ea7601b920418764f3223bd863668c/lib/tests/api.c#L421)

The ring::rsa::padding:RsaPadding trait is public so the padding API is available for me to use. While the bits::BitLength struct is public, the bits module is not public. So there's no easy way to obtain an instance of BitLength to feed into RsaPadding::encode().

Would it be possible to make bits public or otherwise make BitLength constructable outside the crate (possibly via a From<usize> implementation)?

indygreg avatar Mar 29 '22 19:03 indygreg

As you've probably guessed, the traits are public but I didn't intend to make the trait methods publicly callable. That's just a mistake that I was making early on in ring's public API design that I've tried to correct over time. Looks like I missed a case.

That said, I understand you have a need for an API for RSA padding, without the RSA computation. I am open to having a supported public API for that. IIRC, the current implementation of the current API does make some assumptions about preconditions that are ensured by internal callers in ring. We probably need to do a little refactoring beyond making BitLength public in order to have a good, robust API.

Would it be possible to make bits public or otherwise make BitLength constructable outside the crate (possibly via a From implementation)?

BitLength has from_usize_bits and from_usize_bytes to disambiguate between whether the input is measuring bits or measuring bytes, after seeing bugs due to ambiguity in BoringSSL. I think we should "just" make those two construction functions public.

briansmith avatar Apr 06 '22 19:04 briansmith

@indygreg If you, or anybody, contributes a PR that implements a proper public API for this, with tests (in tests/) for that API, I will review and integrate it. Otherwise, I'm just keeping this issue open for tracking that we need to stop leaking these traits' methods in the public API. We can't be exposing them but not thoroughly testing them, and I'll never have time to implement a proper API and write the tests myself.

briansmith avatar Oct 14 '23 17:10 briansmith