add support for Context.set_cert_store #1210
Conversation
src/OpenSSL/SSL.py
Outdated
| # The store is now owned by the context, so we need to | ||
| # remove the gc free in the object. We do this after the | ||
| # set since set may not exist. | ||
| _ffi.gc(store._store, None) |
There was a problem hiding this comment.
This seems like the very wrong way to do this. We should be bumping the refcount here, not getting rid of the GC. Doing it this way means it's totally wrong if you put the same store on 2x ssl contexts
There was a problem hiding this comment.
We don't have that binding 😂 Good catch on the multiple adds though. We can't support this without adding the upref binding then.
src/OpenSSL/SSL.py
Outdated
| # remove the gc free in the object. We do this after the | ||
| # set since set may not exist. | ||
| _ffi.gc(store._store, None) | ||
| except AttributeError: |
There was a problem hiding this comment.
This also catches attribute errors if store is the wrong type. Bad.
tox.ini
Outdated
| @@ -19,6 +19,8 @@ extras = | |||
| deps = | |||
| coverage>=4.2 | |||
| cryptographyMinimum: cryptography==38.0.0 | |||
| # special version to test paths for bindings we temporarily removed | |||
| cryptography40: cryptography==40.0.1 | |||
There was a problem hiding this comment.
Can we just bump the minimum, this is too complicated
|
This is blocked on a release that contains pyca/cryptography#8737 now. |
| @@ -1507,6 +1507,16 @@ def get_cert_store(self): | |||
| pystore._store = store | |||
| return pystore | |||
|
|
|||
| def set_cert_store(self, store): | |||
There was a problem hiding this comment.
Can we added type annotations? Seems like we should start adding them as we go.
| def test_set_cert_store(self): | ||
| context = Context(SSLv23_METHOD) | ||
| store = X509Store() | ||
| context.set_cert_store(store) | ||
| assert store._store == context.get_cert_store()._store |
There was a problem hiding this comment.
Can we make this test actually verify what we want?
No description provided.