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

Add support for OCSP stapling. #580

Merged
merged 12 commits into from Jan 24, 2017
Merged

Add support for OCSP stapling. #580

merged 12 commits into from Jan 24, 2017

Conversation

@Lukasa
Copy link
Member

@Lukasa Lukasa commented Nov 29, 2016

This PR adds a collection of functions that add support for server and client side OCSP stapling.

In the spirit of PyOpenSSL's "thin wrapper" policy, these functions specifically do not provide any validation of stapled OCSP assertions: that's officially Someone Else's Problem. However, they do provide a mechanism whereby servers may staple OCSP assertions if they desire, and clients may retrieve those stapled OCSP assertions.

This PR cannot be merged yet, it will fail tests against most cryptography versions. We will need to wait until a new cryptography release is cut (specifically 1.7) and then make that our minimum supported cryptography version in order to properly enable this functionality.

ocsp_len = _lib.SSL_get_tlsext_status_ocsp_resp(ssl, ocsp_ptr)
if ocsp_len < 0:
# No OCSP data.
ocsp_data = b''
Copy link
Member Author

@Lukasa Lukasa Nov 29, 2016

Choose a reason for hiding this comment

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

It's not clear to me whether this is the best behaviour. In particular, I wonder whether we should set ocsp_data = None here: this means we actively received no OCSP data, and so is technically a bit distinct from the case where the server sent zero-length OCSP data (which I think is technically possible, though isn't possible with the way I've constructed the server callback helper).

Loading

Copy link
Member

@reaperhulk reaperhulk Jan 24, 2017

Choose a reason for hiding this comment

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

Can you think of a scenario where the distinction between "no OCSP data" vs "zero length OCSP data" is meaningful?

Loading

Copy link
Member Author

@Lukasa Lukasa Jan 24, 2017

Choose a reason for hiding this comment

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

So, I double-checked the RFC: the server cannot send zero-length OCSP data. The minimum allowed length for OCSP data is 1 byte. So I think we're safe: if the server sent zero-length OCSP data then we should just act like they didn't send it at all.

Loading

@codecov-io
Copy link

@codecov-io codecov-io commented Dec 12, 2016

Current coverage is 95.75% (diff: 98.48%)

Merging #580 into master will increase coverage by 0.09%

@@             master       #580   diff @@
==========================================
  Files            16         16          
  Lines          5615       5813   +198   
  Methods           0          0          
  Messages          0          0          
  Branches        403        411     +8   
==========================================
+ Hits           5371       5566   +195   
- Misses          167        170     +3   
  Partials         77         77          

Powered by Codecov. Last update 6d97756...7cb25ae

Loading

@Lukasa
Copy link
Member Author

@Lukasa Lukasa commented Dec 12, 2016

I have pushed the requirement for cryptography 1.7 and merged in the current master.

Loading

@hynek
Copy link
Contributor

@hynek hynek commented Jan 11, 2017

jftr i have no idea how ocsp works so y’all on your own :)

Loading

@tiran
Copy link

@tiran tiran commented Jan 11, 2017

@hynek magic! :)

I'm vaguely familiar with OCSP and can assist with review next week. Feel free to ping me if I don't respond by Jan 18th.

Loading

@Lukasa
Copy link
Member Author

@Lukasa Lukasa commented Jan 11, 2017

Remember that this PR does no validation of OCSP of any kind: just exposes the raw binary data of the stapled OCSP responses.

Loading

Copy link
Member

@reaperhulk reaperhulk left a comment

This basically LGTM, two comments.

Loading

ocsp_len = _lib.SSL_get_tlsext_status_ocsp_resp(ssl, ocsp_ptr)
if ocsp_len < 0:
# No OCSP data.
ocsp_data = b''
Copy link
Member

@reaperhulk reaperhulk Jan 24, 2017

Choose a reason for hiding this comment

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

Can you think of a scenario where the distinction between "no OCSP data" vs "zero length OCSP data" is meaningful?

Loading

def test_callbacks_arent_called_by_default(self):
"""
If both the client and the server have registered OCSP callbacks, but
the client does not sent the OCSP request, neither callback gets
Copy link
Member

@reaperhulk reaperhulk Jan 24, 2017

Choose a reason for hiding this comment

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

does not send

Loading

@Lukasa
Copy link
Member Author

@Lukasa Lukasa commented Jan 24, 2017

Ok @reaperhulk, I've responded to your feedback.

Loading

@reaperhulk reaperhulk merged commit 496652a into pyca:master Jan 24, 2017
1 check passed
Loading
@hynek
Copy link
Contributor

@hynek hynek commented Jan 24, 2017

could someone pls add a link to this pr in the changelog? thx!

Loading

Lukasa added a commit to Lukasa/pyopenssl that referenced this issue Jan 24, 2017
@Lukasa
Copy link
Member Author

@Lukasa Lukasa commented Jan 24, 2017

@hynek See #590.

Loading

hynek added a commit that referenced this issue Jan 24, 2017
@Lukasa Lukasa mentioned this pull request Mar 16, 2017
@ghost
Copy link

@ghost ghost commented Apr 5, 2017

I have pyopenssl v16.2.0 but it does not have the code for OCSP. The dependency, Cryptography is at version 1.8.1.
Have the changes been merged?

Loading

@hynek
Copy link
Contributor

@hynek hynek commented Apr 5, 2017

It has been merged but not released yet.

Loading

@ghost
Copy link

@ghost ghost commented Apr 17, 2017

As far as I've seen the code and understood, It has support for stapling the OCSP data during the handshake, but what I am confused about is, does it have support for an outbound connection from server to OCSP server that gets the latest OCSP data? ie if there is any mechanism which involves server and OCSP server?

Loading

@Lukasa
Copy link
Member Author

@Lukasa Lukasa commented Apr 17, 2017

No. As originally stated in the PR, the code provides only support for providing stapled OCSP data. No support is provided for obtaining that OCSP data.

Loading

@jszhang1985
Copy link

@jszhang1985 jszhang1985 commented May 3, 2017

def zjs_ocsp_callback(conn,response,tmp):
print 'response=%s' % response
return True
......
ctx.set_ocsp_client_callback(zjs_ocsp_callback)
....
cnx = SSL.Connection(ctx, sock)
cnx.request_ocsp()
......

The simplified code is above this, sniffer the package, the script will send client hello with status extenssion, and get status reply from ssl server.
Now, I want to print the OCSP stapling response, but the script will print mess code, no matter decode it with asicc or utf-8.
can anybody guard me a way to print this in human being ? thanks

Loading

aszlig added a commit to NixOS/nixpkgs that referenced this issue Jun 21, 2017
Upstream changes:

 * Added OpenSSL.X509Store.set_time() to set a custom verification time
   when verifying certificate chains. pyca/pyopenssl#567
 * Added a collection of functions for working with OCSP stapling. None
   of these functions make it possible to validate OCSP assertions, only
   to staple them into the handshake and to retrieve the stapled
   assertion if provided. Users will need to write their own code to
   handle OCSP assertions. We specifically added:
   Context.set_ocsp_server_callback, Context.set_ocsp_client_callback,
   and Connection.request_ocsp. pyca/pyopenssl#580
 * Changed the SSL module's memory allocation policy to avoid zeroing
   memory it allocates when unnecessary. This reduces CPU usage and
   memory allocation time by an amount proportional to the size of the
   allocation. For applications that process a lot of TLS data or that
   use very lage allocations this can provide considerable performance
   improvements. pyca/pyopenssl#578
 * Automatically set SSL_CTX_set_ecdh_auto() on OpenSSL.SSL.Context.
   pyca/pyopenssl#575
 * Fix empty exceptions from OpenSSL.crypto.load_privatekey().
   pyca/pyopenssl#581

The full upstream changelog can be found at:

https://pyopenssl.readthedocs.io/en/17.0.0/changelog.html

I've also added a patch from pyca/pyopenssl#637 in order to fix the
tests, which was the main reason for the version bump because that patch
won't apply for 16.2.0.

According to the upstream changelog there should be no
backwards-incompatible changes, but I've tested building against some of
the packages depending on pyopenssl anyway. Regardless of this, the
build for pyopenssl fails right now anyway, so the worst that could
happen via this commit would be that we break something that's already
broken.

Signed-off-by: aszlig <aszlig@redmoonstudios.org>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants