Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix invalid RSA private key PKCS8 encoding #120

Merged
merged 9 commits into from
Dec 27, 2018

Conversation

mattsb42-aws
Copy link
Contributor

This addresses #119 to fix the way that the rsa_backend module was converting PKCS1 keys to PKCS8. I left a fallback on read to attempt the old methodology if the valid parsing fails, but writes will only every use the valid encoding.

NOTE: The Firebase tests will still fail. That is addressed in #118. Also, some of the pyca/cryptography and compatibility tests will fail. Those are addressed in #116 and #117.

This is incorrect parsing and only works because the legacy PKCS1-to-PKCS8
encoding was also incorrect.
"""
return der_key[len(LEGACY_INVALID_PKCS8_RSA_HEADER):]

Choose a reason for hiding this comment

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

Please add a check that the prefix of this invalid key is, in fact, equal to LEGACY_INVALID_PKCS8_RSA_HEADER, else reject.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check added looking for the legacy header followed immediately by an ASN1 sequence identifier. That will protect against the case where an otherwise-invalid key is processed here, since for 2048-bit keys the legacy prefix is actually correct, but incomplete.

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Dec 27, 2018

Codecov Report

Merging #120 into backend-explicit-tests will decrease coverage by 27.87%.
The diff coverage is 92.1%.

Impacted file tree graph

@@                     Coverage Diff                     @@
##           backend-explicit-tests     #120       +/-   ##
===========================================================
- Coverage                   96.86%   68.99%   -27.88%     
===========================================================
  Files                          13       13               
  Lines                        1022     1032       +10     
===========================================================
- Hits                          990      712      -278     
- Misses                         32      320      +288
Impacted Files Coverage Δ
jose/backends/rsa_backend.py 49.41% <92.1%> (-47.79%) ⬇️
jose/backends/cryptography_backend.py 4.14% <0%> (-93.97%) ⬇️
jose/backends/__init__.py 54.54% <0%> (-45.46%) ⬇️
jose/utils.py 63.15% <0%> (-24.57%) ⬇️
jose/backends/pycrypto_backend.py 90.9% <0%> (-4.55%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff8a03b...428c29c. Read the comment docs.

@mpdavis
Copy link
Owner

mpdavis commented Dec 27, 2018

Other than the merge conflict, this LGTM.

@mattsb42-aws
Copy link
Contributor Author

Merge conflict resolved, and since it did a full branch merge, all the tests should actually pass now. :)

@mpdavis mpdavis merged commit c28f15a into mpdavis:backend-explicit-tests Dec 27, 2018
@mattsb42-aws mattsb42-aws deleted the fix-rsa branch December 27, 2018 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants