GNOME Bugzilla – Bug 745255
Add support for copying sessions between GTlsClientConnections
Last modified: 2019-08-14 14:50:39 UTC
To implement ftps support, control over what TLS session a connection uses is needed. We need an API for this.
Created attachment 298032 [details] [review] gnutls: Add support for copying session data Add support for copying session data between client connections. This is needed for implementing ftps. Most servers use a separate session for each control connection and enforce sharing of each control connection's session between the related data connection. Copying session data between two connections is needed for two reasons: 1) The data connection runs on a separate port and so has a different server_identity which means it would not normally share the session with the control connection. 2) It is typical to have multiple control connections, each of which uses a different session with the same server_identity, so only one of these sessions gets stored in the cache. If a data connection is opened, (ignoring the port issue) it may try and reuse the wrong control connection's session, and fail. This operation is conceptually the same as OpenSSL's SSL_copy_session_id operation.
Created attachment 298033 [details] [review] tls: Add support for copying session data Add support for copying session data between client connections. This is needed for implementing ftps. Most servers use a separate session for each control connection and enforce sharing of each control connection's session between the related data connection. Copying session data between two connections is needed for two reasons: 1) The data connection runs on a separate port and so has a different server_identity which means it would not normally share the session with the control connection. 2) It is typical to have multiple control connections, each of which uses a different session with the same server_identity, so only one of these sessions gets stored in the cache. If a data connection is opened, (ignoring the port issue) it may try and reuse the wrong control connection's session, and fail. This operation is conceptually the same as OpenSSL's SSL_copy_session_id operation.
The first patch implements support in glib-networking, the second patch implements support in glib.
Created attachment 298048 [details] [review] gnutls: Add support for copying session data Add support for copying session data between client connections. This is needed for implementing ftps. Most servers use a separate session for each control connection and enforce sharing of each control connection's session between the related data connection. Copying session data between two connections is needed for two reasons: 1) The data connection runs on a separate port and so has a different server_identity which means it would not normally share the session with the control connection. 2) It is typical to have multiple control connections, each of which uses a different session with the same server_identity, so only one of these sessions gets stored in the cache. If a data connection is opened, (ignoring the port issue) it may try and reuse the wrong control connection's session, and fail. This operation is conceptually the same as OpenSSL's SSL_copy_session_id operation.
Comment on attachment 298033 [details] [review] tls: Add support for copying session data >+ * g_tls_client_connection_copy_session: >+ * @conn: a #GTlsClientConnection >+ * @other: a #GTlsClientConnection rename "other" to "source"? >+ * Copies a session from one connection to another. This is >+ * not normally needed, but may be used when the same session >+ * needs to be used between different endpoints. A "session" is the communication going on over the TLS connection. This function copies "session state". So I'd change "session" to "session state" in the function name and the first sentence (but not the second sentence). Maybe add something like "as is required by some protocols such as FTP over TLS" to the second sentence. >+ * Since: 2.44 Too late for 2.44 at this point
Comment on attachment 298048 [details] [review] gnutls: Add support for copying session data >- if (gnutls->priv->session_id) >+ g_clear_pointer (&gnutls->priv->session_data, g_bytes_unref); >+ if (gnutls->priv->session_data_override) >+ gnutls->priv->session_data_override = FALSE; >+ else if (gnutls->priv->session_id) > g_tls_backend_gnutls_remove_session (GNUTLS_CLIENT, gnutls->priv->session_id); This seems inconsistent; if we use the "normal" session data and fail, then we remove it from the session cache. But if we use overridden session data and fail, we leave it in the cache... Check how the openssl API behaves? >+g_tls_client_connection_gnutls_copy_session (GTlsClientConnection *conn, >+ gnutls->priv->session_data = g_bytes_ref (other->priv->session_data); rather than adding the session_data member and copying that, couldn't you just copy over priv->session_id?
Created attachment 299253 [details] [review] tls: Add support for copying session data Add support for copying session data between client connections. This is needed for implementing FTP over SSL. Most servers use a separate session for each control connection and enforce sharing of each control connection's session between the related data connection. Copying session data between two connections is needed for two reasons: 1) The data connection runs on a separate port and so has a different server_identity which means it would not normally share the session with the control connection using the session caching currently implemented. 2) It is typical to have multiple control connections, each of which uses a different session with the same server_identity, so only one of these sessions gets stored in the cache. If a data connection is opened, (ignoring the port issue) it may try and reuse the wrong control connection's session, and fail. This operation is conceptually the same as OpenSSL's SSL_copy_session_id operation.
Created attachment 299254 [details] [review] gnutls: Add support for copying session data Add support for copying session data between client connections. This is needed for implementing FTP over TLS. Most servers use a separate session for each control connection and enforce sharing of each control connection's session between the related data connection. Copying session data between two connections is needed for two reasons: 1) The data connection runs on a separate port and so has a different server_identity which means it would not normally share the session with the control connection using the session caching currently implemented. 2) It is typical to have multiple control connections, each of which uses a different session with the same server_identity, so only one of these sessions gets stored in the cache. If a data connection is opened, (ignoring the port issue) it may try and reuse the wrong control connection's session, and fail. This operation is conceptually the same as OpenSSL's SSL_copy_session_id operation.
(In reply to Dan Winship from comment #6) > Comment on attachment 298048 [details] [review] [review] > gnutls: Add support for copying session data > > >- if (gnutls->priv->session_id) > >+ g_clear_pointer (&gnutls->priv->session_data, g_bytes_unref); > >+ if (gnutls->priv->session_data_override) > >+ gnutls->priv->session_data_override = FALSE; > >+ else if (gnutls->priv->session_id) > > g_tls_backend_gnutls_remove_session (GNUTLS_CLIENT, gnutls->priv->session_id); > > This seems inconsistent; if we use the "normal" session data and fail, then > we remove it from the session cache. But if we use overridden session data > and fail, we leave it in the cache... > > Check how the openssl API behaves? No, the overridden session data never went into the cache. Since copying the session data from one connection to another is a bit "special", I'm not sure if it should go in the cache. But since it makes the code simpler, Ive done that in the updated patch. > > >+g_tls_client_connection_gnutls_copy_session (GTlsClientConnection *conn, > > >+ gnutls->priv->session_data = g_bytes_ref (other->priv->session_data); > > rather than adding the session_data member and copying that, couldn't you > just copy over priv->session_id? No. Consider the following situation, connecting to a multiprocess ftp daemon: Connect to port 21, connection A, session=1. Session 1 is stored in the cache. Connect to port 21, connection B, session=2. Session 2 replaces session 1 in the cache since they have the same session_id. Connect to port 45534, data connection for A. By reusing connection A's session_id, it will use Session 2 from the cache and fail. Thus we need to ensure that the actual session state that connection A used is reused for the data connection for A. Changes in the updated patches: - Updated function name and description. - Changed glib api version to 2.46 - simplified the code by storing the session data in the cache.
Comment on attachment 299253 [details] [review] tls: Add support for copying session data OK, this patch is basically ready to go as soon as the other patch is ready. Just one thing: >+void >+g_tls_client_connection_copy_session_state (GTlsClientConnection *conn, >+ GTlsClientConnection *source) >+{ >+ g_return_val_if_fail (G_IS_TLS_CLIENT_CONNECTION (conn), NULL); >+ g_return_val_if_fail (G_IS_TLS_CLIENT_CONNECTION (source), NULL); >+ >+ return G_TLS_CLIENT_CONNECTION_GET_INTERFACE (conn)->copy_session_state (conn, >+ source); >+} g_return_if_fail(), not g_return_val_if_fail(), and remove the return from the last line. Also, it might be nice to add g_return_if_fail (G_TLS_CLIENT_CONNECTION_GET_INTERFACE (conn)->copy_session_state != NULL); so that if you run against an old glib-networking, you just get a warning rather than a crash.
Comment on attachment 299254 [details] [review] gnutls: Add support for copying session data >+ if (*inout_error || >+ !gnutls_session_is_resumed (g_tls_connection_gnutls_get_session (conn))) > { >+ /* Clear session data since the server did not accept what we provided. */ >+ gnutls->priv->session_data_override = FALSE; >+ g_clear_pointer (&gnutls->priv->session_data, g_bytes_unref); >+ if (gnutls->priv->session_id) >+ g_tls_backend_gnutls_remove_session (GNUTLS_CLIENT, gnutls->priv->session_id); >+ >+ /* If the session was resumed */ > if (!*inout_error) The comment should say "If the session was NOT resumed" shouldn't it? It might be clearer to just call gnutls_session_is_resumed() again though. Or else save the earlier value to a variable, and check it again here. That way this won't break if someone changes the outer if() condition. >+static void >+g_tls_client_connection_gnutls_copy_session_state (GTlsClientConnection *conn, >+ GTlsClientConnection *source_) >+{ >+ GTlsClientConnectionGnutls *gnutls = G_TLS_CLIENT_CONNECTION_GNUTLS (conn); >+ GTlsClientConnectionGnutls *source = G_TLS_CLIENT_CONNECTION_GNUTLS (source_); I don't like "source_" vs "source". I'd go for "GTlsClientConnection *source" and "GTlsClientConnectionGnutls *gnutls_source".
Created attachment 300960 [details] [review] tls: Add support for copying session data Add support for copying session data between client connections. This is needed for implementing FTP over SSL. Most servers use a separate session for each control connection and enforce sharing of each control connection's session between the related data connection. Copying session data between two connections is needed for two reasons: 1) The data connection runs on a separate port and so has a different server_identity which means it would not normally share the session with the control connection using the session caching currently implemented. 2) It is typical to have multiple control connections, each of which uses a different session with the same server_identity, so only one of these sessions gets stored in the cache. If a data connection is opened, (ignoring the port issue) it may try and reuse the wrong control connection's session, and fail. This operation is conceptually the same as OpenSSL's SSL_copy_session_id operation.
Created attachment 300961 [details] [review] gnutls: Add support for copying session data Add support for copying session data between client connections. This is needed for implementing FTP over TLS. Most servers use a separate session for each control connection and enforce sharing of each control connection's session between the related data connection. Copying session data between two connections is needed for two reasons: 1) The data connection runs on a separate port and so has a different server_identity which means it would not normally share the session with the control connection using the session caching currently implemented. 2) It is typical to have multiple control connections, each of which uses a different session with the same server_identity, so only one of these sessions gets stored in the cache. If a data connection is opened, (ignoring the port issue) it may try and reuse the wrong control connection's session, and fail. This operation is conceptually the same as OpenSSL's SSL_copy_session_id operation.
Thanks for the review. (In reply to Dan Winship from comment #10) > Comment on attachment 299253 [details] [review] [review] > tls: Add support for copying session data > > OK, this patch is basically ready to go as soon as the other patch is ready. > Just one thing: > > >+void > >+g_tls_client_connection_copy_session_state (GTlsClientConnection *conn, > >+ GTlsClientConnection *source) > >+{ > >+ g_return_val_if_fail (G_IS_TLS_CLIENT_CONNECTION (conn), NULL); > >+ g_return_val_if_fail (G_IS_TLS_CLIENT_CONNECTION (source), NULL); > >+ > >+ return G_TLS_CLIENT_CONNECTION_GET_INTERFACE (conn)->copy_session_state (conn, > >+ source); > >+} > > g_return_if_fail(), not g_return_val_if_fail(), and remove the return from > the last line. Done. > > Also, it might be nice to add > > g_return_if_fail (G_TLS_CLIENT_CONNECTION_GET_INTERFACE > (conn)->copy_session_state != NULL); > > so that if you run against an old glib-networking, you just get a warning > rather than a crash. Done. (In reply to Dan Winship from comment #11) > Comment on attachment 299254 [details] [review] [review] > gnutls: Add support for copying session data > > >+ if (*inout_error || > >+ !gnutls_session_is_resumed (g_tls_connection_gnutls_get_session (conn))) > > { > >+ /* Clear session data since the server did not accept what we provided. */ > >+ gnutls->priv->session_data_override = FALSE; > >+ g_clear_pointer (&gnutls->priv->session_data, g_bytes_unref); > >+ if (gnutls->priv->session_id) > >+ g_tls_backend_gnutls_remove_session (GNUTLS_CLIENT, gnutls->priv->session_id); > >+ > >+ /* If the session was resumed */ > > if (!*inout_error) > > The comment should say "If the session was NOT resumed" shouldn't it? Whoops, yes. > > It might be clearer to just call gnutls_session_is_resumed() again though. > Or else save the earlier value to a variable, and check it again here. That > way this won't break if someone changes the outer if() condition. OK, I've simplified it in the latest patch by separating the ifs. > > >+static void > >+g_tls_client_connection_gnutls_copy_session_state (GTlsClientConnection *conn, > >+ GTlsClientConnection *source_) > >+{ > >+ GTlsClientConnectionGnutls *gnutls = G_TLS_CLIENT_CONNECTION_GNUTLS (conn); > >+ GTlsClientConnectionGnutls *source = G_TLS_CLIENT_CONNECTION_GNUTLS (source_); > > I don't like "source_" vs "source". I'd go for "GTlsClientConnection > *source" and > "GTlsClientConnectionGnutls *gnutls_source". Done.
Thanks for the reviews!
Ross, did you wind up using this for FTPS? I don't see any users of this function anywhere in Debian. Please pop into https://gitlab.gnome.org/GNOME/glib/merge_requests/1039 and leave a comment there regarding your thoughts on the future of this function. Thanks!