Support compiling without cryptography primitives #208
Conversation
rcgen/src/key_pair.rs
Outdated
| @@ -211,6 +229,12 @@ impl KeyPair { | |||
| } | |||
| } | |||
|
|
|||
| /// Generate a new random key pair for the specified signature algorithm | |||
| #[cfg(not(feature = "crypto"))] | |||
| pub fn generate(_alg: &'static SignatureAlgorithm) -> Result<Self, Error> { | |||
There was a problem hiding this comment.
Original comment by @djc: #207 (comment)
It doesn't seem like a great idea to generate an impl that can only error?
Would you rather I remove this entirely and the generate just isn't available without the crypto feature?
There was a problem hiding this comment.
yes, that's generally preferable. Rust has strong static checks. the std implementations for wasm are panicking, and now I heard that it is considered to be a mistake.
There was a problem hiding this comment.
I see. Well, this has already been done.
There was a problem hiding this comment.
Did a commit for this change not get pushed or am I confused about the outcome of the discussion?
The state of the branch at the moment is that there is still a generate impl gated by #[cfg(not(feature = "crypto"))] that will only ever produce an error. I read the above discussion as preferring this impl not exist.
There was a problem hiding this comment.
@corrideat I think this piece of feedback is the only merge blocker. Do you think you'll have a chance to fix it?
|
I think this needs a rebase now that the refactoring in #205 has landed. Would you mind tidying up the commit history at the same time? I think all of the commits could be squashed down into one. |
|
@cpu I've now rebased; it probably needs another review just in case. |
rcgen/src/key_pair.rs
Outdated
| @@ -211,6 +229,12 @@ impl KeyPair { | |||
| } | |||
| } | |||
|
|
|||
| /// Generate a new random key pair for the specified signature algorithm | |||
| #[cfg(not(feature = "crypto"))] | |||
| pub fn generate(_alg: &'static SignatureAlgorithm) -> Result<Self, Error> { | |||
There was a problem hiding this comment.
Did a commit for this change not get pushed or am I confused about the outcome of the discussion?
The state of the branch at the moment is that there is still a generate impl gated by #[cfg(not(feature = "crypto"))] that will only ever produce an error. I read the above discussion as preferring this impl not exist.
This makes all of the crypto libraries optional. My use case requires simply generating the binary ASN.1 structures and all cryptography is handled externally with HSMs.
This change removes unnecessary code and build complexity (for example, I'm targeting WASM, and I need to take extra steps to build the dependencies I don't need).
Note to the maintainer: this is a clean (and ideally better) rework of a previous PR, #207. Because I deleted the older and messier branch, the previous PR couldn't be updated, so I closed it.