fix: calculation of session ticket age

[boquan-fang]

Release Summary:

Fix the improper calculation of session ticket lifetime.

Resolved issues:

Resolve issue #4583. Resolve issue #2756.

Description of changes:

Our current calculations of session ticket lifetimes are incorrect, please refers to the relevant issues for more details. The session ticket age should not exceed the user defined ticket age, remaining lifetime of encrypt decrypt key, remaining lifetime of PSK keying material, and one week.

• Fix the s2n_generate_ticket_lifetime() function:
  • Introduce new parametesr uint64_t key_intro_time, and uint64_t current_time to s2n_generate_ticket_lifetime(), so that the function is aware of when the key is introduced and what the current time is.
  • Acquire the chosen_psk->keying_material_expiration in a s2n_connection struct to calculate the remaining lifetime of the PSK.
• Add uint64_t *key_intro_time parameter to s2n_resume_encrypt_session_ticket():
  • S2N-TLS select STEK in s2n_resume_encrypt_session_ticket() function. We can retrieve the key_intro_time from that function without moving the location where we called s2n_get_ticket_encrypt_decrypt_key().
  • S2N-TLS is doing something similar in s2n_resume_decrypt_session():
    

    s2n-tls/tls/s2n_resume.c

    Lines 892 to 893 in a439c1a

    	static S2N_RESULT s2n_resume_decrypt_session(struct s2n_connection *conn, struct s2n_stuffer *from,
    	uint64_t *key_intro_time)

    
    

    s2n-tls/tls/s2n_resume.c

    Lines 959 to 960 in a439c1a

    	/* Store this key timestamp for session ticket logic */
    	*key_intro_time = key->intro_timestamp;

  • If the user or the function don't need to retrieve key_intro_time, then they can pass NULL into that parameter.
• Add uint64_t current_time into struct s2n_ticket_fields.
  • I did a refactor to change the variable name in s2n_connection from tls13_ticket_fields to ticket_fields, since both TLS1.2 and TLS1.3 have access to that variable.
  • I used this variable to record current time, so that all calculation using now will have consistent time stamp.
  • This also improve readability as I also mentioned in the Call-outs section.
• Fix tests those use s2n_generate_ticket_lifetime() function:
  • Use s2n_resumption_test_ticket_key_setup() to set up STEK for the connection.
  • Aquire the STEK and retrieve the key's intro time stamp.
    • Use s2n_find_ticket_key() with the key's name to acquire the actual s2n_ticket_key.
    • Even if there are multiple session ticket keys associated with one config, we do know which key will be used for encryption in tests. One example would be in s2n_session_ticket_tests.c:
      

      s2n-tls/tests/unit/s2n_session_ticket_test.c

      Lines 817 to 821 in 1c38961

      	/* Verify that the client received NST which is encrypted using a key which is at it's peak encryption */
      	serialized_session_state_length = s2n_connection_get_session_length(client_conn);
      	EXPECT_EQUAL(s2n_connection_get_session(client_conn, serialized_session_state, serialized_session_state_length), serialized_session_state_length);
      	EXPECT_BYTEARRAY_EQUAL(serialized_session_state + S2N_TICKET_KEY_NAME_LOCATION,
      	ticket_key_name1, s2n_array_len(ticket_key_name1));

  • Add test cases for the situation where the PSK keying material lifetime is the shortest, and when session ticket lifetime is zero.

Call-outs:

• I didn't reset the lifetime for encrypt decrypt keys and user defined session ticket age when I test s2n_generate_ticket_lifetime() for shortest remaining PSK keying material lifetime. I set the PSK keying material expiration to be half a week, and reuse the previous configurations.
• In s2n_server_new_session_ticket_test.c, I didn't record realtime time stamp for two test cases where Session state has shortest lifetime and Both session state and decrypt key have longer lifetimes than a week.
  • The ticket lifetime in those two situations don't depend on the realtime time stamp. That's why I didn't record them.
• This PR is to fix TLS1.2 and TLS1.3.
• I have worked on another method to refactor the STEK information out of s2n_resume_encrypt_session_ticket_function(), which is to pass both key andcurrent_time parameter into all functions involved. However, that will damaged the overall readability of the code. Such changes can be found in this feature branch.

Testing:

• I have added an additional test case for the case where the remaining PSK keying material lifetime is the shortest.
• I have updated the test for encrypt + decrypt key has shortest lifetime.
• The new unit tests pass locally.
• The CI will run the test again.
  • The regression test will fail currently, because the commit ci: fix regression test paths #4996 that fixes that issue is not yet merged.
  • The S2nIntegrationV2SmallBatch test requires this PR to be updated with main. I have tested it on a back up branch. The test should succeed after the update. Here is the test result.
  • Since S2nIntegrationV2SmallBatch job failed, the Linters / validate start_codebuild.sh failed as well. However, that job should pass after this PR branch is updated to main.

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

[maddeleine]

This question from #2756 is not in scope of this PR.

Why not?

[boquan-fang]

This question from #2756 is not in scope of this PR.

Why not?

I will do it in this PR.

[maddeleine]

Honestly it might be better to just put this value on the ticket_fields struct as well. Doesn't really make sense to pass this by parameter.

[boquan-fang]

I agree. It makes sense to put key_intro_time into the ticket field. This information is supposed to be a part of the ticket field information. Doing this also improve the readability of the code, since it doesn't add additional parameter.

[maddeleine]

Can you pull out the calculation of the key's remaining lifetime? It's still really hard to read right now. I think what would help is if you did:

1. Calculate ticket_key_age in sec
2. Calculate key_lifetime in sec
3. Then calculate remaining_key_lifetime = key_lifetime - ticket_key_age.

[boquan-fang]

I don't want to straight up calculate those variables in seconds, since that might make the calculation a bit inaccurate: we are doing integer division with ONE_SEC_IN_NANOS:

s2n-tls/tls/s2n_server_new_session_ticket.c

Lines 198 to 200 in e79b1b9

	uint32_t key_lifetime_in_secs =
	(conn->config->encrypt_decrypt_key_lifetime_in_nanos + conn->config->decrypt_key_lifetime_in_nanos) / ONE_SEC_IN_NANOS;
	uint32_t session_lifetime_in_secs = conn->config->session_state_lifetime_in_nanos / ONE_SEC_IN_NANOS;

Some details might be missed if we first convert nanos to secs and then do the calculation.

I would follow your thoughts and calculate it like this:

1. Calculate ticket_key_age in nanos
2. Calculate key_lifetime in nanos
3. Calculate remaining_key_lifetime_in_sec = (key_lifetime - ticket_key_age) / ONE_SEC_IN_NANOS
4. Calculate remaining_psk_keying_material_lifetime_in_sec
5. Use MIN to find the minimum among them.

[maddeleine]

Yeah, I think that approach will work. I'm basically just think you need to separate out the calculations in a way that's easy to follow.

[maddeleine]

Maybe let's try doing uint8_t *lifetime_ptr = s2n_stuffer_raw_write(output, sizeof(uint32_t) instead. We want to avoid messing directly with write_cursors if we can.

[maddeleine]

Hmmm, not really following this. Why are you setting it to UINT32_MAX?

[boquan-fang]

Originally, I want to set UINT32_MAX as the default for the PSK keying material lifetime. If PSK doesn't exist, then it wouldn't be updated. Subsequently, it will always be larger than the rest variables. This method is hard to understand, and I will improve this logic.

[maddeleine]

Note for this entire function, if you're doing any type of subtraction, you should assert you're not underflowing before actually doing the calculation.