s2n-tls
s2n-tls copied to clipboard
Make s2n_init idempotent or return error for multi-s2n_init invocations
Problem:
Customer is facing issues due to a double-call to s2n_init due to dependencies re-calling s2n_init. Directly from the customer:
Specifically, our application depends on multiple libraries that all individually depend on S2N. They all have a need to call s2n_init and expect it to succeed, which is critical to the functionality of their library. However, as currently implemented, s2n_init is not idempotent, nor it returns an error code that indicates it has already been initialized. As result, only the first calling library works, and others fail. It makes it hard for application like ours that directly/indirectly depend on S2N via multiple dependency paths.
Solution:
- There was a stale pr that tried to support multiple
s2n_init
invocations (https://github.com/aws/s2n-tls/pull/1999). - Alternatively, have s2n_init return an error code if it has been initialised already (static flag tracking this https://github.com/aws/s2n-tls/blob/main/utils/s2n_init.c#L41)
Requirements / Acceptance Criteria:
What must a solution address in order to solve the problem? How do we know the solution is complete?
- RFC links: Links to relevant RFC(s)
- Related Issues: Link any relevant issues
- Will the Usage Guide or other documentation need to be updated?
-
Testing: How will this change be tested? Call out new integration tests, functional tests, or particularly interesting/important unit tests.
- Will this change trigger SAW changes? Changes to the state machine, the s2n_handshake_io code that controls state transitions, the DRBG, or the corking/uncorking logic could trigger SAW failures.
- Should this change be fuzz tested? Will it handle untrusted input? Create a separate issue to track the fuzzing work.
Out of scope:
Is there anything the solution will intentionally NOT address?
It is NOT a good idea to make it idempotent (do nothing if already initialized). Imagine this scenario:
- Application starts up and calls s2n_init()
- later on, application dynamically loads some Library, which also calls s2n_init()
- later on, application unloads that dynamic Library, which calls s2n_cleanup()
- Application continues trying to use S2N, but S2N is no longer initialized, so everything is broken
It would be better to go the route of returning some ALREADY_INITIALIZED error, then the Library could know that it shouldn't call s2n_cleanup()
Another idea: Do reference counting with s2n_init() and s2n_cleanup() calls.
Very similar to being idempotent (the only thing that changes on a 2nd call to s2n_init() is the reference count). But if s2n_init() is called 3 times by an application, then s2n_cleanup() won't ACTUALLY clean up until the reference count hits 0.