Support for AECDH key exchange on server side #111

mildas · 2016-07-13T18:25:35Z

The main changes are in _serverAnonKeyExchange in parts if cipherSuite in CipherSuite.ecdhAnonSuites:.
else: part is same as it was (ADH support).

This change is

tomato42 · 2016-07-13T18:59:00Z

no need to close the pull request when some test fail :)

It's actually good to keep the same branch name and the same pull request ID for subsequent versions of the code. You can do that by modifying your local commit (either by amending or rebasing) and then force pushing to github.

Now, for actual code: I'd rather not add more code to tlsconnection.py but work to move code out from it. In other words, please first move the already implemented anonymous DH to tlslite.keyexchange module and in another commit(s) add support for new key exchange that uses AECDH (there probably will be some code duplication between RSA_DHE and ADH and similarly between RSA_ECDHE and AECDH, so combining then will be a good idea too).

Finally, in comments you can use FFDHE to differentiate the "old" DH from the new, elliptic curve based, ECDHE. The FF stands for Finite Field and is already somewhat standard designation (see for example draft-ietf-tls-negotiated-ff-dhe-10.

tomato42 · 2016-07-13T18:59:48Z

tlslite/tlsconnection.py

@@ -1697,15 +1701,39 @@ def _serverCertKeyExchange(self, clientHello, serverHello,

    def _serverAnonKeyExchange(self, clientHello, serverHello, cipherSuite, 
                               settings):
+        # Calculate ECDH Xs, Ys
+        print("1")


don't forget to remove it :)

mildas · 2016-07-13T19:36:04Z

Ok :) Should I reopen this pull request?
I tried fix these PEP-8 problems, so I can commit it, but it is probably unnecessary and will be better to make next commit to this pull request after I will move ADH to tlslite.keyexchange, true?
Question about adding ADH (AECDH) to tlslite.keyexchange: Should I add new classes for these anonymous cipher suites (maybe inherite from RSA_DHE/RSA_ECDHE) or better will be add them to existing classes?

tomato42 · 2016-07-13T20:01:08Z

Ok :) Should I reopen this pull request?

yes

I tried fix these PEP-8 problems, so I can commit it, but it is probably unnecessary and will be better to make next commit to this pull request after I will move ADH to tlslite.keyexchange, true?

yes

Question about adding ADH (AECDH) to tlslite.keyexchange: Should I add new classes for these anonymous cipher suites (maybe inherite from RSA_DHE/RSA_ECDHE) or better will be add them to existing classes?

the difference between RSA_DHE and ADH are the calls to signServerKeyExchange and verifyServerKeyExchange so you may want to introduce the new ADH class and make the RSA_DHE inherit from the ADH class, but I haven't thought about it much - the goal is to have little code and make it reusable/testable at the same time without changing the already defined API. If it means that most of the RSA_DHE code lands in the ADH class, that's fine.

mildas · 2016-07-29T17:12:26Z

Sorry, that long time nothing happend. Im almost done with moving ADH to keyexchange, but I wanted to ask, if you please could help me, how to make openssl client with DHE-RSA to try connection with tlslite server. I want to test it before sending it, ADH works good, but I want to test DHE-RSA too, but I dont know how to right generate certificate and use it in openssl s_client. Everything that I tried gave me no peer certificate available or unable to load client certificate private key file

tomato42 · 2016-07-29T18:03:52Z

since tlslite-ng does not verify client certificates, you can use any set of certificates for testing, there is already a set in the tests/ directory

something like this should be enough to test it:

PYTHONPATH=. python scripts/tls.py server -c tests/serverX509Cert.pem \
-k tests/serverX509Key.pem --reqcert localhost:4433

openssl s_client -key tests/clientX509Key.pem -cert tests/clientX509Cert.pem \
-connect localhost:4433 -cipher EDH

mildas · 2016-07-30T00:54:35Z

Ok, thanks. When I tried it like you wrote, it works.
Next question is, how to right add these changes to this pull request? I can make New pull request, but it wont add to this, right? I can also attach files to comment, but it isnt best way in my opinion, right?
Im just asking to be sure, because I dont want to add you next problems, that I will make bad pull request etc. to wrong places.

tomato42 · 2016-07-30T08:02:08Z

first: don't worry, you can't do anything to break my repository (or rather, if you do, tell those people: https://bounty.github.com/ 😄 ), and even in your repository (both local and remote) you can fix most if not all problems (if you're really worried you may loose your work, just copy the whole repository folder, with the hidden .git directory to some other place)

second: you add changes to this pull request by changing your local branch (that usually is just a series of git add and git commit or git gui and few "commit" actions there), then you may want to rebase) this local branch (usually that will be git rebase -i master) and then force pushing to github to overwrite the old remote branch with new fixed commits (in most cases you do that by git push origin aecdh-key-exchange --force while working on the aecdh-key-exchange branch).

alternatively, if you created a new branch to work out the pull request, you can rename local copy of the aecdh-key-exchange branch (git checkout aecdh-key-exchange and git branch -m aecdh-key-exchange-old-obsolete) or just simply remove it completely (git checkout master and git branch -D aecdh-key-exchange). Then you can rename your new branch (not necessary, but keeps things orderly) to the name of the branch of this pull request (git checkout new-hot-stuff then git branch -m aecdh-key-exchange) after all of that you can force push this branch to github as you would in the rebase case above (this is just what the rebase command automatically).

mildas · 2016-07-30T10:05:41Z

Ok, thanks.
I commited it. I inspired at _serverCertKeyExchange, how is there used keyExchange, but I am not sure with:

Part if dh_Yc % dh_p == 0: isnt used at DHE_RSA, but at serverAnonKeyExchange is this checked. So this part is in comment, because Ive got some problem with return with argument inside generator, and wasnt sure, if should I let it here or dont.
ADH got getRandomBytes with constant 32, but DH calculate this with strength, so is it correct, what I have done, or should ADH calculate with strength too?

tomato42 · 2016-07-30T11:04:08Z

tlslite/keyexchange.py


    NOT stable API, do NOT use
    """

    def __init__(self, cipherSuite, clientHello, serverHello, privateKey):
-        super(DHE_RSAKeyExchange, self).__init__(cipherSuite, clientHello,
+        super(ADHKeyExchange, self).__init__(cipherSuite, clientHello,


you need to do the same in makeClientKeyExchange (see Travis-CI errors)

tomato42 · 2016-07-30T11:21:56Z

finally, please merge those two commits together, in git rebase it's the squash command

tomato42 · 2016-07-30T11:25:41Z

one more thing, you may want to read CONTRIBUTING.md as it says how to set up your system so that you can run the tests from the Makefile (using make test-dev) or unittests (using python -m unittest discover -v)

mildas · 2016-07-30T21:08:32Z

I tried to make merge with git rebase and then git push origin aecdh-key-exchange --force, but it added some old commits to this pull request and didnt merge them.
I dont know, what I have done wrong. I have studied this almost all day to do it right and failed... I have explored all github.com, if there isnt any merge button for commits in pull request and nothing.
Sorry for these problems, but I have almost no experiences with git.

tomato42 · 2016-07-31T08:27:15Z

I tried to make merge with git rebase and then git push origin aecdh-key-exchange --force, but it
added some old commits to this pull request and didnt merge them.

hmm, maybe it's because your local master doesn't match my master

I dont know, what I have done wrong. I have studied this almost all day to do it right and failed... I
have explored all github.com, if there isnt any merge button for commits in pull request and nothing.

no, there isn't one, you have to do everything locally

Sorry for these problems, but I have almost no experiences with git.

no problem, nobody is born with the knowledge how to operate git :) and it was one of the reasons why I proposed to Standa that you work on such a relatively simple change code wise, so that you could learn the process

ok, so now I see it, your master doesn't match my master, so the suggestion to rebase on top of master was incorrect, at least for you, sorry about that

so, on https://github.com/mildass/tlslite-ng/network I see that the master and aecdh-key-exchange contain exactly the same commits

If that is not the case locally (you can check that using either gitk --all or tig --all, but you'll need to install those tools, or manually using git rev-parse master, git rev-parse aecdh-key-exchange, they should return the same commit id), then git checkout master to switch to the master branch and git branch work-in-progress to save your current local master as the work-in-progress branch.

it may be good idea to save the state of that branch anyway, as the first commit is correct functionally, and you may want to use it later as a template... (as later rebases will essentially drop it)

once we know that we can operate on master without fear of loosing previous work, we need to update it to my version of master

first, make sure that you're on master, use git status, check if it reports

On branch master

now you have to make a local copy of my repository, to do that, you first need to add a remote:
git remote add tomato42 https://github.com/tomato42/tlslite-ng.git and then download the changes: git fetch tomato42

now your local repository knows how my repository looks like

to switch your local master to the correct position, you can run the following command:
git reset --hard tomato42/master but do note that this is not safe to do in general case, we're doing it only to restore the repository to known good state

(normal workflow is to git fetch tomato42 --prune to get changes from my repository and remove not existing branches, and then git pull tomato42 master to update your local copy, this will never overwrite your local changes and performs checks to make sure that the local version doesn't contain any stray commits)

now that we have updated local master, we need to update your remote master on github: git push origin master --force (again, we're doing --force here only because we're restoring the repo to good state, once that's done, you should be able to do just git push origin master to update remote repository, if you follow this workflow, you should never have a reason to use --force again on master branch)

now we can switch to the feature branch and and combine all the commits together:

git checkout aecdh-key-exchange
git rebase -i master

in the git rebase change pick to fixup for all the commits but the first one, this will squash the commits together, and drop their commit messages (use squash if you want to combine messages, but you will want to edit the commit message anyway, as it won't be adding AECDH any more but refactoring DHE/ADH)

do not push the branch to github yet! if you do, github sometimes gets confused and drops the comments and you then won't know what changes you should do to the code

now's the time to resolve all the comments I provided; edit the code as normal, but instead of doing git commit after git add tlslite/keyexchange.py, do git commit --amend this will combine all the changes to a single commit again (I'm assuming you'll want to perform the ADH move first, and only after that's done, start work on AECDH)

once you think that all the changes are as they should be, run the test suite with make test-dev (see CONTRIBUTING.md for dependencies of the test suite) and make sure the tests pass (at the very least, python2 -m unittest discover -v, python3 -m unittest discover -v and the ./tlstest.py tests should pass)

now, that the commit is updated, and the tests pass, you can force push your local branch to github:
git push origin aecdh-key-exchange --force and you can start working on AECDH :)

note that for feature branches like aecdh-key-exchange, it is normal to --force push them, since you're still modifying the commits and working with a set of patches, it's almost expected that you will depart from how the branch looks remotely

mildas · 2016-08-02T11:43:14Z

Everything looks good, but make test-dev and python2 -m unittest discover -v are still giving me 2 fails, both ImportError: No module named enum. Dunno in which lib is problem. I have got everything from CONTRIBUTING.md except optional modules.
python3 -m unittest discover -v is ok.

tomato42 · 2016-08-02T14:18:29Z

neither tlslite-ng nor ecdsa use the enum module, I don't know where this error could be coming from... does the new code use it?

mildas · 2016-08-02T14:39:31Z

I got this error from tests unit_tests.test_tlslite_utils_constanttime and unit_tests.test_tlslite_utils_cryptomath. It looks that hypothesis lib causes it.
So I think, that isnt problem in new code.

tomato42 · 2016-08-02T14:45:03Z

aah, yes, indeed the new hypothesis requires enum34 package (I had an old version checked out, that's why I didn't find it), but then it should have been installed as a dependency of hypothesis, if it was installed through a package manager... 😕

tomato42 · 2016-08-02T18:07:15Z

tlslite/keyexchange.py

+    def makeServerKeyExchange(self, sigHash=None):
+        """Prepare server side of key exchange with selected parameters"""
+        super(DHE_RSAKeyExchange, self).makeServerKeyExchange()
+        self.signServerKeyExchange(self.serverKeyExchange, sigHash)


why adding a serverKeyExchange field in the object?

anything wrong with this?:

ske = super(DHE_RSAKeyExchange, self).makeServerKeyExchange() self.signServerKeyExchange(ske, sigHash) return ske

super(DHE_RSAKeyExchange, self).makeServerKeyExchange() creates attribute serverKeyExchange. This attribute will be created in both cases, so why cant we use it and do not create new variable ske?

because adding attributes to objects outside constructors is bad form (you can see landscape test complaining about it)

and makeServerKeyExchange() needs to return (and already returns) a ServerKeyExchange object, so there is no need to pass this SKE through a field, it just makes code more complex

Ok, I got it. I'll change it, before working on AECDH.
When I will add changes now, its just on aecdh-key-exchange branch - git add, git commit, git rebase -i master and git push origin aecdh-key-exchange --force, right?

yes, but if you still have just one commit in the branch, you can do also git commit --amend instead of git commit, git rebase -i master (with squash/fixup)

tomato42 · 2016-08-05T13:49:42Z

tlslite/keyexchange.py

+class ADHKeyExchange(KeyExchange):
+    """
+    Handling of anonymous Diffe-Hellman Key exchange
+    FFDHE without signing serverKeyExchange useful for anonymous DH
    NOT stable API, do NOT use


two things, the summary of the documentation string (the first line) should be a single line and be separated by an empty line from the rest of the doc string

while we're editing it, I think we can remove the "Not stable API" warning, it's not the case any more (it was added when the code was in a different module actually)

tomato42 · 2016-08-05T13:51:48Z

sorry, for some reason github didn't notify me of the new pull request, please mention/ping me (@tomato42) if I haven't provided a review on a patch in 24h (especially during the week)

tomato42 · 2016-08-08T18:06:23Z

looks good 👍

you may want to rebase on top of my master branch after you've created additional commits - travis will then finish tests much quicker

mildas · 2016-08-09T08:03:02Z

You mean git rebase -i master and then git push origin aecdh-key-exchange --force, when I am at master branch?

tomato42 · 2016-08-09T08:22:03Z

yes, but first you need to go to your master branch and update it to my master, so it will be

git checkout master
git pull tomato42 master
git push origin master

then switch to the feature branch, and rebase and push changes:

git checkout aecdh-key-exchange
git rebase master # since there's no editing necessary, you can drop the "interactive" switch
git push origin aecdh-key-exchange --force

Adds ADHKeyExchange class, where is ADH from tlsconnection.py, from which DHE_RSAKeyExchange inherit.

mildas · 2016-08-10T08:03:39Z

Is it now ok and ready to start working on AECDH?

mildas · 2016-08-11T07:26:50Z

@tomato42 Is it rebased, how you have meant? I'm not sure, because I think that it is same as every rebase, that I have done after commit.

tomato42 · 2016-08-11T09:10:05Z

you could have started working on AECDH right away, the rebase -i allows you to modify previous commits, so even if we had anything to left to change in the already present commit, we could have done that, just remember to add AECDH work as another commit, not modification of existing one (don't use --amend or git rebase -i with fixup or squash)

and yes, this is what I meant, if you go to the commit: mildas@76cbe31 , on the upper right side you'll see the commit id, to left of it is "1 parent" with a link to other commit, it now points to f9950c8, which is a new commit in my repo, and in Travis you can see that the tests finished in 3 minutes per configuration, not 25 minutes per configuration

mildas · 2016-08-11T09:20:18Z

So now just git commit and git push origin aecdh-key-exchange --force at aecdh-key-exchange branch, am I right?

tomato42 · 2016-08-11T10:12:01Z

in general yes, but if you don't modify the existing commit, you shouldn't need to use --force to git push: since you have not rewritten history, you don't need to update past commits on the server, and that's what --force does

tomato42 · 2016-08-13T17:41:43Z

tlslite/tlsconnection.py

+                                                      cipherSuite):
+                if result in (0,1): yield result
+                else: break
+            premasterSecret = result


everything from for result ... is the same as in the ADH, the code can reuse it

mildas · 2016-08-14T00:24:19Z

If I git rebase -i master now, wont it rebase both commits(Moves ADH...+Adds AECDH...) to one?

tomato42 · 2016-08-14T08:04:14Z

only if you set the action for the second action to squash or fixup, but since you need to modify just the latest patch, you can do git commit --amend

mildas · 2016-08-17T11:42:58Z

@tomato42 What do you think about it?
Codeclimate says that makeServerKeyExchange is similar in DHE_RSAKeyExchange and ECDHE_RSAKeyExchange, but can I do anything with it?

tomato42 · 2016-08-21T10:06:28Z

we could make an abstract class and use multiple inheritance, but I don't think it's worth it this time, we can rethink it with addition of ECDSA.

I'll do the review on Monday

tomato42 · 2016-08-23T17:55:21Z

tlslite/keyexchange.py

+    """
+    def __init__(self, cipherSuite, clientHello, serverHello, acceptedCurves):
+        super(AECDHKeyExchange, self).__init__(cipherSuite, clientHello,
+                                               serverHello)
 #pylint: enable = invalid-name


since code is not disabling the invalid-name check, it doesn't need to re-enable it

tomato42 · 2016-08-23T18:08:22Z

sorry for the delay in review. Those nits are all of the problems, please fix them and next message will be that the PR is merged 😄

…connection.py

tomato42 · 2016-08-25T18:05:08Z

Reviewed 1 of 2 files at r11, 1 of 1 files at r12.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

tomato42 · 2016-08-25T18:06:17Z

Thank you!

mildas closed this Jul 13, 2016

tomato42 reviewed Jul 13, 2016
View reviewed changes

mildas reopened this Jul 13, 2016

tomato42 reviewed Jul 30, 2016
View reviewed changes

mildas force-pushed the aecdh-key-exchange branch from 6bbb1d3 to 49e3ce9 Compare July 30, 2016 20:58

mildas force-pushed the aecdh-key-exchange branch from 49e3ce9 to c9d2b55 Compare August 2, 2016 15:19

tomato42 reviewed Aug 2, 2016
View reviewed changes

tomato42 reviewed Aug 5, 2016
View reviewed changes

mildas force-pushed the aecdh-key-exchange branch from ea91eba to 9ee3935 Compare August 8, 2016 16:17

Moves ADH from tlsconnection.py to keyexchange.py

76cbe31

Adds ADHKeyExchange class, where is ADH from tlsconnection.py, from which DHE_RSAKeyExchange inherit.

mildas force-pushed the aecdh-key-exchange branch from 9ee3935 to 76cbe31 Compare August 9, 2016 08:27

tomato42 reviewed Aug 13, 2016
View reviewed changes

mildas force-pushed the aecdh-key-exchange branch 3 times, most recently from 9e42dce to 732ea1d Compare August 17, 2016 07:48

tomato42 reviewed Aug 23, 2016
View reviewed changes

Adds AECDH key exchange class to keyexchange.py, which is used in tls…

632099d

…connection.py

mildas force-pushed the aecdh-key-exchange branch from 732ea1d to 632099d Compare August 24, 2016 20:50

tomato42 merged commit 05f4f48 into tlsfuzzer:master Aug 25, 2016

tomato42 mentioned this pull request Aug 25, 2016

add support for AECDH key exchange on server side #53

Closed

Support for AECDH key exchange on server side #111

Support for AECDH key exchange on server side #111

Conversation

mildas commented Jul 13, 2016 • edited by tomato42 Loading

tomato42 commented Jul 13, 2016

Choose a reason for hiding this comment

mildas commented Jul 13, 2016

tomato42 commented Jul 13, 2016

mildas commented Jul 29, 2016

tomato42 commented Jul 29, 2016

mildas commented Jul 30, 2016

tomato42 commented Jul 30, 2016

mildas commented Jul 30, 2016 • edited Loading

Choose a reason for hiding this comment

tomato42 commented Jul 30, 2016

tomato42 commented Jul 30, 2016

mildas commented Jul 30, 2016

tomato42 commented Jul 31, 2016

mildas commented Aug 2, 2016

tomato42 commented Aug 2, 2016

mildas commented Aug 2, 2016

tomato42 commented Aug 2, 2016

Choose a reason for hiding this comment

mildas Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomato42 commented Aug 5, 2016

tomato42 commented Aug 8, 2016

mildas commented Aug 9, 2016

tomato42 commented Aug 9, 2016

mildas commented Aug 10, 2016

mildas commented Aug 11, 2016

tomato42 commented Aug 11, 2016

mildas commented Aug 11, 2016 • edited Loading

tomato42 commented Aug 11, 2016

Choose a reason for hiding this comment

mildas commented Aug 14, 2016 • edited Loading

tomato42 commented Aug 14, 2016

mildas commented Aug 17, 2016 • edited Loading

tomato42 commented Aug 21, 2016

Choose a reason for hiding this comment

tomato42 commented Aug 23, 2016

tomato42 commented Aug 25, 2016

tomato42 commented Aug 25, 2016

mildas commented Jul 13, 2016 •

edited by tomato42

Loading

mildas commented Jul 30, 2016 •

edited

Loading

mildas Aug 2, 2016 •

edited

Loading

mildas commented Aug 11, 2016 •

edited

Loading

mildas commented Aug 14, 2016 •

edited

Loading

mildas commented Aug 17, 2016 •

edited

Loading