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

Update for OpenSSL 1.1 #875

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ppaeps
Copy link

@ppaeps ppaeps commented Oct 18, 2021

This replaces all uses of the OpenSSL API with post-1.1 idioms. It builds on the preparations put in place by @sraustein several years ago. No effort has been made to retain portability with obsolete versions of OpenSSL. It is clear that those versions of OpenSSL should no longer be used anywhere.

With these changes, rpki.net will work (and hopefully work well!) on systems that still have Python 2.7 (also obsolete) but that have moved beyond OpenSSL version 1.1.

I would very much appreciate help testing these changes. My testing has been largely focussed on unit-testing the Python OpenSSL Wrappers and the relying-party code. The CA components have seen almost no testing from me. This pull request should probably not be merged before at least one other person compiles this branch!

Replace all uses of the OpenSSL API with post-1.1 idioms.  No effort has
been made to retain portability with obsolete versions of OpenSSL.  This
enables rpki.net to work on systems that still have Python 2.7 but have
updated OpenSSL beyond OpenSSL 1.1.
ext/POW.c Outdated
@@ -10198,7 +10217,9 @@ init_POW(void)
* if you tinker with the build script and start seeing nasty
* memory-related issues, this might be the cause.
*/
#ifdef notyet
CRYPTO_set_mem_functions(PyMem_Malloc, PyMem_Realloc, PyMem_Free);
Copy link

Choose a reason for hiding this comment

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

From https://man.openbsd.org/CRYPTO_set_mem_functions#DESCRIPTION

DESCRIPTION
     Do not use any of the interfaces documented here.  They are provided
     purely for compatibility with legacy application code.

     CRYPTO_get_mem_functions() assigns pointers to the C library functions
     malloc(3), realloc(3), and free(3) to those of its arguments that are not
     NULL.

     CRYPTO_set_mem_functions(), CRYPTO_mem_ctrl(), CRYPTO_mem_leaks(),
     CRYPTO_mem_leaks_fp(), and CRYPTO_mem_leaks_cb() have no effect.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! I forgot about this. I also found that documentation you cite.

Note that, unlike LibreSSL, OpenSSL 1.1 still has a working CRYPTO_set_mem_functions().

For completeness, here is the Python perspective on this, from https://docs.python.org/3/c-api/memory.html:

In most situations, however, it is recommended to allocate memory from the Python heap specifically because the latter is under control of the Python memory manager. For example, this is required when the interpreter is extended with new object types written in C. Another reason for using the Python heap is the desire to inform the Python memory manager about the memory needs of the extension module. Even when the requested memory is used exclusively for internal, highly-specific purposes, delegating all memory requests to the Python memory manager causes the interpreter to have a more accurate image of its memory footprint as a whole. Consequently, under certain circumstances, the Python memory manager may or may not trigger appropriate actions, like garbage collection, memory compaction or other preventive procedures. Note that by using the C library allocator as shown in the previous example, the allocated memory for the I/O buffer escapes completely the Python memory manager.

This is clearly "encouraging" language, as per the comment.

I am still torn about the best way forward. On the one hand, I feel better about OpenSSL managing its memory without potential interference from Python. On the other hand, I can easily imagine Python getting very confused about memory that isn't under its control. The code does work fine as-is, without using the Python memory allocator. Though I've not done any specific testing to try to catch edge cases.

For what it's worth, everyone currently running rpki.net is using the Python memory allocator... Given that this is very much code in maintenance mode (if not outright suspended animation), I'm leaning towards providing wrappers to OpenSSL.

Copy link
Author

Choose a reason for hiding this comment

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

[...] from https://docs.python.org/3/c-api/memory.html [...]

The Python 2.7 manual uses the same language: https://docs.python.org/2.7/c-api/memory.html

ppaeps and others added 4 commits October 26, 2021 15:18
Remove the #ifdef notyet fence around CRYPTO_set_mem_functions().  While
it it generally preferable to let OpenSSL manage its memory, existing
users of rpki.net have been using the Python memory allocator all along.
Correct the check for whether OpenSSL has been compiled with RFC3779
support.  v3_addr_validate_path is spelled X509v3_addr_validate_path
in OpenSSL 1.1 and later.
@ppaeps
Copy link
Author

ppaeps commented Nov 1, 2021

Picked up two commits from @sraustein. These add wrappers around the Python memory management functions and update the included OpenSSL distribution for users whose system OpenSSL is not built with RFC 3779 support.

What this branch really needs now is more thorough testing in a real-world environment.

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