Refactor some s2n_resume functions

[maddeleine]

Resolved issues:

N/A

Description of changes:

I'm planning on making some changes to our resumption feature and it's going to be much easier for me to make these changes if I refactor the existing code. Essentially in this PR I refactored the encrypt/decrypt session ticket functions as follows:

1. Changed name of functions to match module
2. Changed to S2N_RESULT
3. Changed ordering of initializations to be easier to follow
4. Defer cleanups when possible

Call-outs:

Gist of old decrypt_ticket and old decrypt_cache functions: https://gist.github.com/maddeleine/284a6f2d7769cc05b74e509e61fa18a6.

Testing:

All existing tests pass. Looking for recommendations about tests I could add here.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[lrstewart]

This probably needs a more detailed comment. Without knowing the history / digging into s2n_deserialize_resumption_state, branching on the arguments to s2n_deserialize_resumption_state looks pretty strange.

Should we also open an issue to fix that logic? It works, but it's pretty odd.

[maddeleine]

What's the fix for this logic? No matter what, we're going to need a branch somewhere to make sure session cache isn't using the middle parameter.

[lrstewart]

Somewhere, but here may not be the right place. It's very unclear why we're branching here, and passing "NULL" to the method is odd. I'd say ideally we should branch further down the stack where the parameter should either be used or not used. Like, where we're checking for NULL.

[maddeleine]

I took out the branching logic. That middle parameter never gets used if you're entering the codepath from the decrypt function in TLS1.2. There is still a branch to make sure the last code paragraph doesn't get executed if you're not using tickets.

[lrstewart]

Since there's confusion here, I was wondering if we could somehow write a test to prove no behavior change.

s2n_decrypt_session_cache was only called by s2n_resume_from_cache, which is only called here. That means that s2n_decrypt_session_cache could only be called for a TLS1.2 server connection.

At the other end, the "ticket" parameter is only used by s2n_deserialize_resumption_state for TLS1.2 tickets + client connections OR TLS1.3 tickets.

So I agree that the "ticket" parameter never gets used on the s2n_decrypt_session_cache path. We'd somehow need a TLS1.2 server decrypting a TLS1.3 ticket. Since s2n_resume_from_cache already assumes TLS1.2 tickets (it only allocates enough space for a TLS1.2 ticket), we're not making that path worse.

But that also means that testing this would be hard. The best I can think of is to resume from the session cache and verify that 1) conn->ticket isn't set 2) no psk exists. Not exactly a great test, so feel free not to implement it. Two people scrutinizing the code path is probably sufficient.