Refactor `KeyPair` constructor #170

thomaseizinger · 2023-10-03T08:04:56Z

The TryFrom impl doesn't communicate that these bytes need to be DER-encoded. Forcing the user to use the constructor makes this obvious.

This is a breaking change because it removes trait implementations.

codecov · 2023-10-03T08:18:41Z

Codecov Report

Merging #170 (e086f4a) into main (aaf4dd7) will increase coverage by 0.60%.
The diff coverage is 75.90%.

@@            Coverage Diff             @@
##             main     #170      +/-   ##
==========================================
+ Coverage   72.14%   72.75%   +0.60%     
==========================================
  Files           7        7              
  Lines        1881     1901      +20     
==========================================
+ Hits         1357     1383      +26     
+ Misses        524      518       -6

Files	Coverage Δ
src/key_pair.rs	`75.83% <75.90%> (+3.56%)`	⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

djc

I'd squash these commits together. Maybe KeyPair::from_raw() should be called from_der(), too?

est31 · 2023-10-03T11:44:26Z

If what @djc said is implemented, this can be merged I think.

est31 · 2023-10-03T13:42:23Z

(also needs a rebase)

The `TryFrom` impl doesn't communicate that these bytes need to be DER-encoded. Forcing the user to use the constructor makes this obvious.

This more accurately describes what the function does.

Instead of chaining if-let-else constructs, we early return as soon as we successfully parse a keypair from the provided bytes. This is a tiny bit more code but easier to understand.

thomaseizinger · 2023-10-04T02:28:06Z

I'd squash these commits together. Maybe KeyPair::from_raw() should be called from_der(), too?

I've named it guess_kind_from_der and refactored it to use early-returns. Let me know what you think.

djc · 2023-10-04T08:09:12Z

I don't find all the refactoring very convincing because it still ends up with quite a bit of duplication. Would be happy to merge something closer to the original form though, I'd maybe rename guess_kind_from_der() to try_from_der() which feels more idiomatic (and more concise).

thomaseizinger · 2023-10-05T03:02:56Z

I don't find all the refactoring very convincing because it still ends up with quite a bit of duplication.

Fair enough. I am surprised you mention duplication as the reason because the current version has less duplication than what is in main. At least when it comes to the semantic coupling of the different algorithm identifiers. Those are in a single place now whereas before they were duplicated between two functions.

Would be happy to merge something closer to the original form though, I'd maybe rename guess_kind_from_der() to try_from_der() which feels more idiomatic (and more concise).

I went with guess because I deemed it important to convey to the caller that this function isn't necessarily failing because the bytes are malformed but simply because we might not support a particular combination. Additionally, if you pass malformed bytes, the error is not going to tell you what is wrong because we are hiding this by simply trying the next one. To me, that is very different from what I'd expect from a typical try_from function, hence the special name.

cpu · 2023-10-10T15:58:05Z

I went with guess because I deemed it important to convey to the caller that this function isn't necessarily failing because the bytes are malformed but simply because we might not support a particular combination. Additionally, if you pass malformed bytes, the error is not going to tell you what is wrong because we are hiding this by simply trying the next one. To me, that is very different from what I'd expect from a typical try_from function, hence the special name.

By this logic shouldn't KeyPair::from_der be renamed to KeyPair::guess_from_der? The KeyPair::guess_from_der fn is pub(crate) so the extra information being conveyed to the caller through the updated name is only helpful to other rcgen developers, not external users right?

thomaseizinger · 2023-10-10T22:16:00Z

I went with guess because I deemed it important to convey to the caller that this function isn't necessarily failing because the bytes are malformed but simply because we might not support a particular combination. Additionally, if you pass malformed bytes, the error is not going to tell you what is wrong because we are hiding this by simply trying the next one. To me, that is very different from what I'd expect from a typical try_from function, hence the special name.

By this logic shouldn't KeyPair::from_der be renamed to KeyPair::guess_from_der? The KeyPair::guess_from_der fn is pub(crate) so the extra information being conveyed to the caller through the updated name is only helpful to other rcgen developers, not external users right?

Yeah good thinking! Happy to change that if there is consensus on it :)

cpu · 2023-11-14T22:59:11Z

Yeah good thinking! Happy to change that if there is consensus on it :)

It seems fine to me. Does anyone else have an opinion? I think it would be a good idea to take an action on this PR one way or the other.

thomaseizinger · 2023-11-22T21:33:14Z

the updated name is only helpful to other rcgen developers, not external users right?

Not sure about others but I very often have a quick glance at the "first" layer of abstraction that I am calling from my code so in addition to there being value for contributors, I think there is also value for users. Arguably, not as much as the name of a public function of course.

Yeah good thinking! Happy to change that if there is consensus on it :)

It seems fine to me. Does anyone else have an opinion? I think it would be a good idea to take an action on this PR one way or the other.

I am open to either! There is also an open concern from @djc about whether the early-return style and separate functions are desirable. Should I revert those changes so we are just removing the TryFrom impls?

est31 · 2023-11-22T23:24:51Z

src/key_pair.rs


+	fn pkcs_rsa_sha256(der: &[u8]) -> Result<KeyPair, Error> {


Could you remove these functions in favour of the old state which assigned kind? This saved us a lot of repetitive code.

est31 · 2023-11-22T23:28:13Z

I'm fine with the changes to the removal of TryFrom and the rename of from_raw to guess_kind_from_der, but I don't like the addition of these constructor functions and the changes of the implementations of guess_kind_from_der and from_der_and_sign_algo. Could we have former without the latter?

est31 · 2023-12-09T01:03:34Z

Also probably needs a rebase once #183 is merged.

djc · 2023-12-11T08:43:33Z

Actually, I think maybe a cleaner route might be to introduce rustls-pki-types as a public dependency and use TryFrom implementations for PrivateKeyDer<'_>. This would facilitate interoperability with rustls-pemfile (already a pretty widely used crate -- about twice as many recent downloads as the pem crate) for input from PEM files and make the types pretty clear.

thomaseizinger · 2024-02-14T07:43:18Z

Closing as stale. Unfortunately, I don't have the resources to continue on this, sorry!

cpu · 2024-02-14T14:06:24Z

Sorry that this one ended up sitting around for so long. Thanks for the contribution.

djc reviewed Oct 3, 2023

View reviewed changes

Remove TryFrom impls

345bc9b

The `TryFrom` impl doesn't communicate that these bytes need to be DER-encoded. Forcing the user to use the constructor makes this obvious.

thomaseizinger force-pushed the refactor-try-from branch from e57110c to 345bc9b Compare October 4, 2023 02:19

thomaseizinger requested a review from djc October 4, 2023 02:20

thomaseizinger added 2 commits October 4, 2023 13:21

Rename from_raw to guess_kind_from_der

95fb9fb

This more accurately describes what the function does.

Refactor guess_kind_from_der

c3abdc5

Instead of chaining if-let-else constructs, we early return as soon as we successfully parse a keypair from the provided bytes. This is a tiny bit more code but easier to understand.

Refactor from_der_and_sign_algo

0b14bd5

thomaseizinger force-pushed the refactor-try-from branch from 8182fbf to 3b61b11 Compare October 4, 2023 02:49

Remove duplication between constructors

e086f4a

thomaseizinger force-pushed the refactor-try-from branch from 3b61b11 to e086f4a Compare October 4, 2023 02:50

est31 reviewed Nov 22, 2023

View reviewed changes

This was referenced Dec 8, 2023

Add PreSpecified(Vec<u8>) option to KeyIdMethod. #197

Merged

0.12.0 release planning #199

Closed

Tudyx mentioned this pull request Feb 11, 2024

Use pki_types to improve the interoperability with the rustls ecosystem #223

Merged

thomaseizinger closed this Feb 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `KeyPair` constructor #170

Refactor `KeyPair` constructor #170

thomaseizinger commented Oct 3, 2023

codecov bot commented Oct 3, 2023 •

edited

Loading

djc left a comment

est31 commented Oct 3, 2023

est31 commented Oct 3, 2023

thomaseizinger commented Oct 4, 2023

djc commented Oct 4, 2023

thomaseizinger commented Oct 5, 2023

cpu commented Oct 10, 2023

thomaseizinger commented Oct 10, 2023

cpu commented Nov 14, 2023

thomaseizinger commented Nov 22, 2023

est31 Nov 22, 2023

est31 commented Nov 22, 2023

est31 commented Dec 9, 2023

djc commented Dec 11, 2023 •

edited

Loading

thomaseizinger commented Feb 14, 2024

cpu commented Feb 14, 2024

Refactor KeyPair constructor #170

Refactor KeyPair constructor #170

Conversation

thomaseizinger commented Oct 3, 2023

codecov bot commented Oct 3, 2023 • edited Loading

Codecov Report

djc left a comment

Choose a reason for hiding this comment

est31 commented Oct 3, 2023

est31 commented Oct 3, 2023

thomaseizinger commented Oct 4, 2023

djc commented Oct 4, 2023

thomaseizinger commented Oct 5, 2023

cpu commented Oct 10, 2023

thomaseizinger commented Oct 10, 2023

cpu commented Nov 14, 2023

thomaseizinger commented Nov 22, 2023

est31 Nov 22, 2023

Choose a reason for hiding this comment

est31 commented Nov 22, 2023

est31 commented Dec 9, 2023

djc commented Dec 11, 2023 • edited Loading

thomaseizinger commented Feb 14, 2024

cpu commented Feb 14, 2024

Refactor `KeyPair` constructor #170

Refactor `KeyPair` constructor #170

codecov bot commented Oct 3, 2023 •

edited

Loading

djc commented Dec 11, 2023 •

edited

Loading