[#8966] Adding curve25519 and ed25519

[the0id]

This adds curve25519 and ed25519 to Twisted. Cryptography doesn't support 25519 yet, so pyNaCL is required.

[codecov-io]

Codecov Report

Merging #644 into trunk will decrease coverage by 0.37%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##            trunk     #644      +/-   ##
==========================================
- Coverage    91.3%   90.93%   -0.38%     
==========================================
  Files         839      839              
  Lines      148399   148609     +210     
  Branches    12970    13006      +36     
==========================================
- Hits       135502   135141     -361     
- Misses      10821    11356     +535     
- Partials     2076     2112      +36

[glyph]

Thanks for your continued work on Conch's cipher suites.

We should discuss the results of this review, but my general feeling here is that even if the dependency isn't optional, we are 4 mostly copy/pasted algorithms in to needing to refactor the way that conch deals with curves. I think this one might be a bridge too far. Do you see an easy way to modularize? I'm OK with deferring to a subsequent ticket, but I just worry that this is becoming less and less maintainable.

Please address this feedback and resubmit.

[glyph]

See comment in setup.py about being an optional dependency.

[glyph]

Just a nitpick, but, this is perhaps more idiomatically expressed as if b'25519' in self.kexAlg

[the0id]

I remember if X in Y causing problems with python3, but that seems to be incorrect and I agree if X in Y is better. Changed.

[glyph]

This looks like it's reaching into the private implementation details of a dependency. Is it? If so - why does it need to?

[the0id]

It is, and it's because of how pynacl decided to do their secret key generation. They call their method "salsa" which follows the standard way of generating a curve25519 secret key but then throws an extra transform on it. Which means to get it to work with Twisted I have to call the private method.

[glyph]

I don't quite understand; wouldn't "throwing an extra transform" on it mean that the values are different?

[the0id]

Yup. If you use the standard method it will return a value that works for pynacl, but won't work for almost every other implementation, including OpenSSH.

Why they do this I have no idea, I'm sure they have their reasons, but this was the only way I could get it to work.

[glyph]

One other nice thing about making this an optional dependency would be that we could potentially factor out this extremely long function into a couple of smaller cipher-management functions so as to avoid monolithic changes like having to indent this lengthy "else" block even though the lines aren't being touched.

[glyph]

Given that this comment is repeated twice, and now the MP(... expression is repeated 4 times, some refactoring seems to be in order.

[the0id]

I see what you're saying, and have had similar thoughts, but I don't know if I'm convinced it's needed yet.

DH, ECDH, and curve25519 all do something like this, but each of them do it in a slightly different way. So if we were to refactor and abstract this behavior I think we'd just end up moving the same code to a different location.

[glyph]

I realize that this is just repeating an instance of an existing pattern, but could we replace this with something that generates a test key on the fly, rather than supplying static data? Long term, we should be deleting every static private key in this module.

[the0id]

I don't have a strong opinion on whether or not this should be done, but I think it'd be best to do it as it's own ticket.

[glyph]

How is this comparison value "patched"? Also; can this refer more specifically to what does the SecureRandom patching, rather than just alluding to it? Long-term, this is the sort of thing we should parameterize rather than patch.

[the0id]

When trial is run, SecureRandom is patched by test_keys.py:217 to return '\xff' for each byte requested.

I wasn't sure how detailed to go into this explanation in the comment since it's only relevant when running trial. I saw it that it was more important to let a normal user know that every time this function is called the output will be slightly different.

[glyph]

Additional cryptographic dependencies are often met with high levels of scrutiny. If possible, I'd really like to see this implemented in a way which makes pynacl an optional dependency.

This doesn't necessarily mean that the entry in setup.py needs to go away, but it strikes me that "cryptography yes, pynacl no" might end up being a configuration someone cares about.

[glyph]

This is not mandatory feedback, feel free to push back on this, if you disagree. I am not really sure about the political environment re: nacl as a crypto lib. Perhaps @reaperhulk or @alex could opine as well, if this is a legitimate concern.

[reaperhulk]

libsodium is definitely still the "less acceptable to upper management" crypto library, but its presence here is because OpenSSH already extensively uses crypto primitives from djb and OpenSSL doesn't (yet) support them. If you want to support these things in the near term you'll need to pull in pynacl or do it in pure Python (don't do that).

[glyph]

The question is really: should Twisted be concerned about environments where someone will say "uh oh, pynacl, you can't install that, denied"? Or is this the kind of thing that will be configured at a policy level? (After all you'd need to actually use these ciphers in your deployment to exercise this code.)

[reaperhulk]

Good question. I've never been a huge fan of optional deps, but a project like twisted probably does need to care about things like that. It doesn't seem unreasonable to state that a base install of twisted conch must support RSA and NIST ecdsa (via a non-optional cryptography dep), but that users who want access to ed25519 should install pynacl. ed25519 is likely to appear in cryptography soon-ish as well, albeit only accessible to users linking against the latest OpenSSL.

[the0id]

I have issues with how pynacl operates myself, but I found it to be the lesser evil. There are other libraries that support *25519, but some only support curve25519, some only support ed25519, and most of them haven't been updated in at least a year or two.

How I see things is that pynacl is optional to the extent that cryptography and pyasn1 are optional. They're not needed unless you want to use conch, and since *25519 is the default (and strongly encouraged) algorithm of OpenSSH it should be included when conch is.

[glyph]

@the0id I don't think you're grasping my concern here:

Let's say I work at a company that's using Twisted for SSH, but that has a heavy-weight dependency audit process for each new library that gets deployed. Now, we're still adopting new dependencies and causing more trouble for people working at such shops; however, new crypto dependencies are often given much tighter scrutiny. By making this change unconditional, I have to upgrade Twisted and go through the pynacl review process at the exact same time - rather than installing new Twisted immediately and kicking off the review process for pynacl, which, once it gets approved, gives me a new capability.

[the0id]

I see what you're saying now, and making curve25519 an optional dependency may not be a much work as I thought. So I'll look into it.

[glyph]

I should note; it's OK to make the conch extra hard depend on this, since for most users, that should be the right thing. I'm mainly talking about leaving the code in such a state that an escape hatch like a separate conch-no25519 extra would be possiblel.

[alex]

I think you have a merge issue, this appears to be readding back 1024-bit DH and a few other curves we removed.

[the0id]

Now that I've had some time to look at this again, making pynacl an option dependency was pretty easy. With the exception that pyflakes doesn't like how I'm importing nacl.public into _kex.py but not using it.

I see why pyflakes is objecting to this, but I'm doing this to check to see if pynacl is installed before adding curve25519 as a supported kex. I could create a nacl.public object to make pyflakes happy, but that doesn't seem like a good solution to me.

How should I go about this?

[alex]

You can use requireModule for that: https://github.com/twisted/twisted/blob/trunk/src/twisted/conch/test/test_openssh_compat.py#L14

[the0id]

@alex yeah as with our conversation on #749 I think those curves should be in Twisted, and since they were originally included in this request I'm adding them back in.

1024-DH I put back in as well because it was also originally in this merge, and while I know it's been deprecated, it's still supported and I think it's worth supporting here as it's still widely used and I believe Twisted should be as compatible with as many applications as possible.

[the0id]

@alex Oh thanks for the link, I'll look into that. 👍

[alex]

This PR is for adding X25519 and Ed25519 support, disagreements about 1024-bit DH or other curves belong somewhere else, not here.

[the0id]

@alex I guess you're right. I'm reverting those changes and will open a new PR.

[the0id]

I'm not sure if my last commit made things better or worse, but the root of the problem seems to be the same.

The tests are failing because nacl isn't present. Since those tests are skipped, codecov fails. Is there a way to get TravisCI to use nacl without having it be required in _setup.py? How should I do this?

[glyph]

My preference would be for you to add it as a new required dependency of the conch extra, but then add a new extra, conch_no_25519 or somesuch, which doesn't include pynacl. You can then test it both ways by adding tox environments which install each extra.

[the0id]

@glyph I see what you're saying now. No problem, can do, as soon as I figure out how to do. :)

[the0id]

I can't tell if I did this right. The new tests ran but codecov didn't.

[glyph]

Codecov has the occasional reliability issue :-\

[the0id]

I got 99 commits, so lets merge this one.

[alex]

This still needs to be removed.

[alex]

This should just say Ed25519, this module doesn't do anything with X25519

[alex]

The variable name pr has me a bit confused; can we pick something more intuitive?

[alex]

The data will continue ssh-ed25519 right? Might be easier to read if you include that in the string.

[the0id]

It will, and I see what you're saying, but I think it's better how it is. It's a little faster to process and it fits the convention of the rest of the checks. :)

[alex]

These changes appear to be unrelated, there's nothing wrong with just return when you have the value.

[the0id]

Moving the return where it is now fits in better with the block. Each conditional creates a reprstr value, with a single return of that value at the end.

[alex]

This should not use is, it should use ==

[the0id]

Good catch. 👍

[alex]

Conventionally, it's typed Ed25519, lowercase d.

[the0id]

I think it's better to leave this all caps because the rest of the return strings are in all caps.

[alex]

This logic should be split out into its own method for readability.

[alex]

Catch a specific exception, not a bare catch.

[alex]

This can just be b'curve25519' in self.kexAlg

[alex]

There's also a merge conflict.

[rodrigc]

You need to take a merge from trunk and move your 8966.feature file to the newsfragments directory. Things have moved around due to this: #763

[the0id]

@alex, @rodrigc thanks for the review. I've made some of the changes requested, and gave my reasons for not changing the others.

@rodrigc thanks for the head's up about moving the file to newsfragments. @markrwilliams helped me out with the same thing on another PR. :)

[the0id]

The last two times appveyor has run two of the tests fail because openssh can't be installed.

Build started
git clone -q https://github.com/twisted/twisted.git C:\projects\twisted
git fetch -q origin +refs/pull/644/merge:
git checkout -qf FETCH_HEAD
Restoring build cache
Cache 'C:\Users\appveyor\AppData\Local\pip\Cache' - Restored
Running Install scripts
choco install openssh -confirm
Chocolatey v0.10.7
Installing the following packages:
openssh
By installing you accept licenses for the packages.
Progress: Downloading openssh 0.0.18.0... 45%openssh not installed. An error occurred during installation:
 The operation has timed out.
openssh package files install completed. Performing other installation steps.
The install of openssh was NOT successful.
openssh not installed. An error occurred during installation:
 The operation has timed out.
Chocolatey installed 0/1 packages. 1 packages failed.
 See the log for details (C:\ProgramData\chocolatey\logs\chocolatey.log).
Failures
 - openssh (exited 1) - openssh not installed. An error occurred during installation:
 The operation has timed out.
Command exited with code 1

[rodrigc]

See:
https://twistedmatrix.com/pipermail/twisted-python/2017-August/031596.html

[the0id]

@rodrigc Thanks, I missed that message.