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 a subset of pyOpenSSL 19.0.0. #69

Closed
wants to merge 1 commit into from

Conversation

haydenroche5
Copy link
Contributor

Specifically, the subset needed to support the Python module ndg_httpsclient.

Copy link
Member

@julek-wolfssl julek-wolfssl left a comment

Choose a reason for hiding this comment

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

And some additional comments on slack.

pyopenssl/19.0.0/README.md Show resolved Hide resolved
pyopenssl/19.0.0/README.md Show resolved Hide resolved
+ `cd ../../..` to get back to the root directory.
+ Run the tests (all should pass):
+ `python -m ndg.httpsclient.test.test_https`.
+ `python -m ndg.httpsclient.test.test_urllib2`.
Copy link
Member

Choose a reason for hiding this comment

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

All tests passed for me but for some reason this one didn't exit. I needed to interrupt manually. Any idea why this might be and if this may be an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, no ideas right now, but I'll try the tests again and see if I notice the same thing. If so, will debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

See next review.

pyopenssl/19.0.0/pyopenssl-19.0.0.patch Outdated Show resolved Hide resolved
Specifically, the subset needed to support the Python module ndg_httpsclient.
Comment on lines +47 to +57
+ # _from_raw_x509_ptr has code to call X509_free on the X509
+ # object when it's time to do garbage collection. With wolfSSL,
+ # when that X509_free call happens, it's possible that the X509
+ # object has already been freed by FreeX509 (an internal
+ # function). That function doesn't care what the reference count
+ # is and does the free unconditionally. This causes problems
+ # when the garbage collector comes along and tries to free the
+ # already freed X509. The solution for the wolfSSL case is to
+ # duplicate the object rather than fiddling with the ref count.
+ x509_copy = _lib.X509_dup(x509)
+ cert = X509._from_raw_x509_ptr(x509_copy)
Copy link
Member

Choose a reason for hiding this comment

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

All calls to FreeX509 should be changed to wolfSSL_X509_free. Leaving it like this is just going to cause problems down the line. I've found the following calls. Please change all calls to wolfSSL_X509_free. Then this section should not need a wolfSSL exception.

$ grep -rP '[^_]FreeX509\(' --include=*.[ch]
wolfssl/internal.h:    WOLFSSL_LOCAL void FreeX509(WOLFSSL_X509*);
src/ssl.c:        FreeX509(&ssl->peerCert);
src/ssl.c:static void ExternalFreeX509(WOLFSSL_X509* x509)
src/ssl.c:                FreeX509(x509);
src/ssl.c:    ExternalFreeX509(x509);
src/internal.c:void FreeX509(WOLFSSL_X509* x509)
src/internal.c:    FreeX509(&ssl->peerCert);
src/internal.c:                    FreeX509(x509);
src/internal.c:            FreeX509(x509);
src/internal.c:                            FreeX509(&ssl->peerCert);
src/internal.c:                            FreeX509(&ssl->peerCert);

Copy link
Member

Choose a reason for hiding this comment

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

Obviously only ExternalFreeX509 should be calling FreeX509 (which means that FreeX509 could be a static function in ssl.c.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me. I'll create a wolfSSL PR today or Monday doing that (I'm off tomorrow).

@JacobBarthelmeh
Copy link
Contributor

#150

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