ssl: Store PeerCertificate for session resumption

[sindrip]

This PR subsumes PR: #5957 and closes issue: #5870

Calling ssl:peercert/1 after a client connects with session resumption returns {error, no_peercert}. This PR adds the new options stateful_with_cert and stateless_with_cert to the server_session_tickets option that allow ssl:peercert/1 to return the client certificate even after session resumption.

[sindrip]

Addressing review from PR: #5957 @u3s

• [x] 1. we would like this behavior to be optional, not default on server side.
  so only interested users are affected. preferably we would like to avoid new option, maybe we could extend server_session_tickets() = disabled | stateful | stateless with values: stateful_with_cert | stateless_with_cert ?

• [x] 2. it would be preferred to have equivalent behavior for stateful tickets, although having different implementation (cert stored on server side).

• [x] 3. ssl_session_ticket_SUITE - we would like to have blackbox tests using ssl API 1. if we lack some test coverage, we might add some whitebox 2. with new code ssl:peercert/1 should return {ok, Cert} instead of {error, no_peercert} when stateless_with_cert or stateful_with_cert 3. maybe also add instructions in existing tests, for checking behavior with existing code

This is done, but please take a look at the new test groups that I have set up in ssl_session_ticket_SUITE. Let me know if you agree with what I have there.

Can you clarify what you mean by point 3, adding instructions to existing tests?

• [x] 4. for new option values we would need necessary doc update

• [x] 5. see also minor comments in code

I think the indentation is done correctly now, and the tls_server_session_ticket_SUITE is passing now. (The suite was being skipped on the commit that this branch is based on and I also fixed that).

[github-actions[bot]]

CT Test Results

       2 files       64 suites   49m 12s :stopwatch:    754 tests    720 :heavy_check_mark:   33 :zzz: 1 :x: 3 581 runs  2 905 :heavy_check_mark: 675 :zzz: 1 :x:

For more details on these failures, see this check.

Results for commit 75b704cf.

:recycle: This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• Windows Installer

// Erlang/OTP Github Action Bot

[u3s]

* [ ]  3. `ssl_session_ticket_SUITE` - we would like to have blackbox tests using ssl API
  1. if we lack some test coverage, we might add some whitebox
  2. with new code `ssl:peercert/1` should return `{ok, Cert}` instead of
  `{error, no_peercert}` when `stateless_with_cert` or `stateful_with_cert`
  3. maybe also add instructions in existing tests, for checking behavior with existing code

This is done, but please take a look at the new test groups that I have set up in ssl_session_ticket_SUITE. Let me know if you agree with what I have there. Can you clarify what you mean by point 3, adding instructions to existing tests?

I initially thought that adding peercert_stateless_stateful_without_cert test case might not be required as one of existing test cases already covers that flow - then such instruction {error, no_peercert} = ssl:peercert(SSocket1) could be added to improve verification done there. Do you think we need peercert_stateless_stateful_without_cert ?

PS. we will do code review a bit later and come back to you.

[sindrip]

I initially thought that adding peercert_stateless_stateful_without_cert test case might not be required as one of existing test cases already covers that flow - then such instruction {error, no_peercert} = ssl:peercert(SSocket1) could be added to improve verification done there. Do you think we need peercert_stateless_stateful_without_cert ?

PS. we will do code review a bit later and come back to you.

Now I see, yes that makes sense. I'm looking at it, and I might even be able to put it into basic_test with a conditional on what the response should be. Then we can get rid of the 2 new testcases, and run the same testcases for both implementations.

[sindrip]

Made the tests simpler, I think this looks much better. I also found one bug with option handling between anti_replay and the new options and fixed that.

[sindrip]

Rebased due to conflicts, fixed the option handling in ssl.erl that had changed.

[u3s]

Thanks for work so far. Tests in our environment look good. I would @IngelaAndin to also have a look before we merge.

[sindrip]

I extended the tests and squashed

[sindrip]

I just checked the failure, and it's not in a testcase that I changed and the tests are passing on my end at least. Can you re-run?

[sindrip]

I see that the tests passed on the re-run, but the results don't seem to have been updated in the ct results check. I don't think there's anything that needs to be done from my side at this moment, just want to confirm.

[u3s]

yes. no worries. github test results are just early verification (I don't know why re-run are not taken in summary). we've more tests which are successful for this PR. we will merge code soon.

[u3s]

thanks for contribution!