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     

... and 1 file with indirect coverage changes

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

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

(also needs a rebase)

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.

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).

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.

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?

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 :)

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.

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?

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?

Also probably needs a rebase once #183 is merged.

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.

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

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