Add methods to clear out peer certs on SSL_SESSION

[ngoyal]

Once an SSL connection is established, applications may want to free up some memory by clearing out peer cert information. This provides an API to let them free up peer cert information when needed.

Checklist

• documentation is added or updated

[mspncp]

Just a side note from a noninvolved: IMHO the function name does not follow the naming conventions: I would expect the name SSL_clear_peer_certificate, analogous to SSL_get_peer_certificate. See also existing examples like SSL_clear_chain_certs and SSL_clear_options.

[ngoyal]

Happy to change - there is a SSL_certs_clear which is what I mimicked - https://github.com/openssl/openssl/blob/master/include/openssl/ssl.h#L1744

[mspncp]

Oh, I see! It looks like OpenSSL itself does not have consistent naming conventions 😉. Well, in this part of the library I'm just an uninvolved spectator who happened to read this thread. I'll leave the question of the optimal name to a more qualified reviewer.

[richsalz]

close/open to rescan for CLA.

[vdukhovni]

Does this have any effect on renegotiation?
Does session resumption still work?

[ngoyal]

I don't think this should break either, but it would depend when you call it. Certainly not during mid handshake, but if established, then clearing up peer certs shouldn't be a problem. If code does something like:

SSL* s1 = createSSL();
SSL* s2 = createSSL();
connectSSLSync(s1, addr);
SSL_peer_certificate_clear(s1);
SSL_set_session(s2, SSL_get_session(s1));
connectSSLSync(s2, addr2);

Then I'm sure there could be oddities. This call should be invoked after any new ticket callbacks. I think it's valuable to provide this for applications where they establish a connection (either full handshake or from a duped session (using SSL_SESSION_dup for instance), do auth/mutual auth in the handshake / shortly after connect, and then no longer need cert information.

[vdukhovni]

I think it is important to document and test any limitations or lack thereof with subsequent session behaviour.

[davidben]

Once inserted into the session cache, the SSL_SESSION may be attached to other SSL objects which may even live on other threads. They're thus usually considered immutable once established (don't have the numbers off-hand, but previous violations of this have gotten CVEs).

As this is an API call the caller is responsible for making, I suppose that could all just be suitably disclaimed in the documentation, but that seems a little error-prone. Perhaps instead a config that tells the library "after you've verified the certificate and before you've done anything particularly scary like insert it into the session cache, please drop the peer certificate; I promise I don't need it". That would retain the immutability invariant.

[ngoyal]

Some applications may want to put additional verification outside of the handshake logic once the connection has been established. A config would require them to always use a verify callback, which may not be ideal under some instances.

[vdukhovni]

So it seems this is an unsafe API at least for server-side sessions. Is the memory cost worth it? The certs are freed eventually, and the total cost is a few KB per connection. I am not sure the savings are worth the reduced safety.

[ngoyal]

I think a few KB can be worth it for high fan in services or high fan out clients. Servers can be setup in such a way where this is safe to use. This discussion certainly implies a lot more documentation is needed around this.

[vdukhovni]

Perhaps more than just "be careful" documentation is required. It may be necessary to configure the SSL context in such a way that session sharing does not take place (say completely disable the internal session cache, allowing resumption only via session tickets, if that's safe, or not at all). If the call is not known to be safe, it should be a NOOP.

[ngoyal]

I'd really like to avoid overcomplicating this code if possible. I don't think it should be held to a higher standard than something like SSL_certs_clear or even many of the publicly available *_free functions which are completely unsafe in a number of situations.

[vdukhovni]

I disagree. Many of the _free() functions handle reference-counted objects, and are quite safe, and the ways in which they are unsafe are obvious and localized. This PR mutates a shared structure in subtle ways, and other code that makes use of the shared state may later change invalidating the assumptions that a developer made when originally mutating the state.

Therefore, if an application wants to save RAM, it should avoid having a session cache entirely, that saves even more RAM. And even within a single unshared session, we need to check that later uses of the session (say renegotiation with TLS &le 1.2) don't depend on retaining the peer certificate chain.

The requirements are likely different for client vs. server sessions etc. If making this robust is too much work, the PR should IMHO be deferred, rather than adopted half-done.

[vdukhovni]

If the current SSL object has the only reference to the session then indeed it is likely not in the session cache. I am not sure that's enough, but perhaps it is. Mind you, if sessions are being added to the cache automatically, perhaps by the time the connection is established you'll rarely find the session having a refcount of 1 (there'll be another reference from the cache). So you can probably only do this when the session cache is off, and only session tickets are being issued (server-side). On the client side there is no default session cache IIRC.

This requires careful analysis, which I have not performed.

[ngoyal]

I'll look into ways to make this robust. Deleted my previous comment since I think ref count == 1 is too limiting based on our tests. Outside of checking if prev/next are NULL, I'll check how this affects renegotiation, but is adversely affecting renegotiation a deal breaker for this patch if we document it? (i.e. subsequent calls to SSL_renegotiate_abbreviated will behave like...). I'll try to put together some tests.

FWIW, our use cases are already saving RAM by using tickets and disabling the session cache. This ends up saving us more since we have some pretty high fan in/out, so the more we can save the more it helps us.

[ngoyal]

@vdukhovni what about slimming this down to just SSL_SESSION_clear_peer_certificate and having that method check if prev/next are NULL before proceeding?

[davidben]

prev/next aren't necessarily a sufficient check when extension session cache callbacks are used, so you would probably still need to suitably document the constraints.

[kaduk]

There is another thread to throw in the mix, namely that TLS 1.3 may prompt us to add in a call to explicitly request "send a NewSessionTicket message now". That is, of course, driven by the application code and so the application author could in theory enforce the requisite sequencing between clearing peer certs and sending the ticket, but it is additional complexity.

[ngoyal]

I've limited the scope to just SSL_SESSION now to prevent some accidental misuse and documented cautionary tales about using this.

[ngoyal]

Any additional thoughts on this or things the team would like for approval?