Page MenuHomePhabricator

ecdh: enable ecdh_sha2_nistp{384,521} kex methods
AbandonedPublic

Authored by asn on Aug 20 2017, 7:35 PM.

Details

Reviewers
aris
simonsj
Summary

Enable the ecdh_sha2_nistp{384,521} key exchange methods.

The ecdh_sha2_nistp256 method is already enabled, and this
patch adds the {384,521} variants and updates the existing
test cases.

Based on Dirkjan's original patch series here:

Here the changes are adapted for the current master
branch, and expanded to include libgcrypt support.

Co-Authored-By: Dirkjan Bussink <d.bussink@gmail.com>
Signed-off-by: Jon Simons <jon@jonsimons.org>

Test Plan
  • Ran pkd tests for libcrypto and libgcrypt builds.
  • Ran client torture_algorithms.c tests for libcrypto and libgcrypt builds.
  • Tested across multiple libgcrypts ("1.6.3" and "1.7.6-beta").
  • cmake fix enables testing with libgcrypt version "1.7.6-beta" (Debian stretch).
  • .arcconfig fix appeases the arc tool.

Diff Detail

Repository
rLIBSSH libssh
Branch
simonsj/patch/8-20-2017/enable-ecdh-sha2-nistp-384-and-521-methods
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9
Build 9: arc lint + arc unit

Event Timeline

simonsj created this revision.Aug 20 2017, 7:35 PM
simonsj edited the test plan for this revision. (Show Details)Aug 20 2017, 7:40 PM
asn edited edge metadata.Aug 21 2017, 9:35 AM

This looks good so far. I can see that Differential knows the 'git history' and the individual commits. However if I download it with 'arc patch D5' I only get it as one big patch and not as individual commits. I need to check if it is possible somehow to download each commit.

In D5#63, @asn wrote:

This looks good so far. I can see that Differential knows the 'git history' and the individual commits. However if I download it with 'arc patch D5' I only get it as one big patch and not as individual commits. I need to check if it is possible somehow to download each commit.

I was able to figure out this much to get a feel for what the commits would look like if they are pushed using arc land:

I set up a dummy "local" git remote:

$ git remote show simonsj-local | grep URL
  Fetch URL: file:///home/simonsj/code/libssh
  Push  URL: file:///home/simonsj/code/libssh

Ensure there's a local branch that matches the branch in the uploaded diff. Here is simonsj/patch/8-20-2017/enable-ecdh-sha2-nistp-384-and-521-methods at b86ae55c1ec535a87b8aed42ef2c97b867774938:

$ git checkout -b simonsj/patch/8-20-2017/enable-ecdh-sha2-nistp-384-and-521-methods b86ae55c1ec535a87b8aed42ef2c97b867774938
Switched to a new branch 'simonsj/patch/8-20-2017/enable-ecdh-sha2-nistp-384-and-521-methods'

$ git rev-parse simonsj/patch/8-20-2017/enable-ecdh-sha2-nistp-384-and-521-methods
b86ae55c1ec535a87b8aed42ef2c97b867774938

Here is a local test master branch set to the current upstream master, I will arc land to this to see what the result looks like:

$ git fetch simonsj-local
# ...

$ git rev-parse simonsj-local-test-master
74d17a6531517d6fcd5aa0505063a0beb52806e8

Try arc land --remote <my-local-remote> <diff-branch-name> --onto <my-local-test-master-branch>:

$ arc land --remote simonsj-local simonsj/patch/8-20-2017/enable-ecdh-sha2-nistp-384-and-521-methods --onto simonsj-local-test-master
 TARGET  Landing onto "simonsj-local-test-master", selected by the --onto flag.
 REMOTE  Using remote "simonsj-local", selected by the --remote flag.
 FETCH  Fetching simonsj-local/simonsj-local-test-master...
These commits will be landed:

      - b86ae55 .arcconfig: fix missing ',' syntax error
      - 8e9ac3c cmake: relax GCRYPT_VERSION regex
      - fe1af6f torture_algorithms: deduplicate kex method passes
      - 90dcd28 ecdh: enable ecdh_sha2_nistp{384,521} kex methods



    Revision 'D5: ecdh: enable ecdh_sha2_nistp{384,521} kex methods' has not
    been accepted. Continue anyway? [y/N] y

Landing revision 'D5: ecdh: enable ecdh_sha2_nistp{384,521} kex methods'...
 BUILDS PASSED  Harbormaster builds for the active diff completed successfully.
 PUSHING  Pushing changes to "simonsj-local/simonsj-local-test-master".
Counting objects: 1, done.
Writing objects: 100% (1/1), 742 bytes | 0 bytes/s, done.
Total 1 (delta 0), reused 0 (delta 0)
To file:///home/simonsj/code/libssh
   74d17a6..0562515  0562515ca17ead53a82c870bc3ecdf849e21a6cb -> simonsj-local-test-master
 UPDATE  Local "simonsj-local-test-master" tracks target remote "simonsj-local/simonsj-local-test-master", checking out and pulling changes.
 PULL  Checking out and pulling "simonsj-local-test-master".
Cleaning up branch "simonsj/patch/8-20-2017/enable-ecdh-sha2-nistp-384-and-521-methods"...
(Use `git checkout -b simonsj/patch/8-20-2017/enable-ecdh-sha2-nistp-384-and-521-methods b86ae55c1ec535a87b8aed42ef2c97b867774938` if you want it back.)
 DONE  Landed changes.

Seems to work, now looking at what branch simonsj-local-test-master looks like:

$ git rev-parse HEAD
0562515ca17ead53a82c870bc3ecdf849e21a6cb

# ^ it was updated, that's good

$ git log --oneline | head -n7
0562515 ecdh: enable ecdh_sha2_nistp{384,521} kex methods
74d17a6 arcconfig: Add missing comma
b86ae55 .arcconfig: fix missing ',' syntax error
8e9ac3c cmake: relax GCRYPT_VERSION regex
fe1af6f torture_algorithms: deduplicate kex method passes
90dcd28 ecdh: enable ecdh_sha2_nistp{384,521} kex methods
b9b89ef arc: Make the history immutable

# ^ Good: we have separate git commits that
#   come from what was originally uploaded.
#
# ^^ Commit 0562515 is a new merge commit that is not
#    in the original upload.  The text of that commit
#    looks like it matches what was uploaded as the
#    original description, including the 'Summary',
#    'Test Plan', 'Reviews',  'Subscribers', and a
#    'Differential Revision' field filled out.

There is an arc land --preview, but it only emits a summary of the commits that will be included (doesn't give a good git view into what would happen).

There is an arc land --hold which seems to prepare a branch for pushing and will leave the workspace in a detached HEAD state (it won't do the push). I think it could be used to achieve the same effect as above ^.

asn added a comment.Aug 22 2017, 8:26 AM

The problem is arc diff here. It uploads one big patch from a branch instead of the small logical commits you made. It doesn't create a Differential Task for each commit, you have to do that on your own with:

git rebase -x arc diff HEAD^
In D5#65, @asn wrote:

The problem is arc diff here. It uploads one big patch from a branch instead of the small logical commits you made. It doesn't create a Differential Task for each commit, you have to do that on your own with:

git rebase -x arc diff HEAD^

Okay here's what I've done -- I rebased my commits locally and amended all of their messages to include a Test Plan: ... section (otherwise arc complains).

I then ran git rebase -i --exec 'arc diff --use-commit-message HEAD HEAD^' upstream/master, which resulted in these three new revisions (I dropped the fix for .arcconfig as it was pushed to master yesterday):

^ I believe this will work to obtain the individual commits, but it seems like it will be heavy-weight for larger branches with more commits.

asn commandeered this revision.Aug 24 2017, 6:26 PM
asn abandoned this revision.
asn edited reviewers, added: simonsj; removed: asn.