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

PKCS#1 2.0: Implementation of MGF1 #89

Merged
merged 1 commit into from
Jun 10, 2017
Merged

PKCS#1 2.0: Implementation of MGF1 #89

merged 1 commit into from
Jun 10, 2017

Conversation

adamantike
Copy link
Contributor

@adamantike adamantike commented Apr 20, 2017

Implementation of the Mask Generation Function MGF1 used in the OAEP encoding step.
For more information, the MGF1 specification is at https://tools.ietf.org/html/rfc2437#section-10.2.1

Note: Still a WIP, but I'm interested in comments about the current state. Please move the discussion about the paths or file organization to any other place, so this PR isn't blocked by that kind of details

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 91.858% when pulling c5340f5 on adamantike:pkcs1-v2-mgf1 into 000e84a on sybrenstuvel:master.

@adamantike
Copy link
Contributor Author

@sybrenstuvel any feedback about this?

@sybrenstuvel
Copy link
Owner

Hey dude! Sorry, had a crazy busy period. I'm looking forward to diving into your code this weekend ;-)

Copy link
Owner

@sybrenstuvel sybrenstuvel left a comment

Choose a reason for hiding this comment

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

Looking good! I only have a few minor comments.

rsa/pkcs1_v2.py Outdated

# If l > 2^32(hLen), output "mask too long" and stop.
if length > (2**32 * hash_length):
raise OverflowError
Copy link
Owner

Choose a reason for hiding this comment

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

Add a message to the OverflowError with description of the error.

rsa/pkcs1_v2.py Outdated
:param hasher: hash function (hLen denotes the length in octets of the hash
function output)

:return: mask, an octet string of length `length`
Copy link
Owner

Choose a reason for hiding this comment

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

Add :rtype: bytes so that it's clear bytes are returned. This also helps IDEs that parse the docstring to perform autocompletion and linting.

rsa/pkcs1_v2.py Outdated
c = transform.int2bytes(counter, fill_size=4)

# Concatenate the hash of the `seed` and C to the octet string `output`
output += pkcs1._hash(seed + c, method_name=hasher)
Copy link
Owner

Choose a reason for hiding this comment

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

This is a bad idea. output is stored using a fixed-length array, and concatenating to that will cause a copy to be made. Instead, append each output of pkcs1._hash() to a list, then return b''.join(thelist).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Moved the whole thing to a joined generator solution

rsa/pkcs1_v2.py Outdated
the output of the mask generation function, which in turn relies on the
random nature of the underlying hash.

:param seed: seed from which mask is generated, an octet string
Copy link
Owner

Choose a reason for hiding this comment

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

Document parameter types using :type:.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"""Tests PKCS #1 version 2 functionality.

Most of the mocked values come from the test vectors found at:
http://www.itomorrowmag.com/emc-plus/rsa-labs/standards-initiatives/pkcs-rsa-cryptography-standard.htm
Copy link
Owner

Choose a reason for hiding this comment

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

Nice!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 91.848% when pulling 816f1d2 on adamantike:pkcs1-v2-mgf1 into 425eb24 on sybrenstuvel:master.

@adamantike
Copy link
Contributor Author

@sybrenstuvel ready to review!

Implementation of the Mask Generation Function `MGF1` used in the OAEP encoding step.
For more information, the MGF1 specification is at https://tools.ietf.org/html/rfc2437#section-10.2.1
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 91.848% when pulling 169e882 on adamantike:pkcs1-v2-mgf1 into 425eb24 on sybrenstuvel:master.

Copy link
Owner

@sybrenstuvel sybrenstuvel left a comment

Choose a reason for hiding this comment

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

Looks very good! I wish all programmers wrote such well-commented and well-motivated code ;-)

Sorry for the wait -- this code is good to go!

@sybrenstuvel sybrenstuvel merged commit d008525 into sybrenstuvel:master Jun 10, 2017
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.

None yet

3 participants