Skip to content

Implement exercise diffie-hellman - #756

Merged
ilya-khadykin merged 9 commits into
exercism:masterfrom
kusti8:diffie-hellman
Oct 11, 2017
Merged

Implement exercise diffie-hellman#756
ilya-khadykin merged 9 commits into
exercism:masterfrom
kusti8:diffie-hellman

Conversation

@kusti8

@kusti8 kusti8 commented Oct 6, 2017

Copy link
Copy Markdown

Fixes #747

@kusti8

kusti8 commented Oct 7, 2017

Copy link
Copy Markdown
Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 :)

@kusti8 kusti8 left a comment

Copy link
Copy Markdown
Author

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?

Comment thread exercises/diffie-hellman/README.md Outdated

## 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed.

@N-Parsons N-Parsons left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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