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

added method to export keying material from an ssl connection #686

Closed
wants to merge 4 commits into from

Conversation

kelbyludwig
Copy link

This adds a new method to export keying material from an SSL connection (RFC 5705).

It currently will not work without merging in pyca/cryptography#3878.

@codecov
Copy link

codecov bot commented Aug 26, 2017

Codecov Report

Merging #686 into master will decrease coverage by 0.34%.
The diff coverage is 28.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #686      +/-   ##
==========================================
- Coverage   97.01%   96.67%   -0.35%     
==========================================
  Files          16       16              
  Lines        5630     5651      +21     
  Branches      391      392       +1     
==========================================
+ Hits         5462     5463       +1     
- Misses        112      131      +19     
- Partials       56       57       +1
Impacted Files Coverage Δ
src/OpenSSL/SSL.py 94.39% <50%> (-0.5%) ⬇️
tests/test_ssl.py 98.28% <9.09%> (-0.83%) ⬇️

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 332848f...348afd4. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 26, 2017

Codecov Report

Merging #686 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #686      +/-   ##
==========================================
+ Coverage   97.01%   97.02%   +0.01%     
==========================================
  Files          16       16              
  Lines        5630     5652      +22     
  Branches      391      392       +1     
==========================================
+ Hits         5462     5484      +22     
  Misses        112      112              
  Partials       56       56
Impacted Files Coverage Δ
tests/test_ssl.py 99.11% <100%> (ø) ⬆️
src/OpenSSL/SSL.py 94.94% <100%> (+0.05%) ⬆️
src/OpenSSL/crypto.py 96.81% <0%> (ø) ⬆️

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 332848f...317c9c8. Read the comment docs.

@kelbyludwig kelbyludwig reopened this Aug 30, 2017
@alexwlchan
Copy link
Contributor

This is blocked not just on the patch in cryptography being merged, but also a new release. The PyOpenSSL CI doesn’t install from cryptography master, so this will keep failing for now.

@reaperhulk
Copy link
Member

That's not strictly true -- we have cryptography master tests (that are allowed to fail) in travis as well. In fact, they're currently failing on py3 indicating a bytes/str bug in this PR (https://travis-ci.org/pyca/pyopenssl/jobs/269830756).

@reaperhulk
Copy link
Member

That said, Travis won't go green until after a cryptography release (and this PR will need to bump the cryptography dependency as well at that point).

@kelbyludwig
Copy link
Author

The Travis checks look like they might only be failing for versions of cryptography that don't have pyca/cryptography#3878 included?

@alex
Copy link
Member

alex commented Oct 23, 2017

The set of tests that are failing is the cryptographyMinimum set. Take a look at tox.ini and see about bumping that.

@kelbyludwig
Copy link
Author

the py35-urllib3Master failure seems unrelated.

@kelbyludwig
Copy link
Author

Is there anything else I could be doing to get this ready for review?

# RFC 5705: "Labels here have the same definition as in TLS, i.e., an
# ASCII string with no terminating NULL."
# So we must remove the NULL terminator.
label_buf = _ffi.new("char[]", label)[0:len(label)]
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to slice this? cffi will add a terminating NULL but when you pass it to SSL_export_keying_material with len(label) the terminating NULL simply won't be read.

label_buf, len(label),
context_buf, context_len,
use_context)
return _ffi.buffer(outp, olen)[:] if success == 1 else None
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't return None if this fails. This should call _openssl_assert(success == 1), which will raise an exception if exporting the keying material fails.

Copy link
Member

@reaperhulk reaperhulk left a comment

Choose a reason for hiding this comment

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

Modulo my two comments this looks good. Thanks for sticking with us on this extremely long review cycle.

@reaperhulk
Copy link
Member

Finished in #725

@reaperhulk reaperhulk closed this Nov 30, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 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.

None yet

4 participants