Merge branch 'tmp-persistent-id' into 'master'

resumption: keep persistent session identifiers

Closes #484

See merge request gnutls/gnutls!721

9 files changed, 111 insertions, 71 deletions

diff --git a/doc/cha-upgrade.texi b/doc/cha-upgrade.texi
index 83e1120840..28c9249a7d 100644
--- a/doc/cha-upgrade.texi
+++ b/doc/cha-upgrade.texi
@@ -227,6 +227,10 @@ flag using @funcref{gnutls_certificate_set_flags}.
 @tab It is replaced by separate key update and re-authentication functionality
 which can be accessed directly via @funcref{gnutls_session_key_update} and @funcref{gnutls_reauth}.
 
+@item TLS session identifiers are not shared with the server under TLS 1.3.
+@tab The TLS session identifiers are persistent across resumption only on
+server side and can be obtained as before via @funcref{gnutls_session_get_id2}.
+
 @item @funcref{gnutls_pkcs11_privkey_generate3}, @funcref{gnutls_pkcs11_copy_secret_key}, @funcref{gnutls_pkcs11_copy_x509_privkey2}
 @tab These functions no longer create an exportable key by default; they require the flag @code{GNUTLS_PKCS11_OBJ_FLAG_MARK_NOT_SENSITIVE} to do so.
 
diff --git a/lib/constate.c b/lib/constate.c
index 3ff6b526da..62623c10dd 100644
--- a/lib/constate.c
+++ b/lib/constate.c
@@ -678,9 +678,9 @@ int _gnutls_epoch_set_keys(gnutls_session_t session, uint16_t epoch, hs_stage_t
 		dst->prf = src->prf; \
 		dst->grp = src->grp; \
 		dst->pversion = src->pversion; \
-		memcpy( dst->session_id, src->session_id, GNUTLS_MAX_SESSION_ID_SIZE); \
-		dst->session_id_size = src->session_id_size; \
 	} \
+	memcpy( dst->session_id, src->session_id, GNUTLS_MAX_SESSION_ID_SIZE); \
+	dst->session_id_size = src->session_id_size; \
 	dst->timestamp = src->timestamp; \
 	dst->cert_type = src->cert_type; \
 	dst->client_auth_type = src->client_auth_type; \
diff --git a/lib/ext/session_ticket.c b/lib/ext/session_ticket.c
index 11d5db75c4..0cab3afe8c 100644
--- a/lib/ext/session_ticket.c
+++ b/lib/ext/session_ticket.c
@@ -652,6 +652,20 @@ int _gnutls_send_new_session_ticket(gnutls_session_t session, int again)
 			return ret;
 		}
 
+		/* Under TLS1.2 with session tickets, the session ID is used for different
+		 * purposes than the TLS1.0 session ID. Ensure that there is an internally
+		 * set value which the server will see on the original and resumed sessions */
+		if (session->internals.resumed != RESUME_TRUE) {
+			ret = _gnutls_generate_session_id(session->security_parameters.
+							  session_id,
+							  &session->security_parameters.
+							  session_id_size);
+			if (ret < 0) {
+				gnutls_assert();
+				return ret;
+			}
+		}
+
 		session->security_parameters.epoch_write =
 		    session->security_parameters.epoch_next;
 
diff --git a/lib/gnutls_int.h b/lib/gnutls_int.h
index 412bb368a1..925759049c 100644
--- a/lib/gnutls_int.h
+++ b/lib/gnutls_int.h
@@ -115,6 +115,8 @@ typedef struct {
  */
 #define MAX_HANDSHAKE_PACKET_SIZE 128*1024
 
+#define GNUTLS_DEF_SESSION_ID_SIZE 32
+
 /* The maximum digest size of hash algorithms.
  */
 #define MAX_FILENAME 512
diff --git a/lib/handshake.c b/lib/handshake.c
index ba3911d160..7274bafebd 100644
--- a/lib/handshake.c
+++ b/lib/handshake.c
@@ -158,7 +158,7 @@ int _gnutls13_handshake_hash_buffers_synth(gnutls_session_t session,
  * internals, and to security_parameters.
  * this will keep as less data to security_parameters.
  */
-static int resume_copy_required_values(gnutls_session_t session)
+static int tls12_resume_copy_required_vals(gnutls_session_t session, unsigned ticket)
 {
 	int ret;
 
@@ -201,11 +201,13 @@ static int resume_copy_required_values(gnutls_session_t session)
 	session->security_parameters.cert_type =
 	    session->internals.resumed_security_parameters.cert_type;
 
-	memcpy(session->security_parameters.session_id,
-	       session->internals.resumed_security_parameters.session_id,
-	       sizeof(session->security_parameters.session_id));
-	session->security_parameters.session_id_size =
-	    session->internals.resumed_security_parameters.session_id_size;
+	if (!ticket) {
+		memcpy(session->security_parameters.session_id,
+		       session->internals.resumed_security_parameters.session_id,
+		       sizeof(session->security_parameters.session_id));
+		session->security_parameters.session_id_size =
+		    session->internals.resumed_security_parameters.session_id_size;
+	}
 
 	return 0;
 }
@@ -667,7 +669,7 @@ read_client_hello(gnutls_session_t session, uint8_t * data,
 			if (ret < 0)
 				return gnutls_assert_val(ret);
 
-			ret = resume_copy_required_values(session);
+			ret = tls12_resume_copy_required_vals(session, 0);
 			if (ret < 0)
 				return gnutls_assert_val(ret);
 
@@ -724,12 +726,6 @@ read_client_hello(gnutls_session_t session, uint8_t * data,
 
 	/* resumed by session_ticket extension */
 	if (!vers->tls13_sem && session->internals.resumed != RESUME_FALSE) {
-		/* to indicate the client that the current session is resumed */
-		memcpy(session->internals.resumed_security_parameters.
-		       session_id, session_id, session_id_len);
-		session->internals.resumed_security_parameters.
-		    session_id_size = session_id_len;
-
 		session->internals.resumed_security_parameters.
 		    max_record_recv_size =
 		    session->security_parameters.max_record_recv_size;
@@ -741,10 +737,14 @@ read_client_hello(gnutls_session_t session, uint8_t * data,
 		if (ret < 0)
 			return gnutls_assert_val(ret);
 
-		ret = resume_copy_required_values(session);
+		ret = tls12_resume_copy_required_vals(session, 1);
 		if (ret < 0)
 			return gnutls_assert_val(ret);
 
+		/* to indicate to the client that the current session is resumed */
+		memcpy(session->security_parameters.session_id, session_id, session_id_len);
+		session->security_parameters.session_id_size = session_id_len;
+
 		return 0;
 	}
 
@@ -2307,6 +2307,18 @@ int _gnutls_send_server_hello(gnutls_session_t session, int again)
 				gnutls_assert();
 				goto fail;
 			}
+
+			/* Under TLS1.3, the session ID is used for different purposes than
+			 * the TLS1.0 session ID. Ensure that there is an internally set
+			 * value which the server will see on the original and resumed sessions */
+			ret = _gnutls_generate_session_id(session->security_parameters.
+							  session_id,
+							  &session->security_parameters.
+							  session_id_size);
+			if (ret < 0) {
+				gnutls_assert();
+				goto fail;
+			}
 		}
 
 		bufel = _gnutls_buffer_to_mbuffer(&buf);
@@ -3411,15 +3423,15 @@ static int handshake_server(gnutls_session_t session)
 	return _gnutls_check_id_for_change(session);
 }
 
-int _gnutls_generate_session_id(uint8_t * session_id, uint8_t * len)
+int _gnutls_generate_session_id(uint8_t * session_id, uint8_t *len)
 {
 	int ret;
 
-	*len = GNUTLS_MAX_SESSION_ID_SIZE;
+	*len = GNUTLS_DEF_SESSION_ID_SIZE;
 
 	ret =
 	    gnutls_rnd(GNUTLS_RND_NONCE, session_id,
-			GNUTLS_MAX_SESSION_ID_SIZE);
+			GNUTLS_DEF_SESSION_ID_SIZE);
 	if (ret < 0) {
 		gnutls_assert();
 		return ret;
diff --git a/lib/session.c b/lib/session.c
index 08da6e8d00..90e3a78a4a 100644
--- a/lib/session.c
+++ b/lib/session.c
@@ -163,20 +163,23 @@ gnutls_session_get_data2(gnutls_session_t session, gnutls_datum_t *data)
  * @session_id: is a pointer to space to hold the session id.
  * @session_id_size: initially should contain the maximum @session_id size and will be updated.
  *
- * Returns the current session ID. This can be used if you want to
- * check if the next session you tried to resume was actually
- * resumed.  That is because resumed sessions share the same session ID
- * with the original session.
- *
- * The session ID is selected by the server, that identify the
- * current session.  In all supported TLS protocols, the session id
- * is less than %GNUTLS_MAX_SESSION_ID_SIZE.
- *
- * The TLS session ID cannot be relied for uniquely identifying sessions.
- * It should not be used by new applications, nor be assumed
- * that it will remain the same on resumption. The session ID changes
- * even on resumed sessions under session tickets, and has no defined meaning
- * under TLS 1.3.
+ * Returns the TLS session identifier. The session ID is selected by the
+ * server, and in older versions of TLS was a unique identifier shared
+ * between client and server which was persistent across resumption.
+ * In the latest version of TLS (1.3) or TLS with session tickets, the
+ * notion of session identifiers is undefined and cannot be relied for uniquely
+ * identifying sessions across client and server.
+ *
+ * In client side this function returns the identifier returned by the
+ * server, and cannot be assumed to have any relation to session resumption.
+ * In server side this function is guarranteed to return a persistent
+ * identifier of the session since GnuTLS 3.6.4, which may not necessarily
+ * map into the TLS session ID value. Prior to that version the value
+ * could only be considered a persistent identifier, under TLS1.2 or earlier
+ * and when no session tickets were in use.
+ *
+ * The session identifier value returned is always less than
+ * %GNUTLS_MAX_SESSION_ID_SIZE.
  *
  * Returns: On success, %GNUTLS_E_SUCCESS (0) is returned, otherwise
  *   an error code is returned.
@@ -210,14 +213,23 @@ gnutls_session_get_id(gnutls_session_t session,
  * @session: is a #gnutls_session_t type.
  * @session_id: will point to the session ID.
  *
- * Returns the current session ID. The returned data should be
- * treated as constant.
- *
- * The TLS session ID cannot be relied for uniquely identifying sessions.
- * It should not be used by new applications, nor be assumed
- * that it will remain the same on resumption. The session ID changes
- * even on resumed sessions under session tickets, and has no defined meaning
- * under TLS 1.3.
+ * Returns the TLS session identifier. The session ID is selected by the
+ * server, and in older versions of TLS was a unique identifier shared
+ * between client and server which was persistent across resumption.
+ * In the latest version of TLS (1.3) or TLS 1.2 with session tickets, the
+ * notion of session identifiers is undefined and cannot be relied for uniquely
+ * identifying sessions across client and server.
+ *
+ * In client side this function returns the identifier returned by the
+ * server, and cannot be assumed to have any relation to session resumption.
+ * In server side this function is guarranteed to return a persistent
+ * identifier of the session since GnuTLS 3.6.4, which may not necessarily
+ * map into the TLS session ID value. Prior to that version the value
+ * could only be considered a persistent identifier, under TLS1.2 or earlier
+ * and when no session tickets were in use.
+ *
+ * The session identifier value returned is always less than
+ * %GNUTLS_MAX_SESSION_ID_SIZE and should be treated as constant.
  *
  * Returns: On success, %GNUTLS_E_SUCCESS (0) is returned, otherwise
  *   an error code is returned.
diff --git a/lib/session_pack.c b/lib/session_pack.c
index 2ed04a8eeb..a8659b5a2d 100644
--- a/lib/session_pack.c
+++ b/lib/session_pack.c
@@ -886,6 +886,11 @@ pack_security_parameters(gnutls_session_t session, gnutls_buffer_st * ps)
 	BUFFER_APPEND_NUM(ps,
 			  session->security_parameters.server_auth_type);
 
+	BUFFER_APPEND(ps, &session->security_parameters.session_id_size,
+		      1);
+	BUFFER_APPEND(ps, session->security_parameters.session_id,
+		      session->security_parameters.session_id_size);
+
 	BUFFER_APPEND_NUM(ps, session->security_parameters.pversion->id);
 
 	/* if we are under TLS 1.3 do not pack keys or params negotiated using an extension
@@ -902,11 +907,6 @@ pack_security_parameters(gnutls_session_t session, gnutls_buffer_st * ps)
 		BUFFER_APPEND_PFX1(ps, session->security_parameters.server_random,
 			      GNUTLS_RANDOM_SIZE);
 
-		BUFFER_APPEND(ps, &session->security_parameters.session_id_size,
-			      1);
-		BUFFER_APPEND(ps, session->security_parameters.session_id,
-			      session->security_parameters.session_id_size);
-
 		BUFFER_APPEND_NUM(ps,
 				  session->security_parameters.
 				  max_record_send_size);
@@ -977,6 +977,16 @@ unpack_security_parameters(gnutls_session_t session, gnutls_buffer_st * ps)
 		       session->internals.resumed_security_parameters.
 		       server_auth_type);
 
+	BUFFER_POP(ps,
+		   &session->internals.resumed_security_parameters.
+		   session_id_size, 1);
+
+	BUFFER_POP(ps,
+		   session->internals.resumed_security_parameters.
+		   session_id,
+		   session->internals.resumed_security_parameters.
+		   session_id_size);
+
 	BUFFER_POP_NUM(ps, version);
 	session->internals.resumed_security_parameters.pversion =
 	    version_to_entry(version);
@@ -1024,15 +1034,6 @@ unpack_security_parameters(gnutls_session_t session, gnutls_buffer_st * ps)
 		if (t.size == GNUTLS_RANDOM_SIZE)
 			memcpy(session->internals.resumed_security_parameters.server_random, t.data, t.size);
 
-		BUFFER_POP(ps,
-			   &session->internals.resumed_security_parameters.
-			   session_id_size, 1);
-
-		BUFFER_POP(ps,
-			   session->internals.resumed_security_parameters.
-			   session_id,
-			   session->internals.resumed_security_parameters.
-			   session_id_size);
 
 		BUFFER_POP_NUM(ps,
 			       session->internals.resumed_security_parameters.
diff --git a/src/serv.c b/src/serv.c
index 0edb176f59..ffaffc98ae 100644
--- a/src/serv.c
+++ b/src/serv.c
@@ -570,17 +570,15 @@ static char *peer_print_info(gnutls_session_t session, int *ret_length,
 	version = gnutls_protocol_get_version(session);
 
 	/* print session_id */
-	if (version < GNUTLS_TLS1_3) {
-		sesid_size = sizeof(sesid);
-		gnutls_session_get_id(session, sesid, &sesid_size);
-		snprintf(tmp_buffer, tmp_buffer_size, "\n<p>Session ID: <i>");
-		for (i = 0; i < sesid_size; i++)
-			snprintf(tmp_buffer, tmp_buffer_size, "%.2X", sesid[i]);
-		snprintf(tmp_buffer, tmp_buffer_size, "</i></p>\n");
-		snprintf(tmp_buffer, tmp_buffer_size,
-			 "<h5>If your browser supports session resuming, then you should see the "
-			 "same session ID, when you press the <b>reload</b> button.</h5>\n");
-	}
+	sesid_size = sizeof(sesid);
+	gnutls_session_get_id(session, sesid, &sesid_size);
+	snprintf(tmp_buffer, tmp_buffer_size, "\n<p>Session ID: <i>");
+	for (i = 0; i < sesid_size; i++)
+		snprintf(tmp_buffer, tmp_buffer_size, "%.2X", sesid[i]);
+	snprintf(tmp_buffer, tmp_buffer_size, "</i></p>\n");
+	snprintf(tmp_buffer, tmp_buffer_size,
+		 "<h5>If your browser supports session resumption, then you should see the "
+		 "same session ID, when you press the <b>reload</b> button.</h5>\n");
 
 	/* Here unlike print_info() we use the kx algorithm to distinguish
 	 * the functions to call.
diff --git a/tests/resume.c b/tests/resume.c
index 1a08e0f0bb..c0217b5dc0 100644
--- a/tests/resume.c
+++ b/tests/resume.c
@@ -383,9 +383,6 @@ static void verify_server_params(gnutls_session_t session, unsigned counter, str
 		char id2[GNUTLS_MAX_SESSION_ID];
 		size_t id2_size = sizeof(id2);
 
-		if (params->enable_session_ticket_client && params->enable_session_ticket_server)
-			goto finish;
-
 		if (id_size == 0)
 			fail("no session ID was set\n");
 
@@ -452,7 +449,7 @@ static void client(int sds[], struct params_res *params)
 
 	if (debug) {
 		gnutls_global_set_log_function(tls_log_func);
-		gnutls_global_set_log_level(2);
+		gnutls_global_set_log_level(4);
 	}
 
 #ifdef USE_PSK
@@ -726,7 +723,7 @@ static void server(int sds[], struct params_res *params)
 	 */
 	if (debug) {
 		gnutls_global_set_log_function(tls_log_func);
-		gnutls_global_set_log_level(2);
+		gnutls_global_set_log_level(4);
 	}
 
 #ifdef USE_PSK