Skip to content

Implement exercise diffie-hellman #756

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

Merged
merged 9 commits into from
Oct 11, 2017
Merged

Implement exercise diffie-hellman #756

merged 9 commits into from
Oct 11, 2017

Conversation

kusti8
Copy link

@kusti8 kusti8 commented Oct 6, 2017


Fixes #747

@kusti8
Copy link
Author

kusti8 commented Oct 7, 2017

Done!

@kusti8 kusti8 changed the title [WIP] Implement exercise diffie-hellman Implement exercise diffie-hellman Oct 7, 2017


def private_key(p):
return random.randint(2, p-1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this should use secrets rather than random, since the exercise is a cryptographic one. Perhaps we should also have a note in README.md that solutions should avoid using random since it's not cryptographically secure?

def private_key(p):
    return 2 + secrets.randbelow(p-2)

Copy link
Author

Choose a reason for hiding this comment

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

I did look at that, but according to the PEP it was introduced in version 3.6, so it isn't compatible with Python 2 or most Python 3 versions on stable distros. It seems that exercism wants cross compatibility which is why I chose to use the default random instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good points!

These are just training exercises, so random is good enough I think.
But it would be a really good idea to mention secrets in HINTS.md for such exercises.

@N-Parsons, could you please create an issue for it to not forget?

Copy link
Author

Choose a reason for hiding this comment

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

I'll add a note to the readme about it and pseudo randomness.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, @kusti8, I hadn't realised that secrets was only in Python 3.6.

@m-a-ge, I'll create an issue for it now :)

Copy link
Author

@kusti8 kusti8 left a comment

Choose a reason for hiding this comment

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

I've added a note about pseudo-randomness. Can you check to make sure it is clear and correct?


## Notes

Python, as of version 3.6, includes two different random modules. The module called `random` is pseudo-random, meaning it does not generate true randomness, but follows and algorithm that simulates randomness. Since random numbers are generated through a known algorithm, they are not truly random. The `random` module is not correctly suited for crypotography and should not be used, because it is pseudo-random. In version 3.6, Python introduced the `secrets` module which is more cryptographically secure and produces more random numbers suited for cryptography. Since this is only an exercise, `random` is fine to use, but note that it would be very insecure if actually used for crypotgraphy.
Copy link
Contributor

Choose a reason for hiding this comment

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

@kusti8 Looks good. My only suggestion would be to reword the penultimate sentence to:

"In version 3.6, Python introduced the secrets module, which generates cryptographically strong random numbers that provide the greater security required for cryptography."

Copy link
Contributor

Choose a reason for hiding this comment

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

There are also a typos in the middle and at the end:
"crypotography" --> "cryptography"
"crypotgraphy" --> "cryptography"

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

@N-Parsons N-Parsons left a comment

Choose a reason for hiding this comment

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

Looks good.

@ilya-khadykin ilya-khadykin merged commit a439912 into exercism:master Oct 11, 2017
@ilya-khadykin
Copy link
Contributor

@N-Parsons thanks for the review

@kusti8 thanks a lot for working on this!

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.

3 participants