Issue #704, module `dh` #742

ejmg · Sep 28, 2017

closes #704

ejmg · Sep 28, 2017

Deleted comments about failed tests due to #![deny(missing_docs)] and whether to include #![deny(missing_docs)] after finishing documenting and whether to even document Dh and DhRef. Fackler answered in Gitter.

sfackler · Sep 29, 2017

sfackler requested changes Sep 29, 2017

View changes

sfackler · Sep 29, 2017

openssl/src/dh.rs

+//! 
+//! summary: bindings for dh library of OpenSSL
+//! author: steven fackler
+//! version: 


I don't think the first couple of lines here are really relevant for someone reading this documentation.

sfackler · Sep 29, 2017

openssl/src/dh.rs

    fn drop = ffi::DH_free;

+    /// Diffie-Hellman Key agreement object
+    /// A key object that is based on the diffie-hellman algorithm


This line will be folded up to the first line by Markdown.

sfackler · Sep 29, 2017

openssl/src/dh.rs

 impl Dh {
+
+    /// This method will set the c pointers for the DH object, p, q, and g. If the parameters have
+    /// not been set, they will default to NULL.


The second sentence doesn't seem super relevant - we are setting them.

@sfackler When I read the code, I read it as we're taking values in, regardless of what they are, and setting them as the ptrs. Do we not inherit the behavior of defaulting to NULL when someone calls this function and provides nothing for the actual values for p, g, and q?

The user can't provide nothing for those values.

sfackler · Sep 29, 2017

openssl/src/dh.rs

+//! author: steven fackler
+//! version: 
+//!
+//! dh is the diffie-hellman (dh) algorithm which allows for two parties to derive the key used for


Capitalization

sfackler · Sep 29, 2017

openssl/src/dh.rs

+//! communication. in dh, the two parties agree to some large prime, `p`, and a generator, `g`,
+//! where 0 < g < p.
+//!
+//! For a full summary of the algo, please read https://wiki.openssl.org/index.php/Diffie_Hellman


Can the links be shortened up instead of being raw URLs in the documentation? I.e. For a full summary of Diffie-Hellman, refer to the [OpenSSL wiki](https://wiki.openssl.org/index.php/Diffie_Hellman).

+1, didn't even know this was possible in rustdoc

sfackler · Sep 29, 2017

openssl/src/dh.rs

+    /// This method will set the c pointers for the DH object, p, q, and g. If the parameters have
+    /// not been set, they will default to NULL.
+    ///
+    /// This calls DH_get0_pqg() on OpenSSL 1.1.0 and inits DH's fields manually in 1.1.x.


"initializes"

sfackler · Sep 29, 2017

openssl/src/dh.rs

    from_pem!(Dh, ffi::PEM_read_bio_DHparams);
    from_der!(Dh, ffi::d2i_DHparams);

+    /// return a DH object for the IETF RFC 5114 value.


Capitalization

The phrasing here could be a bit more clear - maybe something like "Returns the standard 1024 bit MODP group with 160-bit prime order subgroup specified in RFC 5114"

Does the following work?

/// Return a DH object for the standard 1024 bit MODP group with 160-bit prime order subgroup /// specified in [IETF RFC 5114](https://tools.ietf.org/html/rfc5114#section-2.1). /// /// get_1024_160 corresponds to DH_get_1024_160() in 1.0.2 and 1.1.x /// /// Requires the `v102` or `v110` features and OpenSSL 1.0.2 or OpenSSL 1.1.0. /// [OpenSSL Documentation](https://www.openssl.org/docs/man1.1.0/crypto/DH_get_1024_160.html).

sfackler · Sep 29, 2017

openssl/src/dh.rs

@@ -48,7 +84,11 @@ impl Dh {
        }
    }

+    /// return a DH object for the IETF RFC 5114 value.


sfackler · Sep 29, 2017

openssl/src/dh.rs

@@ -57,7 +97,11 @@ impl Dh {
        }
    }

+    /// return a DH object for the IETF RFC 5114 value.


Am I to assume this method is for the 2047 bit MODP group with 224-bit prime order subgroup (and so on)? I want to be sure since this is well outside of my realm of knowledge right now.

Yeah, they all come from that same RFC, just with different sizes.

sfackler · Sep 29, 2017

openssl/src/dh.rs

@@ -67,11 +111,13 @@ impl Dh {
    }
 }

+/// sets rust-openssl to call DH_set0_pqg as defined by fn from_params if using openSSL 1.1.x


These docs won't be rendered anywhere since this is a private module, FYI.

Okay, pulled.

ejmg · Sep 29, 2017

Hey, @sfackler I'm sorry for failing on standards. I'll fix them first thing tomorrow morning. Thanks for the feedback.

sfackler · Oct 9, 2017

Any update here?

ejmg · Oct 21, 2017

@sfackler sorry for completely ghosting on you. Classes have not been the easiest on me so far this semester. I rebased and pushed my changes. Please let me know if all is well this time round. I can get back to you quicker this time if something is awry again.

sfackler/rust-openssl

Issue #704, module `dh` #742

Issue #704, module `dh` #742

ejmg commented Sep 28, 2017

This comment has been minimized.

ejmg commented Sep 28, 2017 •

edited

sfackler requested changes Sep 29, 2017

View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ejmg commented Sep 29, 2017

This comment has been minimized.

sfackler commented Oct 9, 2017

ejmg added some commits Sep 28, 2017

This comment has been minimized.

ejmg commented Oct 21, 2017

sfackler/rust-openssl

Join GitHub today

Issue #704, module `dh` #742

Conversation

ejmg commented Sep 28, 2017

This comment has been minimized.

ejmg commented Sep 28, 2017 • edited

sfackler requested changes Sep 29, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ejmg commented Sep 29, 2017

This comment has been minimized.

sfackler commented Oct 9, 2017

ejmg added some commits Sep 28, 2017

This comment has been minimized.

ejmg commented Oct 21, 2017

ejmg commented Sep 28, 2017 •

edited

sfackler requested changes Sep 29, 2017

View changes