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

Advanced DH support #1353

Merged
merged 6 commits into from
Nov 29, 2020
Merged

Advanced DH support #1353

merged 6 commits into from
Nov 29, 2020

Conversation

wiomoc
Copy link
Contributor

@wiomoc wiomoc commented Oct 7, 2020

This implements the functionality to generate the keys needed and use them for key derivation
closes #498

implement better prime handling
@wiomoc wiomoc marked this pull request as ready for review October 8, 2020 13:45
Copy link
Contributor

@stbuehler stbuehler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking or returning 3 values of the same type in a function seems always bad to me, because you are gonna screw up the order eventually. Perhaps this should be wrapped into structs (one for owning them, one for borrowed values)?

You also should add some doc comments for the functions in the openssl crate (but not openssl-sys); usually a short text what it does and a reference to the openssl man page.

openssl-sys/src/dh.rs Outdated Show resolved Hide resolved
@@ -45,15 +45,59 @@ where
}

impl Dh<Params> {
pub fn from_params(p: BigNum, g: BigNum, q: BigNum) -> Result<Dh<Params>, ErrorStack> {
pub fn from_params(p: BigNum, g: BigNum, q: Option<BigNum>) -> Result<Dh<Params>, ErrorStack> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the public API. How about providing this variant as a new function (and use the new function to implement the old one by simply wrapping q in Some(...))?

openssl/src/dh.rs Outdated Show resolved Hide resolved
openssl/src/dh.rs Outdated Show resolved Hide resolved
openssl/src/dh.rs Outdated Show resolved Hide resolved
@wiomoc wiomoc force-pushed the master branch 2 times, most recently from 4968047 to 4fe7836 Compare October 10, 2020 13:41
Copy link
Contributor

@stbuehler stbuehler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looking good to me now.

I think removing Result<...> from accessors was the right choice; those shouldn't fail with a consistent state, which the rust types should ensure.

We'll see what sfackler thinks about DhParamsRef; perhaps another alternative to returning a tuple would be standalone accessors (prime_p, prime_q, generator?).

The local DH_get0_pqg implementation doesn't handle returning only partial data yet like DH_get0_key (the openssl does I think); as long as this isn't used it probably doesn't matter.

Tests still fail as transpose wasn't stable in rust 1.31.0 (the current min version; I think sfackler considers raising it breaking the API).

openssl/src/dh.rs Outdated Show resolved Hide resolved
@sfackler
Copy link
Owner

I think it's probably best to follow the convention used by e.g. the Rsa type and have separate accessors for the different components.

While I do in crease the MSRV over time, I like to keep it where it is where possible. In this case it's easy to work around the lack of transpose so I'd just not use the method.

@wiomoc wiomoc force-pushed the master branch 2 times, most recently from 1b7678b to 37ce7a7 Compare October 11, 2020 18:47
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

Successfully merging this pull request may close these issues.

Implement DH functionality
3 participants