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

FFDHE: add new class for accessing RFC 7919 Diffie-Hellman parameters. #3183

Merged
merged 2 commits into from Jul 28, 2017

Conversation

@mkrautz
Copy link
Member

commented Jul 18, 2017

This PR adds a new generator script that generates a FFDHETable.h. This file contains static string data of the Diffie-Hellman parameters from RFC 7919 in PEM form.

The PR also implements a new class, FFDHE, which exposes a static method PEMForNamedGroup. This static method can be used to look up the PEM data for a specific RFC 7919 group by using the name from RFC 7919. (Such as 'ffdhe2048').

This is the first step in the process of allowing Murmur users to use the RFC 7919 Diffie-Hellman parameters easily in murmur.ini. (For example, via sslDHParams=@ffdhe2048). Once this groundwork has been laid, we can start that piece of the puzzle.

@mkrautz mkrautz force-pushed the mkrautz:ffdhe branch 5 times, most recently from 369e056 to e86c89f Jul 18, 2017

@mkrautz mkrautz changed the title WIP: FFDHE FFDHE: add new class for accessing RFC 7919 Diffie-Hellman parameters. Jul 18, 2017

@mkrautz mkrautz requested review from Kissaki, davidebeatrici and hacst Jul 18, 2017

@mkrautz mkrautz force-pushed the mkrautz:ffdhe branch from e86c89f to 7d8683a Jul 18, 2017

@Kissaki
Copy link
Member

left a comment

Typo in first commit message "Diffie-Hellan"

# For the INTEGER type (identifier 0x02), the content
# bytes are simply the bytes of the integer itself in
# two's complement form, stored in big-endian order.
# As such, the numbers can be both signed an unsigned.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 21, 2017

Member

typo an -> and

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 21, 2017

Author Member

Fixed.

'''
out = ''
for c in s:
if c == ' ' or c == '\n' or c == '\r':

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 21, 2017

Member

Does python not provide an adequate whitespace removal function? From the function comment I expected more whitespace to be removed (e.g. tabs).

This comment has been minimized.

Copy link
@bendem

bendem Jul 21, 2017

Contributor

it has strip, lstrip and rstrip

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 21, 2017

Author Member

trim is now gone, part of dehexify.

which is 128 decimal.
'''
if size > 127:
hexString = hex(int(size), )[2:]

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 21, 2017

Member
hex(int(size), )

What's this syntax?

This comment has been minimized.

Copy link
@bendem

bendem Jul 21, 2017

Contributor

Transforms a string to an int then to back to a string in it's hex form. The trailing comma might be a leftover from a removed parameter.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 21, 2017

Author Member

Removed the comma.

buf.append(int(hexString[2 * idx : 2 * idx + 2], 16))
return bytearray(buf)

def derLength(size):

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 21, 2017

Member

This functions complexity is so high I’d really like some tests for it. :)
Would also document the different cases instead of the pure text documentation.
We don’t have tests for scripts/python yet though.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 21, 2017

Author Member

I've added doctests. They're run before the output file is written (it's the first thing in main), so if it fails, it won't generate the file.

WDYT?

if nbytes > 127:
raise Exception('unencodable')
buf = dehexify(hexString)
out = bytearray((0x80 | nbytes,))

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 21, 2017

Member

What does this comma syntax do?
(0x80 | nbytes,)

This comment has been minimized.

Copy link
@bendem

bendem Jul 21, 2017

Contributor

without the comma, the parenthesis are interpreted as an expression surrounded by parenthesis. The trailing comma makes it a one element tuple.


f.write(hdr)
f.write('\n')
f.write('// Automatically generated by "generate-ffdhe.py". DO NOT EDIT BY HAND.\n')

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 21, 2017

Member

Can we print the file name through a self reference/variable?

This comment has been minimized.

Copy link
@bendem

bendem Jul 21, 2017

Contributor

That would be __file__.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 21, 2017

Author Member

Done.

f.write('\n')
f.write('// Automatically generated by "generate-ffdhe.py". DO NOT EDIT BY HAND.\n')
f.write('//\n')
f.write('// I also agree to have manually vetted this file for correctness.\n')

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 21, 2017

Member

What? Who is I? Also to what?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 21, 2017

Author Member

Removed the extra text.

f.write('// If I do not agree, I will not have removed the line above saying\n')
f.write('// otherwise. Nor will I have removed the line below this one which\n')
f.write('// will cause a preprocessor error. Oops!\n')
f.write('#error Please verify this file is correct\n')

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 21, 2017

Member

Do we generate the file and then the committer has to verify the output and have to modify the auto-generated files comments??
Should be documented. And I don't really like/understand the two-split (non-)agreement. Code comments should not talk about the author as I.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 21, 2017

Author Member

Removed the extra text.

derUnsignedInteger(trim(ffdhe8192_p)),
derUnsignedInteger(trim(g)),
))
))

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 21, 2017

Member

Redundant block structure could be put into a function with parameter f, ffdhePamName, p, g.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 21, 2017

Author Member

Hmmm... Is it that bad? I don't think I personally would factor it out into a function. (Which is why I didn't do it in the first place.)

containing the number.
'''
hexString = trim(hexString)
nbytes = int(len(hexString) / 2)

This comment has been minimized.

Copy link
@bendem

bendem Jul 21, 2017

Contributor

integer division is //. len(hexString) // 2

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 21, 2017

Author Member

Done.

src/FFDHE.h Outdated
/// group with the given name, such as "ffdhe2048",
/// "ffdhe4096", etc.
///
/// Returns an empty and null byte array if the request

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 21, 2017

Member

empty and null byte array?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 21, 2017

Author Member

It returns a null bytearray, but a null bytearray is also empty...
Should I just say null?

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 21, 2017

Member

Qt documents it as

Constructs an empty byte array.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 21, 2017

Author Member

Also: "We recommend that you always use isEmpty() and avoid isNull()."

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 21, 2017

Author Member

I'll change it to say empty.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 21, 2017

Author Member

Fixed.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 21, 2017

Member

Is null even a thing for QByteArray? You are referring to its data struct behind the visible QByteArray being null, right? Like on QString? But QString explicitly talks about that

Constructs a null string. Null strings are also empty.

while QByteArray does not expose that information.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 23, 2017

Author Member

Yes it is. From the docs:

For historical reasons, QByteArray distinguishes between a null byte array and an empty byte array. A null byte array is a byte array that is initialized using QByteArray's default constructor or by passing (const char *)0 to the constructor. An empty byte array is any byte array with size 0. A null byte array is always empty, but an empty byte array isn't necessarily null:

QByteArray().isNull();          // returns true
QByteArray().isEmpty();         // returns true

QByteArray("").isNull();        // returns false
QByteArray("").isEmpty();       // returns true

QByteArray("abc").isNull();     // returns false
QByteArray("abc").isEmpty();    // returns false

All functions except isNull() treat null byte arrays the same as empty byte arrays. For example, data() returns a pointer to a '\0' character for a null byte array (not a null pointer), and QByteArray() compares equal to QByteArray(""). We recommend that you always use isEmpty() and avoid isNull().

buf = []
for idx in range(0, nbytes):
buf.append(int(hexString[2 * idx : 2 * idx + 2], 16))
return bytearray(buf)

This comment has been minimized.

Copy link
@bendem

bendem Jul 21, 2017

Contributor

I'm confused as to what this function does, is it the same as this?

bytearray.fromhex(' '.join(line.strip() for line in ffdhe6144_p.splitlines()))

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 21, 2017

Author Member

Converted dehexify to use that construct. Dropped trim function because it is now unused.

@mkrautz mkrautz force-pushed the mkrautz:ffdhe branch from 0e1490c to 7531e68 Jul 21, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2017

Typo in first commit message "Diffie-Hellan"

Fixed.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2017

(I've done follow-up commits so it's easier to review... Let's squash at the end?)

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2017

Added a few more commits.

# bignum that we can pass to dehexify.
#
# Strip '0x' prefix from the hex string.
hexString = hex(int(size))[2:]

This comment has been minimized.

Copy link
@bendem

bendem Jul 21, 2017

Contributor

if size isn't already an int, your comparison above is wrong.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 22, 2017

Author Member

Fixed, PTAL.

elif size > 0:
# Short form is simply the number itself,
# as a byte.
return bytearray((int(size),))

This comment has been minimized.

Copy link
@bendem

bendem Jul 21, 2017

Contributor

same here

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 22, 2017

Author Member

Fixed, PTAL.

@bendem

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

Left a comment about a conversion from int to int. Looks good from a non-expert point of view otherwise. 👍

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2017

FWIW, there are also clarifications I can make to the comment at the top of the file. I read through it yesterday and thought it could do better in some places.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2017

@Kissaki @bendem PTAL, I've also streamlined the comment block at the start of the file to be more streamlined.

@hacst
hacst approved these changes Jul 27, 2017
Copy link
Member

left a comment

LGTM. Verified data was sourced from RFC and conversion does actually encode to the values in the header the way the standards describe.

To make doubly sure I decoded the base64 asn.1 from the generated header using https://holtstrom.com/michael/tools/asn1decoder.php and checked that g=2 and p has the expected value using

def toint(s): return int( s.replace(' ', '').replace('\n',''), 16)
a = toint("""<p-stringfromrfc>""")
b = <integervaluefromasn.1decoder>
assert(a == b)

on each of the groups.

Here's a signature for the FFDHETable.h I created locally with LF line endings which matches the one currently in this PR (sha256 is 3777965a3d49a1be34b42201ce9a18448de412f8910db19fa6a3ac5eb9944921):

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (MingW32)

iQIcBAABAgAGBQJZelobAAoJEEP28iRwF8dotuwQAMx6Dp1bkDjAsFYQOnuA4o/z
TRKnV4tr8+Tfbaa4XVD/E0CK6ExDLgEhu3g2N6DvP4T8gMpCWxk5f5oZsyAiG5a0
1b1XVNAxt2IBAyWXlgSu1opxmn1iwg9tOhw5IAPW95tSRw5qaONDH9voPdP3AYnC
XGwyKN1lz6xilTKSfkwaThq8ENsnOn23I/awE2R9PHEGM4Z2fyWhNUEPm1EeCFsf
MSinIodtSphIviDZfGgZJC4y1OKbE+SBoUSfvVvRwGs/vpad3ygB8/SpHj7C+RcL
nRy1jhm1iBRemqO9ETPK3rshDdWXiyUpo5aMsjXIByspBuxWn1R0gQscKGFM+NPX
PKF56hfm1eXF9Eqm9lEBJG6ZY28atbyl3ZusFXyxpRg/w3r+Idr/pAg1tRU6xoDL
RtpLPzGWBo7FKkQoim73ERy7gGxxnNrUgBpgK0JZRfjN+lAiII+NYYE8msuMBMYl
QEOSBwUZDYGd50RitWwziCw8idci5xuRA9rP9nklXY7jipab5kEebDF9CKIL8OFd
QRRzAdk2bm7XyfPkSC2UIQPFyb1lLACre/Q15B7RMTg8G+C0ZuctdfYosqvac2u0
ud6CPY0rNCsMnJebdoer88aF0Ia5dMB8djdpokACazeE9HcI6tFPyjeRngEqiKj5
8K6V11dRcn0wwcNbObLv
=PFAt
-----END PGP SIGNATURE-----

Nice work.

Suggestions have been implemented.

scripts: add generate-ffdhe.py for generating FFDHETable.h.
This commit adds a new script for generating PEM forms
of the Diffie-Hellman groups in RFC 7919.

RFC 7919 specifies a set of Diffie-Hellman groups that
can be negotiated by TLS 1.1 and 1.2 servers and clients
using the same mechanism that is used for named elliptic
curves in TLS.

Our TLS implementation doesn't support RFC 7919 at present,
but we can still use the groups defined in the RFC as good
reference Diffie-Hellman parameters for Murmur, since they
are audited and may be more resistant to attacks than ones
randomly generated.
FFDHE: new class for accessing RFC 7919 FFDHE parameters.
This commit adds a new class that gives access to the
Diffie-Hellman parameters from RFC 7919.

The class exposes a static function, PEMForNamedGroup, which
looks up a set of Diffie-Hellman parameters in PEM form by
the name used in RFC 7919.

For example, to get the PEM form of 'ffdhe2048', you would
call

	QByteArray pem = FFDHE::PEMForNamedGroup(QLatin1String("ffdhe2048"));

With that in hand, you can pass the returned PEM data to
QSslDiffieHellmanParameters to construct an object that can be set
on a QSslSocket-based TLS server.

@mkrautz mkrautz closed this Jul 27, 2017

@mkrautz mkrautz force-pushed the mkrautz:ffdhe branch from a031a09 to 5b82a7a Jul 27, 2017

@mkrautz mkrautz reopened this Jul 27, 2017

@mkrautz mkrautz merged commit 36cb960 into mumble-voip:master Jul 28, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.