Skip to content

Conversation

simo5
Copy link
Contributor

@simo5 simo5 commented Apr 30, 2015

Adds Key Exchange interface
Implements ECDH KEy Exchange mechanism.
Tests ECDH w/o Key Derivation

@reaperhulk
Copy link
Member

retest this please

@simo5
Copy link
Contributor Author

simo5 commented May 1, 2015

I fixed the code in the docs, all looks good on that front now

@public public changed the title Kex ECDH implementation May 3, 2015
@simo5
Copy link
Contributor Author

simo5 commented May 3, 2015

I intend to add test based on the vectors provided by #1884 and the KDF function provided by #1898 before merging this PR.

@simo5 simo5 force-pushed the KEX branch 2 times, most recently from ec076a6 to 691fdc9 Compare May 6, 2015 20:30
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be separate from the regular exchange_algorithm_supported function?

@simo5
Copy link
Contributor Author

simo5 commented May 13, 2015

I added a ECDHnoKDF KeyDerivationFunction class, let me know what you think about it.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.13%) to 99.87% when pulling 6141ae5 on simo5:KEX into 91ea3a9 on pyca:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.13%) to 99.87% when pulling 6141ae5 on simo5:KEX into 91ea3a9 on pyca:master.

Copy link
Member

Choose a reason for hiding this comment

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

Having spoken with @alex I think it might actually be better to just leave this functionality out entirely until we have a clear use-case for it. How do you feel about that @simo5 ? Are you aware of any cases where you really need access to the raw ECDH output?

Copy link
Member

Choose a reason for hiding this comment

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

If someone really needs the raw output they can always write a NullKDF very
easily.

On Sat, May 16, 2015 at 11:38 AM, Alex Stapleton notifications@github.com
wrote:

In src/cryptography/hazmat/primitives/asymmetric/ec.py
#1882 (comment):

@@ -252,6 +264,28 @@ def init(self, algorithm):
algorithm = utils.read_only_property("_algorithm")

+@utils.register_interface(KeyDerivationFunction)
+class ECDHnoKDF(object):

Having spoken with @alex https://github.com/alex I think it might
actually be better to just leave this functionality out entirely until we
have a clear use-case for it. How do you feel about that @simo5
https://github.com/simo5 ? Are you aware of any cases where you really
need access to the raw ECDH output?


Reply to this email directly or view it on GitHub
https://github.com/pyca/cryptography/pull/1882/files#r30462024.

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is needed mostly for tests, I am not sure anyone should really use this w/o KDF, so I am ok leaving it out of the official API and just adding one in tests.

@codecov-io
Copy link

Current coverage is 99.98%

Merging #1882 into master will not affect coverage as of c9b97a2

@@            master   #1882   diff @@
======================================
  Files          120     120       
  Stmts        12153   12153       
  Branches      1337    1337       
  Methods          0       0       
======================================
  Hit          12151   12151       
  Partial          2       2       
  Missed           0       0       

Review entire Coverage Diff as of c9b97a2

Powered by Codecov. Updated on successful CI builds.

@simo5 simo5 changed the title ECDH implementation Add Key Exchange Agreement Interface Oct 7, 2015
@simo5
Copy link
Contributor Author

simo5 commented Oct 7, 2015

As requested by @reaperhulk splitting this PR in 2 parts.
this part is about the Key Exchange Agremment, and has been appropriately renamed.
A new PR will be opened for the actual ECDH implementation

Copy link
Member

Choose a reason for hiding this comment

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

This needs a .. versionadded:: 1.1

@reaperhulk
Copy link
Member

Otherwise this LGTM based on trawling through the last 9 months of conversation about this. Would like a quick confirmation from @public and @alex though.

Copy link
Member

Choose a reason for hiding this comment

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

What type is this?

This should also describe what the return value is.

Signed-off-by: Simo Sorce <simo@redhat.com>
reaperhulk added a commit that referenced this pull request Oct 8, 2015
Add Key Exchange Agreement Interface
@reaperhulk reaperhulk merged commit 8e69350 into pyca:master Oct 8, 2015
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

6 participants