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.
• Use s2n_stuffer_reserve_uint32() to skip the section where session_ticket_lifetime_in_secs is calculated.
  • We can only calculate the lifetime after s2n_resume_encrypt_session_ticket() function call is finished, since it records the key_intro_time. Since the ticket is serialized, we need to use s2n_stuffer_reserve_uint32() to skip that section, and then come back to write the session ticket lifetime with s2n_stuffer_write_reservation(). Refer to this comment for more details.
• 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 decide to fix the session ticket lifetime calculation issue in this PR, so two relevant refactor PRs: PR#5014 and PR#5003 will be closed after this PR is merged.
• The way that we reserve space in advance and write it in the future is a common practice. However, this is not mentioned in the development guide. I believe that should be added to the development guide.
• 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]

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]

Do we need to incorporate the server_keying_material_lifetime in this calculation? I'm honestly not sure.

[maddeleine]

Don't you already have the intro timestamp in key_intro_time? What's the difference between that value and key->intro_timestamp?

[boquan-fang]

What's the difference between that value and key->intro_timestamp?

The difference is that key_intro_time variable is recording the intro time in seconds. That variable is passed into s2n_config_add_ticket_crypto_key() and the the intro time is then converted into nanoseconds in that function. This is how the function handles this:

s2n-tls/tls/s2n_config.c

Lines 940 to 943 in efb3500

	int s2n_config_add_ticket_crypto_key(struct s2n_config *config,
	const uint8_t *name, uint32_t name_len,
	uint8_t *key, uint32_t key_len,
	uint64_t intro_time_in_seconds_from_epoch)

s2n-tls/tls/s2n_config.c

Lines 1015 to 1021 in efb3500

	if (intro_time_in_seconds_from_epoch == 0) {
	uint64_t now = 0;
	POSIX_GUARD_RESULT(s2n_config_wall_clock(config, &now));
	session_ticket_key->intro_timestamp = now;
	} else {
	session_ticket_key->intro_timestamp = (intro_time_in_seconds_from_epoch * ONE_SEC_IN_NANOS);
	}

We need the key_intro_time in nanoseconds, and that's why I queried the key and acquire the intro time from the key.

I figured out another way to handle this is to simply convert key_intro_time from seconds to nanoseconds by doing:

key_intro_time * ONE_SEC_IN_NANOS

That will potentially improve the readability. I think I can do that simple calculation for every elifible key_intro_time query in this test. There are some cases later in this test that still needs to query the key in order to get the key intro time, because they didn't record the key_intro_time variable.

[maddeleine]

Do you have any idea if key->intro_timestamp - wall_clock_time_stamp is a constant value? Or maybe since we're removing precision with the conversion to seconds it becomes constant?

I'm curious because if we're not careful this could introduce flakiness to this test, where the test "sometimes" fails because the timing was off.

[boquan-fang]

Or maybe since we're removing precision with the conversion to seconds it becomes constant?

It is constant because we are removing precision with conversion to seconds. However, since we have stored the current timestamp in s2n_connection->ticket_fields, we should use that attribute to query current time stamp every time after a s2n_serialize_resumption_state() is called.

Hence, a more precise way to do this calculation is to do:

S2N_TICKET_ENCRYPT_DECRYPT_KEY_LIFETIME_IN_NANOS + S2N_TICKET_DECRYPT_KEY_LIFETIME_IN_NANOS +key->intro_timestamp - conn->ticket_fields.current_time

[maddeleine]

The logic for zero tickets lives in s2n_tls13_server_nst_write, not s2n_tls13_server_nst_send, so I would expect a test for s2n_tls13_server_nst_write.

[maddeleine]

This test is very confusing. If you're saying psk keying material has the shortest lifetime, it should be smaller than one week. Otherwise we could be just catching one week case.

[maddeleine]

You shouldn't need to set up a resumption test key for this test. You control the inputs to s2n_generate_ticket_lifetime. Just provide the function with the inputs that you want for testing.