Add support for ECDSA key sizes 384 and 521 alongside the existing 256. #731

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@CrazyCasta
Contributor

Previously only 256-bit was handled and in certain cases (private key
reading) 384- and 521-bit keys were treated as 256-bit keys causing
silent errors.

Tests have been added to specifically test the 384 and 521 keysizes. As
RFC 5656 defines 256, 384, and 521 as the required keysizes this seems a
good set to test. Also, this will cover the branches at ecdsakey.py:55.
Test keys were renamed and test_client.py was modified as a result.

This also fixes two bugs in ecdsakey.py. First, when calculating bytes
needed to store a key, the assumption was made that the key size (in
bits) was divisible by 8 (see line 137). This has been fixed by rounding
up (wasn't an issue as only 256-bit keys were used before). Another bug
was that the key padding in asbytes was being done backwards (was
padding on current_length - needed_length bytes).

@CrazyCasta CrazyCasta Add support for ECDSA key sizes 384 and 521 alongside the existing 256.
Previously only 256-bit was handled and in certain cases (private key
reading) 384- and 521-bit keys were treated as 256-bit keys causing
silent errors.

Tests have been added to specifically test the 384 and 521 keysizes. As
RFC 5656 defines 256, 384, and 521 as the required keysizes this seems a
good set to test. Also, this will cover the branches at ecdsakey.py:55.
Test keys were renamed and test_client.py was modified as a result.

This also fixes two bugs in ecdsakey.py. First, when calculating bytes
needed to store a key, the assumption was made that the key size (in
bits) was divisible by 8 (see line 137). This has been fixed by rounding
up (wasn't an issue as only 256-bit keys were used before). Another bug
was that the key padding in asbytes was being done backwards (was
padding on current_length - needed_length bytes).
3924421
@bitprophet
Member

Thanks for this!

Is it feasible for you to split out the bugfixes to another PR (based on the 1.15 branch)? It'd be nice to make those available to folks not on the upcoming 2.x line.

@bitprophet bitprophet added the Feature label Apr 25, 2016
@bitprophet bitprophet added this to the 2.0 milestone Apr 25, 2016
@CrazyCasta
Contributor

The bugs that I refer to don't exist prior to the switch to cryptography.

@bitprophet
Member

Ah, that wasn't clear from your description, sorry. Works for me then, will get on merging this post-haste.

@bitprophet
Member
bitprophet commented Apr 26, 2016 edited

Confirmed merge OK, tests pass locally, did a quick edit/format pass.

I need to write up a changelog entry:

  • List both this and #611, crediting both parties as usual
  • Probably no need to list the bug-fixes as they were never public/released
  • Annotate with "this is in 2.0, but not 1.17", which I can only do once I square bitprophet/releases#45 (it's next!)
@alex alex commented on the diff Apr 27, 2016
paramiko/ecdsakey.py
numbers = ec.EllipticCurvePublicNumbers(
- x=inflate_long(pointinfo[1:1 + curve.key_size // 8], always_positive=True),
- y=inflate_long(pointinfo[1 + curve.key_size // 8:], always_positive=True),
+ x=inflate_long(pointinfo[1:1 + key_bytes],
+ always_positive=True),
+ y=inflate_long(pointinfo[1 + key_bytes:],
+ always_positive=True),
curve=curve
)
@alex
alex Apr 27, 2016 Contributor

If your'e willing to depend on a recent pyca/cryptography, this mess can be replace with ec.EllipcticCurvePublicNumbers.from_encoded_point(curve, pointinfo). I can send a PR to do that separately if you'd like.

@alex
alex Apr 27, 2016 Contributor

(Where recent means "released 6 months ago")

@bitprophet
bitprophet Apr 28, 2016 Member

@alex Given we have no public releases relying on pyca/cryptography yet (& assuming it's always trended towards "easier to install" over time :D) I'm totally fine with modernizing the requires line in setup.py to anything stable.

@alex
alex Apr 28, 2016 Contributor

PR sent #733

@bitprophet bitprophet added a commit that referenced this pull request Apr 28, 2016
@bitprophet bitprophet Formatting tweaks re #731 fdfbdbb
@bitprophet
Member
bitprophet commented Apr 28, 2016 edited

@alex, @CrazyCasta - I just merged #733 into master, then merged that new master into a copy of this here PR, and 4 of the pkey tests are failing (all ones using ecdsakey.py of course).

My merge commit looked pretty innocuous (just accounting for how alex replaced a small chunk w/ new crypto.io-using code) so I don't think it was that, but not sure.

See https://travis-ci.org/paramiko/paramiko/builds/126447338 - those are the same 4 errors I get locally.

Presumably the easiest way forward is for @CrazyCasta to update this branch vs latest master so the tests pass. But if @alex figures it out first I can apply whatever patch he generates & merge my integration branch to master, which should get us over the hump.

(Minor quirk: my local copy of master is ahead of upstream, but it's all just changelog crap that I don't want hitting the website yet.)

@alex
Contributor
alex commented Apr 28, 2016

@bitprophet the problem is https://github.com/paramiko/paramiko/compare/731-int#diff-00344f8a080e03112e93210cdfd37302R134, that should be s/ec.SECP256R1()/self.ecdsa_curve.curve_class.

@reaperhulk

@alex We want an instance so it should be self.ecdsa_curve.curve_class() right?

@alex
Contributor
alex commented Apr 28, 2016

Uhh, yeah.

On Thu, Apr 28, 2016 at 3:45 PM, Paul Kehrer notifications@github.com
wrote:

@alex https://github.com/alex We want an instance so it should be
self.ecdsa_curve.curve_class() right?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#731 (comment)

"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: D1B3 ADC0 E023 8CA6

@bitprophet
Member
bitprophet commented Apr 28, 2016 edited

Cool, that seems to do the trick, thanks guys! EDIT: Travis confirms

@alex
Contributor
alex commented Apr 28, 2016

👍

@bitprophet
Member

Merged to my local master, will go up once I finish the changeloggy junk.

@bitprophet bitprophet closed this Apr 28, 2016
@bitprophet bitprophet added a commit that referenced this pull request Apr 29, 2016
@bitprophet bitprophet Changelog entry re #731, re #611 474b566
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment