GNOME Bugzilla – Bug 697908
Implement Datagram TLS in gnutls backend
Last modified: 2017-11-02 16:27:02 UTC
Based on the datagram support in bug #697907, addding DTLS support to the GnuTLS backend is easy.
Created attachment 241385 [details] [review] gnutls: Fix deprecation warnings
Created attachment 241386 [details] [review] tlsconnection-gnutls: Add DTLS support
Created attachment 241387 [details] [review] tlsconnection-gnutls: Initialize the gnutls_session in initiable_init This way, we are sure that all of the properties have been set by that point. And the GIOStream needs to be set before creating the GnuTLS session to know if it's a datagram session or not.
Created attachment 241388 [details] [review] tlsconnection-gnutls: Implement SRTP related functions
Created attachment 241389 [details] [review] tlsconnection-gnutls: Convert MTU-exceeded errors
Created attachment 241390 [details] [review] tlsconnection-gnutls: Implement status property
Created attachment 241391 [details] [review] tlsconnection-gnutls: Implement SRTP related functions
Created attachment 241392 [details] [review] tlsconnection-gnutls: Convert MTU-exceeded errors
Created attachment 241393 [details] [review] tlsconnection-gnutls: Implement status property
Comment on attachment 241385 [details] [review] gnutls: Fix deprecation warnings The reason I haven't fixed all the deprecated stuff yet is that we still support gnutls 2.12. This is primarily because Fedora still doesn't ship 3.x, at least as of F18. I think this is going to be fixed finally in F19, and if so, then I guess we can probably drop 2.x support in this cycle. But in that case this would need a small configure.ac patch too. (The alternative is to postpone most of this patch for now, and put #ifdefs around the DTLS stuff.)
Comment on attachment 241386 [details] [review] tlsconnection-gnutls: Add DTLS support >+static int >+g_tls_connection_gnutls_pull_timeout_func (gnutls_transport_ptr_t transport_data, >+ unsigned int ms) Ugh. We really need a method that's like poll() except that it takes GSources instead of fds... I don't know if it's a huge improvement, but you could ditch the timeout source and just call g_source_set_ready_time (read_source, g_get_monotonic_time () + ms * 1000); (new 2.36 API) Or alternatively, if you made the read_source be a child source of the timeout_source then you wouldn't need source_loop_quit()... >+ if (g_pollable_input_stream_is_readable (G_POLLABLE_INPUT_STREAM (gnutls->priv->base_istream))) >+ return 1; >+ else >+ return 0; You need to check g_cancellable_is_cancelled (gnutls->priv->read_cancellable) too. You could either return success in that case (and let the followup call to the pull_func return an error) or call gnutls_transport_set_errno(EINTR) and return -1. Should get basically the same effect either way (but with slightly different gnutls debug logging).
Comment on attachment 241387 [details] [review] tlsconnection-gnutls: Initialize the gnutls_session in initiable_init >And the GIOStream needs to be set before creating the GnuTLS session to know >if it's a datagram session or not. Hm... does that mean that the previous patch doesn't actually work without this one? If so, you should reorder them. > g_tls_client_connection_gnutls_get_property (GObject *object, >+ /* This will only be triggered if the identity is set at the start */ >+ if (session) >+ gnutls_server_name_set (session, GNUTLS_NAME_DNS, >+ hostname, strlen (hostname)); I think that comment is backwards; it will only be triggered if the identity is changed *after* the connection is first created.
Comment on attachment 241391 [details] [review] tlsconnection-gnutls: Implement SRTP related functions should be on the DTLS-SRTP bug
Comment on attachment 241392 [details] [review] tlsconnection-gnutls: Convert MTU-exceeded errors >+ /* Don't enforce MTU */ >+ if (flags & GNUTLS_DATAGRAM) >+ gnutls_dtls_set_mtu (gnutls->priv->session, 65535); Hm. Why? Is this something that other DTLS users might want a different behavior for? >+ if (gnutls_dtls_get_data_mtu (gnutls->priv->session) < count) { >+ ret = GNUTLS_E_LARGE_PACKET; >+ g_set_error (error, G_IO_ERROR, G_IO_ERROR_MESSAGE_TOO_LARGE, >+ "Trying to write %" G_GSIZE_FORMAT " bytes, but MTU is %u", >+ count, gnutls_dtls_get_data_mtu (gnutls->priv->session)); I thought we weren't enforcing MTU? The brace on the first line should be on a separate line, and the error message needs _(). Which won't work with G_GSIZE_FORMAT, but I don't like G_GSIZE_FORMAT anyway; just use %lu and cast count to gulong.
(In reply to comment #14) > (From update of attachment 241392 [details] [review]) > >+ /* Don't enforce MTU */ > >+ if (flags & GNUTLS_DATAGRAM) > >+ gnutls_dtls_set_mtu (gnutls->priv->session, 65535); > > Hm. Why? Is this something that other DTLS users might want a different > behavior for? I think ideally, each GIOStream would have a get_mtu() method that the upper one can read. But that would first require implementing MTU Path discovery in GSocket and that looks like a portability pain so I'm not super excited about doing it. > >+ if (gnutls_dtls_get_data_mtu (gnutls->priv->session) < count) { > >+ ret = GNUTLS_E_LARGE_PACKET; > >+ g_set_error (error, G_IO_ERROR, G_IO_ERROR_MESSAGE_TOO_LARGE, > >+ "Trying to write %" G_GSIZE_FORMAT " bytes, but MTU is %u", > >+ count, gnutls_dtls_get_data_mtu (gnutls->priv->session)); > > I thought we weren't enforcing MTU? We're not, but GnuTLS is, so we must somehow return to the user errors, such as if the user tries to send more than 64k of data. Also DTLS adds an extra header, so the MTU is actually smaller than 64k.
Comment on attachment 241393 [details] [review] tlsconnection-gnutls: Implement status property >+ else if (gnutls->priv->started_handshake || >+ gnutls->priv->handshaking || >+ gnutls->priv->need_handshake || >+ gnutls->priv->need_finish_handshake) >+ { >+ if (gnutls->priv->ever_handshaked) >+ g_value_set_enum (value, G_TLS_STATUS_REHANDSHAKING); >+ else >+ g_value_set_enum (value, G_TLS_STATUS_HANDSHAKING); >+ } ever_handshaked actually means "gnutls_handshake() has ever succeeded", not "g_tls_connection_handshake() has ever succeeded". In particular, it gets set while need_finish_handshake is still TRUE in the async case, so there'd be a span of time where this code would mistakenly categorize the initial handshake as being REHANDSHAKING. I think that moving the "ever_handshaked = TRUE" from handshake_thread() to finish_handshake() should fix things. You want to set it if the task returns TRUE (even if accept_peer_certificate() then rejects the certificate).
Comment on attachment 241393 [details] [review] tlsconnection-gnutls: Implement status property Actually only really needed for DTLS-SRTP (so we can know if the key has changed), so moving to bug #697909
Created attachment 241692 [details] [review] tlsconnection-gnutls: Add DTLS support Updated based on your comments
Created attachment 241693 [details] [review] tlsconnection-gnutls: Initialize the gnutls_session in initiable_init This way, we are sure that all of the properties have been set by that point. And the GIOStream needs to be set before creating the GnuTLS session to know if it's a datagram session or not.
(In reply to comment #16) > ever_handshaked actually means "gnutls_handshake() has ever succeeded", not > "g_tls_connection_handshake() has ever succeeded". In particular, it gets set > while need_finish_handshake is still TRUE in the async case, so there'd be a > span of time where this code would mistakenly categorize the initial handshake > as being REHANDSHAKING. > > I think that moving the "ever_handshaked = TRUE" from handshake_thread() to > finish_handshake() should fix things. You want to set it if the task returns > TRUE (even if accept_peer_certificate() then rejects the certificate). Does that also mean that if accept-peer-certificate rejects the cert, it should actually be HANDSHAKING, not re-handshaking ? And I'm worried that moving ever_handshaked=TRUE to finish might break things if one calls close() before finishing the handshake ?
Potentially we can get merge the handshaking and rehandshaking status, I'm not sure the difference is that important.
Created attachment 241698 [details] [review] tlsconnection-gnutls: Convert MTU-exceeded errors
Comment on attachment 241385 [details] [review] gnutls: Fix deprecation warnings OK, yes, please remove the gnutls 2.12 support (configure checks and ifdefs) along with this patch
Comment on attachment 241692 [details] [review] tlsconnection-gnutls: Add DTLS support > GCancellable *write_cancellable; > >+ > #ifndef GNUTLS_E_PREMATURE_TERMINATION stray newline there. everything else looks good
Comment on attachment 241693 [details] [review] tlsconnection-gnutls: Initialize the gnutls_session in initiable_init >Subject: [PATCH] tlsconnection-gnutls: Initialize the gnutls_session in > initiable_init typo ("initable" not "initiable"). otherwise good
Comment on attachment 241698 [details] [review] tlsconnection-gnutls: Convert MTU-exceeded errors >+ if (gnutls_dtls_get_data_mtu (gnutls->priv->session) < count) { >+ ret = GNUTLS_E_LARGE_PACKET; wrong indent style >+ _("Trying to write %lu bytes, but MTU is %u"), That's a little technical... I'd go with something like "message is too large for this DTLS connection" or something.
Created attachment 245691 [details] [review] tlsconnection-gnutls: Add DTLS support Fixed stray newline, updated for API changes
Created attachment 245692 [details] [review] tlsconnection-gnutls: Initialize the gnutls_session in initable_init Fix title
Created attachment 245693 [details] [review] tlsconnection-gnutls: Convert MTU-exceeded errors Fixed indent and message text
Comment on attachment 241385 [details] [review] gnutls: Fix deprecation warnings Attachment 241385 [details] pushed as b20330d - gnutls: Fix deprecation warnings
Adding dependencies on bug #735754 and bug #736809 because I’m rebasing the patch set on top of those and testing is going to be a nightmare otherwise.
Created attachment 307246 [details] [review] gnutls: Fix a potential release of a non-allocated FD If the initial allocation of the poll FD for this cancellable fails, there is no FD to release, so don’t try.
Created attachment 307247 [details] [review] gnutls: Initialize the gnutls_session in initable_init This way, we are sure that all of the properties have been set by that point. And the GIOStream needs to be set before creating the GnuTLS session to know if it's a datagram session or not.
Created attachment 307248 [details] [review] gnutls: Add support for timeouts on GnuTLS pulls Set a gnutls_pull_timeout_func for GnuTLS to use. This is necessary for supporting timeouts and for DTLS.
Created attachment 307249 [details] [review] gnutls: Convert MTU-exceeded errors This is necessary for handling DTLS datagrams, as GnuTLS enforces an MTU on them.
Created attachment 307250 [details] [review] gnutls: Define EMSGSIZE on Windows systems It isn’t clear whether all versions of MinGW define EMSGSIZE, so define it to WSAEMSGSIZE as a fallback if it isn’t defined. See: http://msdn.microsoft.com/en-us/library/windows/desktop/ms740668%28v=vs.85%29.aspx#wsaemsgsize Based on a patch originally by Luciana Fujii <luciana.fujii@collabora.com>.
Created attachment 307251 [details] [review] gnutls: Add support for DTLS to the GnuTLS backend Implement DTLS support in the GnuTLS backend by adding it to GTlsConnectionGnutls, so that it supports both TLS and DTLS (despite its name). This means the GTlsBackendGnutls returns the same type for client and server connection types — the protocol to use is determined by whether the GTlsConnection:base-io-stream or GDtlsConnection:base-communicable property is set on the GTlsConnectionGnutls. Exactly one of the two must be set. A handshake timeout has been added, set as GCommunicable:timeout. This makes the following internal API changes: • Implement GDtlsClientConnection on GTlsClientConnectionGnutls • Implement GDtlsServerConnection on GTlsServerConnectionGnutls • Implement GCommunicable on GTlsConnectionGnutls • Implement GDtlsConnection on GTlsConnectionGnutls The patchset currently doesn’t support vectored I/O. A bug has been filed against GnuTLS for adding underlying support for it: https://gitlab.com/gnutls/gnutls/issues/16. --- There is still one FIXME in the code for supporting vectored I/O, which we need to emulate — currently, it has a g_assert() that the vector is 1 element long. I haven’t had time this week to fix that, but I think the patch is ready for review otherwise. The tests pass!
Created attachment 307252 [details] [review] gnutls: Implement a vectored push callback function for GnuTLS If operating in DTLS mode, this allows GnuTLS to do vectored writes to the base GCommunicable, which should improve performance a little. This is not *required* for DTLS support, as the normal non-vectored push function also works. It is not implemented for normal TLS as GOutputStream does not support vectored I/O.
Created attachment 307253 [details] [review] tests: Add unit tests for DTLS support
Created attachment 307254 [details] [review] gnutls: Fix a minor memory leak
This updated patch set is based on the GDtlsConnection interface from bug #752240, and uses GCommunicable (bug #697907) to provide its datagram support for the base and DTLS connections. Apart from the interface churn, it’s mostly the same as Olivier’s patches. Due to GCommunicable, it has to implement support for vectored scatter–gather I/O, which is not quite complete yet. The GSource handling has changed a little as well. One unit test is provided, and passes at the moment.
Comment on attachment 307254 [details] [review] gnutls: Fix a minor memory leak Obsoleted by commit e9384e2cce11505a15a801512404bf44a40796e3.
Created attachment 308004 [details] [review] gnutls: Fix a potential release of a non-allocated FD If the initial allocation of the poll FD for this cancellable fails, there is no FD to release, so don’t try.
Created attachment 308005 [details] [review] gnutls: Initialize the gnutls_session in initable_init This way, we are sure that all of the properties have been set by that point. And the GIOStream needs to be set before creating the GnuTLS session to know if it's a datagram session or not.
Created attachment 308006 [details] [review] gnutls: Add support for timeouts on GnuTLS pulls Set a gnutls_pull_timeout_func for GnuTLS to use. This is necessary for supporting timeouts and for DTLS.
Created attachment 308007 [details] [review] gnutls: Convert MTU-exceeded errors This is necessary for handling DTLS datagrams, as GnuTLS enforces an MTU on them.
Created attachment 308008 [details] [review] gnutls: Define EMSGSIZE on Windows systems It isn’t clear whether all versions of MinGW define EMSGSIZE, so define it to WSAEMSGSIZE as a fallback if it isn’t defined. See: http://msdn.microsoft.com/en-us/library/windows/desktop/ms740668%28v=vs.85%29.aspx#wsaemsgsize Based on a patch originally by Luciana Fujii <luciana.fujii@collabora.com>.
Created attachment 308009 [details] [review] gnutls: Add support for DTLS to the GnuTLS backend Implement DTLS support in the GnuTLS backend by adding it to GTlsConnectionGnutls, so that it supports both TLS and DTLS (despite its name). This means the GTlsBackendGnutls returns the same type for client and server connection types — the protocol to use is determined by whether the GTlsConnection:base-io-stream or GDtlsConnection:base-socket property is set on the GTlsConnectionGnutls. Exactly one of the two must be set. A handshake timeout has been added, set as GDatagramBased:timeout. This makes the following internal API changes: • Implement GDtlsClientConnection on GTlsClientConnectionGnutls • Implement GDtlsServerConnection on GTlsServerConnectionGnutls • Implement GDatagramBased on GTlsConnectionGnutls • Implement GDtlsConnection on GTlsConnectionGnutls The patchset currently doesn’t support vectored I/O. A bug has been filed against GnuTLS for adding underlying support for it: https://gitlab.com/gnutls/gnutls/issues/16.
Created attachment 308010 [details] [review] gnutls: Implement a vectored push callback function for GnuTLS If operating in DTLS mode, this allows GnuTLS to do vectored writes to the base GDatagramBased, which should improve performance a little. This is not *required* for DTLS support, as the normal non-vectored push function also works. It is not implemented for normal TLS as GOutputStream does not support vectored I/O.
Created attachment 308011 [details] [review] tests: Add unit tests for DTLS support
Created attachment 308012 [details] [review] gnutls: Implement vectored I/O support for TLS and DTLS This bumps the GnuTLS dependency to 3.3.5 for gnutls_record_recv_packet(), which gives us access to the internal plaintext GnuTLS buffer, from which we can copy out to multiple GInputVectors. Similarly, add support for vectored sends, using gnutls_record_cork() (requires GnuTLS 3.1.9) to queue up multiple vectors from a single message before sending the message. Include some trivial modifications of the unit tests to use multiple vectors in a message.
Created attachment 308542 [details] [review] gnutls: Fix a potential release of a non-allocated FD If the initial allocation of the poll FD for this cancellable fails, there is no FD to release, so don’t try.
Created attachment 308543 [details] [review] gnutls: Initialize the gnutls_session in initable_init This way, we are sure that all of the properties have been set by that point. And the GIOStream needs to be set before creating the GnuTLS session to know if it's a datagram session or not.
Created attachment 308544 [details] [review] gnutls: Add support for timeouts on GnuTLS pulls Set a gnutls_pull_timeout_func for GnuTLS to use. This is necessary for supporting timeouts and for DTLS.
Created attachment 308545 [details] [review] gnutls: Convert MTU-exceeded errors This is necessary for handling DTLS datagrams, as GnuTLS enforces an MTU on them.
Created attachment 308546 [details] [review] gnutls: Define EMSGSIZE on Windows systems It isn’t clear whether all versions of MinGW define EMSGSIZE, so define it to WSAEMSGSIZE as a fallback if it isn’t defined. See: http://msdn.microsoft.com/en-us/library/windows/desktop/ms740668%28v=vs.85%29.aspx#wsaemsgsize Based on a patch originally by Luciana Fujii <luciana.fujii@collabora.com>.
Created attachment 308547 [details] [review] gnutls: Add support for DTLS to the GnuTLS backend Implement DTLS support in the GnuTLS backend by adding it to GTlsConnectionGnutls, so that it supports both TLS and DTLS (despite its name). This means the GTlsBackendGnutls returns the same type for client and server connection types — the protocol to use is determined by whether the GTlsConnection:base-io-stream or GDtlsConnection:base-socket property is set on the GTlsConnectionGnutls. Exactly one of the two must be set. This makes the following internal API changes: • Implement GDtlsClientConnection on GTlsClientConnectionGnutls • Implement GDtlsServerConnection on GTlsServerConnectionGnutls • Implement GDatagramBased on GTlsConnectionGnutls • Implement GDtlsConnection on GTlsConnectionGnutls The patch doesn’t support vectored I/O; support will follow in a later commit.
Created attachment 308548 [details] [review] gnutls: Implement a vectored push callback function for GnuTLS If operating in DTLS mode, this allows GnuTLS to do vectored writes to the base GDatagramBased, which should improve performance a little. This is not *required* for DTLS support, as the normal non-vectored push function also works. It is not implemented for normal TLS as GOutputStream does not support vectored I/O.
Created attachment 308549 [details] [review] tests: Add unit tests for DTLS support
Created attachment 308550 [details] [review] gnutls: Implement vectored I/O support for TLS and DTLS This bumps the GnuTLS dependency to 3.3.5 for gnutls_record_recv_packet(), which gives us access to the internal plaintext GnuTLS buffer, from which we can copy out to multiple GInputVectors. Similarly, add support for vectored sends, using gnutls_record_cork() (requires GnuTLS 3.1.9) to queue up multiple vectors from a single message before sending the message. Include some trivial modifications of the unit tests to use multiple vectors in a message.
Created attachment 308551 [details] [review] gnutls: Internally support per-operation timeouts For every operation except handshaking (which is complex), convert the per-operation blocking parameter to a timeout parameter, supporting blocking, non-blocking and timeout behaviour. This can be used by the GDatagramBased interface to support its per-operation timeouts.
Updated patchset which: • Rebases the API on the latest GDatagramBased design from bug #697907 • Adds limited per-operation timeout support (but not for handshakes because that’s hard) • Fixes the vectored I/O support compared with the previous patchset
These patches now need updating again for trivial API changes (to g_datagram_based_condition_timed_wait()) in bug #697907; this shouldn’t complicate the review, so I’ll only update the patches for that after the first review.
Comment on attachment 308543 [details] [review] gnutls: Initialize the gnutls_session in initable_init >+ if (hostname) >+ gnutls_server_name_set (session, GNUTLS_NAME_DNS, >+ hostname, strlen (hostname)); {}s around the if body (in two places). OK to commit with that fix
Comment on attachment 308544 [details] [review] gnutls: Add support for timeouts on GnuTLS pulls >+static gboolean >+read_cb (GPollableInputStream *istream, gpointer user_data) >+{ >+ gboolean *read_done = user_data; >+ >+ g_assert (G_IS_POLLABLE_INPUT_STREAM (istream)); Drop the assert >+g_tls_connection_gnutls_pull_timeout_func (gnutls_transport_ptr_t transport_data, It is probably worth optimizing this by checking is_readable() first, then checking ms == 0, and only then doing the source thing.
Comment on attachment 308545 [details] [review] gnutls: Convert MTU-exceeded errors >+ /* Don't enforce MTU */ >+ if (flags & GNUTLS_DATAGRAM) >+ gnutls_dtls_set_mtu (gnutls->priv->session, 65535); This seems wrong... I think you probably need to expose an :mtu property on GDtlsConnection
Comment on attachment 308547 [details] [review] gnutls: Add support for DTLS to the GnuTLS backend >- gboolean use_ssl3; >+ gboolean enable_negotiation; drop all these changes >+ /* When operating in stream mode. >+ * Mutually exclusive with base_socket. */ Nitpick: I prefer the "*/" on the next line. Otherwise it looks imbalanced. :) >+ GDatagramBased *base_socket; /* owned */ Saying "owned" here implies that everything else in the struct is not owned (as opposed to just not being labelled). So remove that. >+ /* When operating in stream mode; when operating in datagram mode, the >+ * GTlsConnectionGnutls itself is the DTLS GDatagramBased: */ > GInputStream *tls_istream; > GOutputStream *tls_ostream; Move these up with base_io_stream, etc so there's only a single stream-mode-specific section. >+is_client_connection (GTlsConnectionGnutls *gnutls) >+{ >+ return G_IS_TLS_CLIENT_CONNECTION (gnutls) || >+ G_IS_DTLS_CLIENT_CONNECTION (gnutls); >+} GTlsClientConnectionGnutls implements both interfaces, so G_IS_TLS_CLIENT_CONNECTION() will return TRUE for gnutls-based DTLS connections as well. So you don't need this function. >+ /* Either a GDatagramBased (datagram mode), or a GPollableInputStream or >+ * GPollableOutputStream (streaming mode): */ > GObject *stream; may want to rename this... >+ if (!g_cancellable_make_pollfd (gnutls->priv->waiting_for_op, &fds[0])) This can't actually fail unless the cancellable is NULL, which it isn't, so it can't. >+read_pollable_cb (GPollableInputStream *istream, >+ gpointer user_data) > { > gboolean *read_done = user_data; > > g_assert (G_IS_POLLABLE_INPUT_STREAM (istream)); >+g_assert_not_reached (); debugging leftover I assume... >+ /* FIXME: Unfortunately GnuTLS doesnât have a vectored read function. >+ * See: https://gitlab.com/gnutls/gnutls/issues/16 */ >+ g_assert (message->num_vectors == 1); /* TODO: implement vectored reads */ since you actually do implement it a few commits later... >+ >+ if (message->flags != G_SOCKET_MSG_NONE) >+ { >+ g_set_error (&child_error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT, >+ _("Receive flags are not supported")); >+ break; >+ } should do that outside of the loop like the send case
Thanks for the reviews. All these patches depend on those in bug #736809 and bug #735754 — would it be possible to get a review of those too please?
Attachment 308542 [details] pushed as 741a9dc - gnutls: Fix a potential release of a non-allocated FD Attachment 308543 [details] pushed as d393aab - gnutls: Initialize the gnutls_session in initable_init Attachment 308546 [details] pushed as 9b28d30 - gnutls: Define EMSGSIZE on Windows systems
(In reply to Philip Withnall from comment #68) > Thanks for the reviews. All these patches depend on those in bug #736809 and > bug #735754 — would it be possible to get a review of those too please? Dan? If this is to be merged this cycle we need to get a move on.
Created attachment 318393 [details] [review] dtlsconnection: Update to latest DTLS API This is really asn update on the above patch and should be squashed.
Created attachment 318394 [details] [review] dtls: Put GIOCondition in closure callback
Created attachment 318395 [details] [review] dtlsconnection: Fix handshaking when packets are lost During a handshake, the timeout is set to "-1", so blocking forever, this is wrong for DTLS, where the blocking should instead happen inside the pull_timeout function. Also, return EAGAIN during the handshake so that GnuTLS will do the re-tries for us internally and the re-sending as required.
Created attachment 318777 [details] [review] gnutls: Add support for timeouts on GnuTLS pulls Set a gnutls_pull_timeout_func for GnuTLS to use. This is necessary for supporting timeouts and for DTLS.
Created attachment 318778 [details] [review] gnutls: Convert MTU-exceeded errors This is necessary for handling DTLS datagrams, as GnuTLS enforces an MTU on them.
Created attachment 318779 [details] [review] gnutls: Add support for DTLS to the GnuTLS backend Implement DTLS support in the GnuTLS backend by adding it to GTlsConnectionGnutls, so that it supports both TLS and DTLS (despite its name). This means the GTlsBackendGnutls returns the same type for client and server connection types — the protocol to use is determined by whether the GTlsConnection:base-io-stream or GDtlsConnection:base-socket property is set on the GTlsConnectionGnutls. Exactly one of the two must be set. This makes the following internal API changes: • Implement GDtlsClientConnection on GTlsClientConnectionGnutls • Implement GDtlsServerConnection on GTlsServerConnectionGnutls • Implement GDatagramBased on GTlsConnectionGnutls • Implement GDtlsConnection on GTlsConnectionGnutls The patch doesn’t support vectored I/O; support will follow in a later commit. It includes contributions by Olivier Crête <olivier.crete@collabora.com>.
Created attachment 318780 [details] [review] gnutls: Implement a vectored push callback function for GnuTLS If operating in DTLS mode, this allows GnuTLS to do vectored writes to the base GDatagramBased, which should improve performance a little. This is not *required* for DTLS support, as the normal non-vectored push function also works. It is not implemented for normal TLS as GOutputStream does not support vectored I/O.
Created attachment 318781 [details] [review] tests: Add unit tests for DTLS support
Created attachment 318782 [details] [review] gnutls: Implement vectored I/O support for TLS and DTLS This bumps the GnuTLS dependency to 3.3.5 for gnutls_record_recv_packet(), which gives us access to the internal plaintext GnuTLS buffer, from which we can copy out to multiple GInputVectors. Similarly, add support for vectored sends, using gnutls_record_cork() (requires GnuTLS 3.1.9) to queue up multiple vectors from a single message before sending the message. Include some trivial modifications of the unit tests to use multiple vectors in a message.
Created attachment 318783 [details] [review] gnutls: Internally support per-operation timeouts For every operation except handshaking (which is complex), convert the per-operation blocking parameter to a timeout parameter, supporting blocking, non-blocking and timeout behaviour. This can be used by the GDatagramBased interface to support its per-operation timeouts.
Created attachment 318784 [details] [review] gnutls: Fix DTLS handshaking when packets are lost During a handshake, the timeout is set to "-1", so blocking forever, this is wrong for DTLS, where the blocking should instead happen inside the pull_timeout function. Also, return EAGAIN during the handshake so that GnuTLS will do the re-tries for us internally and the re-sending as required.
Updated patches which incorporate all review comments so far, are based against the latest DTLS API from GIO (i.e. incorporating Olivier’s patch from comment #71), and incorporate Olivier’s other bug fix patch from comment #72. His patch from comment #73 is kept as-is (apart from minor changes to the commit message) in the patch series. (In reply to Dan Winship from comment #65) > Comment on attachment 308544 [details] [review] [review] > gnutls: Add support for timeouts on GnuTLS pulls > > >+static gboolean > >+read_cb (GPollableInputStream *istream, gpointer user_data) > >+{ > >+ gboolean *read_done = user_data; > >+ > >+ g_assert (G_IS_POLLABLE_INPUT_STREAM (istream)); > > Drop the assert Done. > >+g_tls_connection_gnutls_pull_timeout_func (gnutls_transport_ptr_t transport_data, > > It is probably worth optimizing this by checking is_readable() first, then > checking ms == 0, and only then doing the source thing. Done. (In reply to Dan Winship from comment #66) > Comment on attachment 308545 [details] [review] [review] > gnutls: Convert MTU-exceeded errors > > >+ /* Don't enforce MTU */ > >+ if (flags & GNUTLS_DATAGRAM) > >+ gnutls_dtls_set_mtu (gnutls->priv->session, 65535); > > This seems wrong... I think you probably need to expose an :mtu property on > GDtlsConnection This should be the MTU of the base socket, so I think we need an :mtu property on GDatagramBased. In the absence of that (at the moment), I’ve dropped the gnutls_dtls_set_mtu() call. The rest of the commit still stands. (In reply to Dan Winship from comment #67) > Comment on attachment 308547 [details] [review] [review] > gnutls: Add support for DTLS to the GnuTLS backend > > >- gboolean use_ssl3; > >+ gboolean enable_negotiation; > > drop all these changes Done. With the final GDatagramBased API they weren’t necessary anyway. > >+ /* When operating in stream mode. > >+ * Mutually exclusive with base_socket. */ > > Nitpick: I prefer the "*/" on the next line. Otherwise it looks imbalanced. > :) Done (in all places I could find it, I think). > >+ GDatagramBased *base_socket; /* owned */ > > Saying "owned" here implies that everything else in the struct is not owned > (as opposed to just not being labelled). So remove that. I’ve left it and can follow up with a commit to add /* owned */ to the other appropriate members of the struct. Don’t want to do it in this commit because it’s not strictly relevant. > >+ /* When operating in stream mode; when operating in datagram mode, the > >+ * GTlsConnectionGnutls itself is the DTLS GDatagramBased: */ > > GInputStream *tls_istream; > > GOutputStream *tls_ostream; > > Move these up with base_io_stream, etc so there's only a single > stream-mode-specific section. Done. > >+is_client_connection (GTlsConnectionGnutls *gnutls) > >+{ > >+ return G_IS_TLS_CLIENT_CONNECTION (gnutls) || > >+ G_IS_DTLS_CLIENT_CONNECTION (gnutls); > >+} > > GTlsClientConnectionGnutls implements both interfaces, so > G_IS_TLS_CLIENT_CONNECTION() will return TRUE for gnutls-based DTLS > connections as well. So you don't need this function. Done. It was a remnant from a previous version of the patch where GTlsClientConnection and GDtlsClientConnection were not the same class. > >+ /* Either a GDatagramBased (datagram mode), or a GPollableInputStream or > >+ * GPollableOutputStream (streaming mode): */ > > GObject *stream; > > may want to rename this... Renamed to ‘base’. > >+ if (!g_cancellable_make_pollfd (gnutls->priv->waiting_for_op, &fds[0])) > > This can't actually fail unless the cancellable is NULL, which it isn't, so > it can't. Dropped the error. > >+read_pollable_cb (GPollableInputStream *istream, > >+ gpointer user_data) > > { > > gboolean *read_done = user_data; > > > > g_assert (G_IS_POLLABLE_INPUT_STREAM (istream)); > >+g_assert_not_reached (); > > debugging leftover I assume... Oops. Removed (and the g_assert()). > >+ /* FIXME: Unfortunately GnuTLS doesnât have a vectored read function. > >+ * See: https://gitlab.com/gnutls/gnutls/issues/16 */ > >+ g_assert (message->num_vectors == 1); > > /* TODO: implement vectored reads */ > > since you actually do implement it a few commits later... Left in place since it doesn’t really matter. > >+ > >+ if (message->flags != G_SOCKET_MSG_NONE) > >+ { > >+ g_set_error (&child_error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT, > >+ _("Receive flags are not supported")); > >+ break; > >+ } > > should do that outside of the loop like the send case Actually, the semantics here were wrong: message->flags is an (out) field. I’ve changed it to be set (to G_SOCKET_MSG_NONE) correctly, plus all the other (out) fields in GInputMessage.
Comment on attachment 318777 [details] [review] gnutls: Add support for timeouts on GnuTLS pulls >+g_tls_connection_gnutls_pull_timeout_func (gnutls_transport_ptr_t transport_data, >+ unsigned int ms) >+{ >+ GTlsConnectionGnutls *gnutls = transport_data; >+ GMainContext *ctx = g_main_context_new (); ctx gets leaked in the fast path otherwise looks good
Comment on attachment 318778 [details] [review] gnutls: Convert MTU-exceeded errors >@@ -1655,10 +1658,22 @@ g_tls_connection_gnutls_write (GTlsConnectionGnutls *gnutls, > blocking, cancellable, error)) > return -1; > >+ if (gnutls_dtls_get_data_mtu (gnutls->priv->session) < count) >+ { >+ ret = GNUTLS_E_LARGE_PACKET; This gets called even for non-DTLS connections, which is wrong. Why is it even needed? Why can't you just try to write it and let gnutls return the error for you?
Comment on attachment 318779 [details] [review] gnutls: Add support for DTLS to the GnuTLS backend >+ /* FIXME: Unfortunately GnuTLS doesnât have a vectored read function. >+ * See: https://gitlab.com/gnutls/gnutls/issues/16 */ >+ g_assert (message->num_vectors == 1); (I still don't like this. If at all possible, every commit should leave the tree in a usable state.) >+ /* We do not close underlying #GDatagramBaseds. */ Why not?
Comment on attachment 318781 [details] [review] tests: Add unit tests for DTLS support i'm just gonna assume these are fine
Comment on attachment 318783 [details] [review] gnutls: Internally support per-operation timeouts actually, ideally this would have tests
(In reply to Dan Winship from comment #83) > Comment on attachment 318777 [details] [review] [review] > gnutls: Add support for timeouts on GnuTLS pulls > > >+g_tls_connection_gnutls_pull_timeout_func (gnutls_transport_ptr_t transport_data, > >+ unsigned int ms) > >+{ > >+ GTlsConnectionGnutls *gnutls = transport_data; > >+ GMainContext *ctx = g_main_context_new (); > > ctx gets leaked in the fast path > > otherwise looks good Whoops, fixed. (In reply to Dan Winship from comment #84) > Comment on attachment 318778 [details] [review] [review] > gnutls: Convert MTU-exceeded errors > > >@@ -1655,10 +1658,22 @@ g_tls_connection_gnutls_write (GTlsConnectionGnutls *gnutls, > > blocking, cancellable, error)) > > return -1; > > > >+ if (gnutls_dtls_get_data_mtu (gnutls->priv->session) < count) > >+ { > >+ ret = GNUTLS_E_LARGE_PACKET; > > This gets called even for non-DTLS connections, which is wrong. > > Why is it even needed? Why can't you just try to write it and let gnutls > return the error for you? Indeed, that’s a better approach. Changed. (In reply to Dan Winship from comment #85) > Comment on attachment 318779 [details] [review] [review] > gnutls: Add support for DTLS to the GnuTLS backend > > >+ /* FIXME: Unfortunately GnuTLS doesnât have a vectored read function. > >+ * See: https://gitlab.com/gnutls/gnutls/issues/16 */ > >+ g_assert (message->num_vectors == 1); > > (I still don't like this. If at all possible, every commit should leave the > tree in a usable state.) Yeah, but to me it feels (marginally) better than bundling in the (largely separate) vectored I/O changes into this patch. I guess technically, having this FIXME and assertion here doesn’t actually leave the tree in an unusable state, because it builds, and none of the existing functionality is broken — you just can’t use the new DTLS stuff fully yet. > >+ /* We do not close underlying #GDatagramBaseds. */ > > Why not? Because there is no g_datagram_based_close() method (or similar). We dropped it from the interface because getting it to work for socket-like and close-handshake-based streams was too painful. (In reply to Dan Winship from comment #87) > Comment on attachment 318783 [details] [review] [review] > gnutls: Internally support per-operation timeouts > > actually, ideally this would have tests Yes, definitely. I will try and write some soon.
Created attachment 319260 [details] [review] gnutls: Add support for timeouts on GnuTLS pulls Set a gnutls_pull_timeout_func for GnuTLS to use. This is necessary for supporting timeouts and for DTLS.
Created attachment 319261 [details] [review] gnutls: Convert MTU-exceeded errors This is necessary for handling DTLS datagrams, as GnuTLS enforces an MTU on them.
Created attachment 319262 [details] [review] gnutls: Add support for DTLS to the GnuTLS backend Implement DTLS support in the GnuTLS backend by adding it to GTlsConnectionGnutls, so that it supports both TLS and DTLS (despite its name). This means the GTlsBackendGnutls returns the same type for client and server connection types — the protocol to use is determined by whether the GTlsConnection:base-io-stream or GDtlsConnection:base-socket property is set on the GTlsConnectionGnutls. Exactly one of the two must be set. This makes the following internal API changes: • Implement GDtlsClientConnection on GTlsClientConnectionGnutls • Implement GDtlsServerConnection on GTlsServerConnectionGnutls • Implement GDatagramBased on GTlsConnectionGnutls • Implement GDtlsConnection on GTlsConnectionGnutls The patch doesn’t support vectored I/O; support will follow in a later commit. It includes contributions by Olivier Crête <olivier.crete@collabora.com>.
Created attachment 319263 [details] [review] gnutls: Implement a vectored push callback function for GnuTLS If operating in DTLS mode, this allows GnuTLS to do vectored writes to the base GDatagramBased, which should improve performance a little. This is not *required* for DTLS support, as the normal non-vectored push function also works. It is not implemented for normal TLS as GOutputStream does not support vectored I/O.
Created attachment 319264 [details] [review] tests: Add unit tests for DTLS support
Created attachment 319265 [details] [review] gnutls: Implement vectored I/O support for TLS and DTLS This bumps the GnuTLS dependency to 3.3.5 for gnutls_record_recv_packet(), which gives us access to the internal plaintext GnuTLS buffer, from which we can copy out to multiple GInputVectors. Similarly, add support for vectored sends, using gnutls_record_cork() (requires GnuTLS 3.1.9) to queue up multiple vectors from a single message before sending the message. Include some trivial modifications of the unit tests to use multiple vectors in a message.
Created attachment 319266 [details] [review] gnutls: Internally support per-operation timeouts For every operation except handshaking (which is complex), convert the per-operation blocking parameter to a timeout parameter, supporting blocking, non-blocking and timeout behaviour. This can be used by the GDatagramBased interface to support its per-operation timeouts.
Created attachment 319267 [details] [review] gnutls: Fix DTLS handshaking when packets are lost During a handshake, the timeout is set to "-1", so blocking forever, this is wrong for DTLS, where the blocking should instead happen inside the pull_timeout function. Also, return EAGAIN during the handshake so that GnuTLS will do the re-tries for us internally and the re-sending as required.
(In reply to Dan Winship from comment #87) > Comment on attachment 318783 [details] [review] [review] > gnutls: Internally support per-operation timeouts > > actually, ideally this would have tests I have not been able to find time to put some together today. The attempt I did make came up against a deadlock somewhere in the implicit handshake code, which makes me think there might be dragons here. Perhaps it would be best if these patches were not pushed for this release. :-(
Review of attachment 319262 [details] [review]: See the comments, I think you should split those changes in a different patch and get it in master already. ::: tls/gnutls/gtlsclientconnection-gnutls.c @@ +142,3 @@ g_object_unref (remote_addr); } + g_clear_object (&base_conn); this should go in a different patch @@ +402,3 @@ session_datum.data); + if (gnutls->priv->session_id) this should also go in a different patch
Another comment (no idea why I cannot use the review tool now) --- return g_tls_connection_gnutls_base_check (gnutls, condition); I think this refactoring could be split in a different patch as well.
Created attachment 325361 [details] [review] fixup! tests: Add unit tests for DTLS support
Created attachment 325362 [details] [review] fixup! gnutls: Internally support per-operation timeouts
Created attachment 325363 [details] [review] gnutls: Support timeouts for implicit handshakes Add support for timeouts on the implicit handshake; previously it always blocked. This fixes per-operation timeout parameters for operations which trigger an implicit handshake.
Created attachment 325364 [details] [review] tests: Add tests for per-operation and handshaking timeouts for DTLS Expand the existing DTLS tests to cover per-operation timeouts, blocking and non-blocking behaviour, in threaded and non-threaded scenarios.
Created attachment 325365 [details] [review] fixup! gnutls: Fix DTLS handshaking when packets are lost
(In reply to Philip Withnall from comment #97) > (In reply to Dan Winship from comment #87) > > Comment on attachment 318783 [details] [review] [review] [review] > > gnutls: Internally support per-operation timeouts > > > > actually, ideally this would have tests > > I have not been able to find time to put some together today. The attempt I > did make came up against a deadlock somewhere in the implicit handshake > code, which makes me think there might be dragons here. > > Perhaps it would be best if these patches were not pushed for this release. > :-( Right, I think I’ve got it now. I’ve added some test cases, and fixed several bugs — mostly it all hinged around implicit handshaking not supporting timeouts. So now it does. I’ve provided the 5 new patches as fixups (or new patches) on top of the existing ones, but can squash them if you think that would be easier for review. The patch which adds the new test cases is probably best reviewed by looking at the whole file after applying the patch, since it does a lot of refactoring to add support for a threaded server. I didn’t get round to addressing Ignacio’s comments from comment #98 and comment #99, but I can do those if patch review takes a while.
Ping?
Michael, Claudio, I was wondering about how best to get this in, and thinking that it probably makes sense to (rebase and) just push it, since it’s the start of the cycle. Any fallout will likely impact on TLS support, which should be noticed over the course of the cycle and can then be fixed. What do you think?
Oh wow, I thought this already existed. I agree now would be a good time to land it. Philip, if you rebase this, I'll make time to go through and review it all sometime before the end of next week. I see Dan and Ignacio have already looked over the patches, which helps a lot.
Whenever this is ready to land, please remember to create a stable branch before doing.
Created attachment 361509 [details] [review] gnutls: Add support for timeouts on GnuTLS pulls Set a gnutls_pull_timeout_func for GnuTLS to use. This is necessary for supporting timeouts and for DTLS.
Created attachment 361510 [details] [review] gnutls: Convert MTU-exceeded errors This is necessary for handling DTLS datagrams, as GnuTLS enforces an MTU on them.
Created attachment 361511 [details] [review] gnutls: Add support for DTLS to the GnuTLS backend Implement DTLS support in the GnuTLS backend by adding it to GTlsConnectionGnutls, so that it supports both TLS and DTLS (despite its name). This means the GTlsBackendGnutls returns the same type for client and server connection types — the protocol to use is determined by whether the GTlsConnection:base-io-stream or GDtlsConnection:base-socket property is set on the GTlsConnectionGnutls. Exactly one of the two must be set. This makes the following internal API changes: • Implement GDtlsClientConnection on GTlsClientConnectionGnutls • Implement GDtlsServerConnection on GTlsServerConnectionGnutls • Implement GDatagramBased on GTlsConnectionGnutls • Implement GDtlsConnection on GTlsConnectionGnutls The patch doesn’t support vectored I/O; support will follow in a later commit. It includes contributions by Olivier Crête <olivier.crete@collabora.com>.
Created attachment 361512 [details] [review] gnutls: Implement a vectored push callback function for GnuTLS If operating in DTLS mode, this allows GnuTLS to do vectored writes to the base GDatagramBased, which should improve performance a little. This is not *required* for DTLS support, as the normal non-vectored push function also works. It is not implemented for normal TLS as GOutputStream does not support vectored I/O.
Created attachment 361513 [details] [review] tests: Add unit tests for DTLS support
Created attachment 361514 [details] [review] gnutls: Implement vectored I/O support for TLS and DTLS This bumps the GnuTLS dependency to 3.3.5 for gnutls_record_recv_packet(), which gives us access to the internal plaintext GnuTLS buffer, from which we can copy out to multiple GInputVectors. Similarly, add support for vectored sends, using gnutls_record_cork() (requires GnuTLS 3.1.9) to queue up multiple vectors from a single message before sending the message. Include some trivial modifications of the unit tests to use multiple vectors in a message.
Created attachment 361515 [details] [review] gnutls: Internally support per-operation timeouts For every operation except handshaking (which is complex), convert the per-operation blocking parameter to a timeout parameter, supporting blocking, non-blocking and timeout behaviour. This can be used by the GDatagramBased interface to support its per-operation timeouts.
Created attachment 361516 [details] [review] gnutls: Fix DTLS handshaking when packets are lost During a handshake, the timeout is set to "-1", so blocking forever, this is wrong for DTLS, where the blocking should instead happen inside the pull_timeout function. Also, return EAGAIN during the handshake so that GnuTLS will do the re-tries for us internally and the re-sending as required.
Created attachment 361517 [details] [review] gnutls: Support timeouts for implicit handshakes Add support for timeouts on the implicit handshake; previously it always blocked. This fixes per-operation timeout parameters for operations which trigger an implicit handshake.
Created attachment 361518 [details] [review] tests: Add tests for per-operation and handshaking timeouts for DTLS Expand the existing DTLS tests to cover per-operation timeouts, blocking and non-blocking behaviour, in threaded and non-threaded scenarios.
OK, I’ve rebased the branch on master and have squashed in the fixups and updated the patches here. They’re ready to review. Apparently Olivier has a couple of fixes buried away for some things here, so he should rework them as fixups and comment here with the details. Here’s the git branch: https://git.gnome.org/browse/glib-networking/log/?h=wip/pwithnall/dtls
(In reply to Philip Withnall from comment #120) > Apparently Olivier has a couple of fixes buried away for some things here, > so he should rework them as fixups and comment here with the details. Olivier, do you have time for this soon?
Review of attachment 361509 [details] [review]: ::: tls/gnutls/gtlsconnection-gnutls.c @@ +1150,3 @@ +static gboolean +read_cb (GPollableInputStream *istream, gpointer user_data) The second parameter should be on a new line.
Review of attachment 361510 [details] [review]: ::: tls/gnutls/gtlsconnection-gnutls.c @@ +27,3 @@ #include <stdarg.h> #include <gnutls/gnutls.h> +#include <gnutls/dtls.h> The headers should be alphabetized.
Review of attachment 361511 [details] [review]: I'd be terrified to merge this if Dan had not reviewed it so thoroughly already. Still only halfway through reviewing this patch, but I'll post my first round of comments now. It's a lot because the patch is complex, but everything I've found is very minor. Now, I'm sure you won't argue that the general design here is elegant... having two different modes of operation for GTlsConnection, with different properties ignored in each mode, is not great. All the comments required to make sense of the differences between TLS and DTLS mode are evidence that it's complicated. That's not a blocker, though, because nothing here is API, so we retain total flexibility to experiment and make changes in the future. ::: tls/gnutls/gtlsclientconnection-gnutls.c @@ +142,3 @@ g_object_unref (remote_addr); } + g_clear_object (&base_conn); Looks like this is a spurious change; it's a local variable and this is the bottom of the function, so no need for g_clear_object here. There will also be a merge conflict, because sadly the patch I just pushed from Martin removed the constructed function entirely. It should be easy enough to undo this change when rebasing again. ::: tls/gnutls/gtlsconnection-gnutls.c @@ +159,3 @@ + /* When operating in stream mode; when operating in datagram mode, the + * GTlsConnectionGnutls itself is the DTLS GDatagramBased: This comment is confusing, because you have a GDatagramBased declared right below. It's worth an expanded comment to explain that the priv GDatagramBased is used to implement the GDatagramBased on GTlsConnectionGnutls itself. @@ +167,3 @@ + * Mutually exclusive with base_io_stream. + */ + GDatagramBased *base_socket; /* owned */ I'd remove the /* owned */ comment, since the reader might be confused into thinking the other priv members are unowned due to lacking the comment. It's easy to tell what's owned or not by checking finalize anyway. @@ +353,3 @@ int status; + g_return_val_if_fail (gnutls->priv->base_socket != NULL || This check is not strict enough. It should be: g_return_val_if_fail ((gnutls->priv->base_socket != NULL && gnutls->priv->base_istream == NULL && gnutls->priv->base_ostream == NULL) || (gnutls->priv->base_socket == NULL && gnutls->priv->base_istream != NULL && gnutls->priv->base_ostream != NULL)); And yeah, that's a mess, but I think it's worth being a bit stricter here. @@ +358,3 @@ + + /* Check whether to use DTLS or TLS. */ + if (gnutls->priv->base_socket != NULL) In the interests of making the code a bit more self-documenting, you might want to split this out into a new function g_tls_connection_gnutls_is_dtls_connection(). There are several places throughout this file where it would be slightly easier to read the code using such a function rather than directly checking if base_socket != NULL. @@ +968,3 @@ + * @condition. */ +static gboolean +g_tls_connection_gnutls_base_check (GTlsConnectionGnutls *gnutls, Can you think of a better name for this function? It's not great, but it's hard to think of something better. @@ +976,3 @@ + else if (condition & G_IO_IN) + return g_pollable_input_stream_is_readable (gnutls->priv->base_istream); + else This is a preexisting problem, but this breaks if the function is called with a GIOCondition that is neither G_IO_IN nor G_IO_OUT (which currently never happens). Might as well just fix it. Either g_assert (condition & G_IO_OUT) in the else clause, or change it to else if (condition & G_IO_OUT) and add a g_assert_not_reached() down below (my preferred style). @@ +1092,3 @@ gpointer user_data) { + GDatagramBasedSourceFunc datagram_based_func = (GDatagramBasedSourceFunc) callback; You've switched between two different code styles on two subsequent lines. It looks like the existing code is slightly inconsistent, but it's more common not to leave the space between the cast and the variable name, so please delete that. @@ +1164,3 @@ + result = g_value_get_boolean (&result_value); + g_value_unset (&result_value); + g_value_unset (¶m[0]); Why don't you need to call g_value_unset (¶m[1])? Because it's just an enum and doesn't really need to be freed? Then why do you need to call g_value_unset (&result_value) if that's just a gboolean? (Note: the TLS closure callback up above does that too.) @@ +1171,1 @@ static GSourceFuncs gnutls_source_funcs = Rename it to gnutls_source_tls_funcs. Or if you adopt my next suggestion: gnutls_tls_source_funcs @@ +1179,3 @@ }; +static GSourceFuncs gnutls_source_dtls_funcs = Nit: I would name it gnutls_dtls_source_funcs. @@ +1185,3 @@ + gnutls_source_dispatch, + gnutls_source_finalize, + (GSourceFunc)g_tls_connection_gnutls_source_dtls_closure_callback, So I'm quite confused by this. Only the first four fields of GSourceFuncs are documented. The final two fields are annotated: /*< private >*/ /* For use by g_source_set_closure */ So I would say it's unacceptable to use them. On the other hand, this class already used them before your patch. And the documentation of g_source_set_closure() actually instructs us to use them. IMO that is clearly a GLib documentation bug. Can you file a bug report? @@ +1216,3 @@ + gnutls_source->base = G_OBJECT (gnutls->priv->tls_istream); + else if (gnutls->priv->tls_ostream != NULL && condition & G_IO_OUT) + gnutls_source->base = G_OBJECT (gnutls->priv->tls_ostream); else g_assert_not_reached ();
I'd love to use this in my application, but I obviously I can't expect users to upgrade to versions of the library that haven't been released yet (and indeed not even once it's released). Can the GTlsBackends co-exist? If I were to import the latest code with DTLS support and then reimplement a version of g_dtls_client_connection_new() as follows: backend = my_own_backend; // g_tls_backend_get_default (); conn = g_initable_new (g_tls_backend_get_dtls_client_connection_type (backend), NULL, error, "base-socket", base_socket, "server-identity", server_identity, NULL); return G_DATAGRAM_BASED (conn); ... should I expect that to work?
Created attachment 361938 [details] [review] dtlsconnection: Fix handshaking when packets are lost During a handshake, the timeout is set to "-1", so blocking forever, this is wrong for DTLS, where the blocking should instead happen inside the pull_timeout function. Also, return EAGAIN during the handshake so that GnuTLS will do the re-tries for us internally and the re-sending as required.
Created attachment 361939 [details] [review] gnutls-connection: Use non-GClosure dummy callback GClosures are a bit costly, so avoid them.
Created attachment 361940 [details] [review] gnutls: Only call gnutls_strerror if needed Remove the useless vaargs and instead just call gnutls_sterror whenever it is needed.
Created attachment 361941 [details] [review] gtlsconnection-gnutls: Set reasonable MTU as the default
Created attachment 361942 [details] [review] connection-gnutls: Only enforce MTU for DTLS Ignore it for regular TLS
I'm attaching a couple more patches, 2 of them ("79e1d46 gnutls: Only call gnutls_strerror if needed" and "270d51e gnutls-connection: Use non-GClosure dummy callback" are not strictly related to this, as they as really performance improvements. But as DTLS is often used to relatively high bitrate, they're in practice quite important to make this really useful. Also, I'd like to suggest removing the translation for the G_IO_ERROR_WOULD_BLOCK message, as it shows up quite high in the profiling, and this is never an error that the user should see, it's entirely internal to the programmer. The two MTU patches could easily be merged, and the handshaking patch should be merged into the one that introduces this code. Michael, my initial design was to allow GIOStreams to support datagrams, which would have made this much more elegant, but it was shot down in GLib, hence we're stuck with these two base interfaces that co-exist even though they're really 90% the same code.
(In reply to David Woodhouse from comment #125) > ... should I expect that to work? I think so. At least, nothing should prevent you from defining your own GDtlsConnection implementations in application code, so I don't see why not. Of course you should expect unexpected problems. :) (In reply to Olivier Crête from comment #131) > Also, I'd like to suggest removing the translation for the > G_IO_ERROR_WOULD_BLOCK message, as it shows up quite high in the profiling, > and this is never an error that the user should see, it's entirely internal > to the programmer. Hmmm OK. > The two MTU patches could easily be merged, and the handshaking patch should > be merged into the one that introduces this code. OK. > Michael, my initial design was to allow GIOStreams to support datagrams, > which would have made this much more elegant, but it was shot down in GLib, > hence we're stuck with these two base interfaces that co-exist even though > they're really 90% the same code. I think the conceptual problem with that is that datagrams and streams are very different concepts, so this approach seems better to me.
Review of attachment 361509 [details] [review]: ::: tls/gnutls/gtlsconnection-gnutls.c @@ +61,3 @@ +static int g_tls_connection_gnutls_pull_timeout_func (gnutls_transport_ptr_t transport_data, + unsigned int ms); (Backtracking to the first patch: here, ms should be aligned with transport_data on the line above.) @@ +1161,3 @@ +static int +g_tls_connection_gnutls_pull_timeout_func (gnutls_transport_ptr_t transport_data, + unsigned int ms) Here too.
Review of attachment 361511 [details] [review]: ::: tls/gnutls/gtlsconnection-gnutls.c @@ +1283,3 @@ + !g_cancellable_is_cancelled (cancellable)) + { + result = g_poll (fds, n_fds, timeout); I think there is a bug here. The timeout is not actually respected, right? It's great that the timeout is passed along to g_poll(), but you need to break out of the loop if g_poll() returns 0: result = g_poll (fds, n_fds, timeout); if (result == 0) break; if (result != -1 || errno != EINTR) continue; // ... Otherwise, it will continue to loop forever until the I/O condition is satisfied, making repeated nonblocking calls to g_poll() even after the timeout has passed and churning CPU. If I'm wrong, please explain why. @@ +1422,3 @@ static gboolean +read_pollable_cb (GPollableInputStream *istream, + gpointer user_data) The spacing is wrong here: user_data should be aligned with istream on the line above. @@ -1186,3 @@ g_cancellable_is_cancelled (gnutls->priv->read_cancellable)) return 1; - else if (ms == 0) I think you should not remove this condition, because... @@ +1456,1 @@ + /* If @ms is 0, GnuTLS wants an instant response, so there’s no need to ...now instead of returning early when it's clear the stream is not readable, you skip over this big block of code and then run exactly the same test to see whether the stream is readable again, which is pointless. Perhaps this must have been needed in some previous version of the patch? Anyway, you should remove this check and bring back the early return instead. @@ +2180,3 @@ */ + /* FIXME: This does not properly support the @timeout parameter. */ I assume this is fixed in one of the later patches? @@ +2225,3 @@ + else if (gnutls->priv->base_socket != NULL) + { + /* We do not close underlying #GDatagramBaseds. */ I know you explained this earlier in the comments on this bug, but it deserves an explanation in this code comment as well, because it seems wrong.
Review of attachment 361512 [details] [review]: ::: tls/gnutls/gtlsconnection-gnutls.c @@ +1446,3 @@ + g_clear_error (&gnutls->priv->write_error); + + /* this entire expression will be evaluated at compile time */ One can hope... so what's the story behind this, is it expected to be compatible on Linux? @@ +1449,3 @@ + if (sizeof *iov == sizeof *vectors && + sizeof iov->iov_base == sizeof vectors->buffer && + G_STRUCT_OFFSET (giovec_t, iov_base) == It'd be easier to read without the line break after the == @@ +1452,3 @@ + G_STRUCT_OFFSET (GOutputVector, buffer) && + sizeof iov->iov_len == sizeof vectors->size && + G_STRUCT_OFFSET (giovec_t, iov_len) == Here too. @@ +1456,3 @@ + /* ABI is compatible */ + { + message.vectors = (GOutputVector *) iov; No space after the cast (I guess). @@ +1464,3 @@ + gint i; + + message.vectors = g_newa (GOutputVector, iovcnt); Why are you confident that this won't blow the stack? g_new() would be safer; is the overhead from the extra malloc really unacceptable here? @@ +1467,3 @@ + for (i = 0; i < iovcnt; i++) + { + message.vectors[i].buffer = (void *) iov[i].iov_base; Ditto.
(In reply to Michael Catanzaro from comment #135) > @@ +1467,3 @@ > + for (i = 0; i < iovcnt; i++) > + { > + message.vectors[i].buffer = (void *) iov[i].iov_base; > > Ditto. Sorry, I meant no space after the cast there, too.
Review of attachment 361513 [details] [review]: This isn't good enough: we could gain a tremendous amount of additional test coverage for a relatively small amount of additional work by running all of the existing tests with both GTlsClientConnection and GDtlsClientConnection. It requires some thinking about the design, but it shouldn't be too hard. The logic for running the TLS and DTLS servers should go into two separate source files. TestConnection should gain a bool to indicate whether it's in TLS or DTLS mode, start_async_server_and_connect_to_it() and start_echo_server_and_connect_to_it() will need to be changed to take a TestConnection* and check that bool to decide which server to start, and everywhere that calls g_tls_client_connection_new() will need to call some function that takes a TestConnection* and checks the bool to decide whether to call g_tls_client_connection_new() or g_dtls_client_connection_new(). Is there any reason this would be harder than I expect?
Review of attachment 361514 [details] [review]: ::: tls/gnutls/gtlsconnection-gnutls.c @@ +2257,3 @@ + "DTLS connection, maximum is %u bytes"), + total_message_size, + (guint) gnutls_dtls_get_data_mtu (gnutls->priv->session)); Extra space
Review of attachment 361515 [details] [review]: ::: tls/gnutls/gtlsconnection-gnutls.c @@ +758,3 @@ + !g_cancellable_is_cancelled (cancellable)) + { + result = g_poll (fds, nfds, timeout); Same problem here, I think the loop never terminates on timeout. But also, now this code is duplicated in two different places: it should be factored out into a new function. @@ +1578,3 @@ + /* Create a read source. We cannot use g_source_set_ready_time() on this + * to combine it with the @timeout_source, as that could mess with the + * internals of the #GDatagramBased’s #GSource implementation. */ It's a bit excessive to use @ and # in comments that are not doc comments, IMO.
(In reply to Olivier Crête from comment #131) > Also, I'd like to suggest removing the translation for the > G_IO_ERROR_WOULD_BLOCK message, as it shows up quite high in the profiling, > and this is never an error that the user should see, it's entirely internal > to the programmer. It actually gets returned in the API to the application, so it could indeed be presented to the user. So let's add a FIXME if removing the translation.
Review of attachment 361516 [details] [review]: This is too confusing without a comment explaining the difference in behavior.
(In reply to Michael Catanzaro from comment #140) > It actually gets returned in the API to the application, so it could indeed > be presented to the user. So let's add a FIXME if removing the translation. Might be better to pass the string through gettext (_) exactly once, and then cache the result locally to avoid further calls. That seems yucky, but better than leaving the error untranslated. I've never heard of gettext showing up high in a profile before, but here it's a consequence of abusing GError to indicate an unexceptional condition, and it's baked into the API so there's not much else we can do about it.
Review of attachment 361517 [details] [review]: ::: tls/gnutls/gtlsconnection-gnutls.c @@ +1741,3 @@ + /* Convert from microseconds to milliseconds, but ensure the timeout + * remains positive. */ + timeout_ms = (timeout + 999) / 1000; This can add an extra millisecond to the timeout. Why not just use MIN? @@ +1767,3 @@ g_tls_connection_gnutls_set_handshake_priority (gnutls); + /* Adjust the timeout for the next operation in the sequence. */ It's a lot of code duplication. Can't it be factored into a new function?
Review of attachment 361518 [details] [review]: Nice! ::: tls/tests/dtls-connection.c @@ +65,3 @@ + gint64 client_timeout; /* microseconds */ + gboolean server_should_disappear; /* whether the server should stop responding before sending a message */ + gboolean server_should_close; /* whether the server should close gracefully once it’s sent a message */ Really, a ’ character...?
Review of attachment 361938 [details] [review]: I'll ignore that the commit message is wrong, since you mentioned this is intended as a fixup patch. ::: tls/gnutls/gtlsconnection-gnutls.c @@ +1406,3 @@ ret = g_datagram_based_receive_messages (gnutls->priv->base_socket, &message, 1, 0, + gnutls->priv->handshaking ? 0 : This should all be on the next line down; it's too hard to read otherwise.
Review of attachment 361939 [details] [review]: This showed up in profiling? Then OK, but it needs a comment as someone is likely to just switch it to use g_source_set_dummy_callback() in the future without some warning. ::: tls/gnutls/gtlsconnection-gnutls.c @@ +1136,3 @@ + g_source_set_callback (gnutls_source->child_source, dummy_callback, NULL, + NULL); There's no reason to put this one parameter on a new line.
Review of attachment 361940 [details] [review]: I guess this also appeared in profiling? ::: tls/gnutls/gtlsconnection-gnutls.c @@ +993,3 @@ if (error) { + *error = g_error_new (G_TLS_ERROR, G_TLS_ERROR_MISC, "%s%s", I'd make it "%s: %s" so that you can remove the trailing ": " from all the other strings.
Review of attachment 361941 [details] [review]: OK I guess
Review of attachment 361941 [details] [review]: Well, maybe the comment should explain what the default MTU used by GnuTLS is, and why that's less reasonable than 1400, which seems arbitrary to me. I have no clue what's generally used, though.
Review of attachment 361942 [details] [review]: Ooop
Whew, that took a while. I really like this code. Thanks very much. If you have time to improve the tests as requested, that would be wonderful. All my other comments look easy. My only serious concern is the use of the timeout with g_poll(), which does not look right, despite all the tests.
I have a plan to write a g_error_new_for_gioerror(GIOError enum_number) that returns a statically allocated GError and then have a special case inside g_error_free() that just ignores those. But I never got around to it, in my actual production project, I just did "#undef _ \n #define _(x) (x)" in the whole file and it fixed my problem, but it was almost only the use case.
(In reply to Olivier Crête from comment #152) > I have a plan to write a g_error_new_for_gioerror(GIOError enum_number) that > returns a statically allocated GError Can't do that, since it will be freed by the caller, and GErrors are not refcounted.
(In reply to Michael Catanzaro from comment #153) > (In reply to Olivier Crête from comment #152) > > I have a plan to write a g_error_new_for_gioerror(GIOError enum_number) that > > returns a statically allocated GError > > Can't do that, since it will be freed by the caller, and GErrors are not > refcounted. I think Olivier was planning on having a special case in g_error_free() which turns it into a no-op for the special statically allocated GErrors.
Yes please! (Should maybe open a new bug for that) Always allocating a GError for every WOULDBLOCK is a bit suboptimal..
(In reply to Tim-Philipp Müller from comment #155) > Yes please! (Should maybe open a new bug for that) Always allocating a > GError for every WOULDBLOCK is a bit suboptimal.. Could potentially be done as bug #744533, or as an entirely new bug. But in any case, yes, it should be a separate bug report from this one.
see also bug 684969, which is about trying to extend GError... I feel like there was further discussion of that somewhere but I can't find it
(In reply to David Woodhouse from comment #125) > I'd love to use this in my application, but I obviously I can't expect users > to upgrade to versions of the library that haven't been released yet (and > indeed not even once it's released). > > Can the GTlsBackends co-exist? If I were to import the latest code with DTLS > support and then reimplement a version of g_dtls_client_connection_new() as > follows: You’d have to copy the code to your project, and rename it so all the GTypes were different. It would probably work then.
(In reply to Dan Winship from comment #157) > see also bug 684969, which is about trying to extend GError... I feel like > there was further discussion of that somewhere but I can't find it I couldn't resist pointing this out (probably because of my involvement there) - #124022.
(In reply to Michael Catanzaro from comment #122) > Review of attachment 361509 [details] [review] [review]: Fixed. (In reply to Michael Catanzaro from comment #123) > Review of attachment 361510 [details] [review] [review]: Fixed. (In reply to Michael Catanzaro from comment #124) > Review of attachment 361511 [details] [review] [review]: All fixed. Some specific replies below: > @@ +968,3 @@ > + * @condition. */ > +static gboolean > +g_tls_connection_gnutls_base_check (GTlsConnectionGnutls *gnutls, > > Can you think of a better name for this function? It's not great, but it's > hard to think of something better. Nope, and it’s consistent with the gnutls_check() function below. > @@ +1185,3 @@ > + gnutls_source_dispatch, > + gnutls_source_finalize, > + (GSourceFunc)g_tls_connection_gnutls_source_dtls_closure_callback, > … > > So I would say it's unacceptable to use them. On the other hand, this class > already used them before your patch. And the documentation of > g_source_set_closure() actually instructs us to use them. IMO that is > clearly a GLib documentation bug. Can you file a bug report? Bug #789815 filed. (In reply to Olivier Crête from comment #131) > I'm attaching a couple more patches, 2 of them ("79e1d46 gnutls: Only call > gnutls_strerror if needed" and "270d51e gnutls-connection: Use non-GClosure > dummy callback" are not strictly related to this, as they as really > performance improvements. But as DTLS is often used to relatively high > bitrate, they're in practice quite important to make this really useful. All pulled in or squashed as appropriate. (In reply to Michael Catanzaro from comment #133) > Review of attachment 361509 [details] [review] [review]: Fixed. (In reply to Michael Catanzaro from comment #134) > Review of attachment 361511 [details] [review] [review]: Fixed. (In reply to Michael Catanzaro from comment #135) > Review of attachment 361512 [details] [review] [review]: > > ::: tls/gnutls/gtlsconnection-gnutls.c > @@ +1446,3 @@ > + g_clear_error (&gnutls->priv->write_error); > + > + /* this entire expression will be evaluated at compile time */ > > One can hope... so what's the story behind this, is it expected to be > compatible on Linux? It originates from gio/gsocket.c. Yes, it is expected to be compatible on Linux. It’s a fast path for Linux. > @@ +1449,3 @@ > + if (sizeof *iov == sizeof *vectors && > + sizeof iov->iov_base == sizeof vectors->buffer && > + G_STRUCT_OFFSET (giovec_t, iov_base) == > > It'd be easier to read without the line break after the == 80 character limit. I’m not changing the patch just for that. > @@ +1456,3 @@ > + /* ABI is compatible */ > + { > + message.vectors = (GOutputVector *) iov; > > No space after the cast (I guess). The coding style has spaces after casts. The places where you saw inconsistencies before have been fixed to include a space. > @@ +1464,3 @@ > + gint i; > + > + message.vectors = g_newa (GOutputVector, iovcnt); > > Why are you confident that this won't blow the stack? g_new() would be > safer; is the overhead from the extra malloc really unacceptable here? Because it hasn’t blown the stack in all the time the same code has been in use in GSocket. This is a performance critical path; we don’t want mallocs here. (In reply to Michael Catanzaro from comment #137) > Review of attachment 361513 [details] [review] [review]: > > This isn't good enough: we could gain a tremendous amount of additional test > coverage for a relatively small amount of additional work by running all of > the existing tests with both GTlsClientConnection and GDtlsClientConnection. > It requires some thinking about the design, but it shouldn't be too hard. > The logic for running the TLS and DTLS servers should go into two separate > source files. TestConnection should gain a bool to indicate whether it's in > TLS or DTLS mode, start_async_server_and_connect_to_it() and > start_echo_server_and_connect_to_it() will need to be changed to take a > TestConnection* and check that bool to decide which server to start, and > everywhere that calls g_tls_client_connection_new() will need to call some > function that takes a TestConnection* and checks the bool to decide whether > to call g_tls_client_connection_new() or g_dtls_client_connection_new(). Sorry, I do not have time to add new tests or rework these ones. Take them or leave them. Sharing the existing tests between TLS and DTLS is unlikely to work as simply as you think, because of the inherent differences between stream- and datagram-based connections. I’ve tried this before. I ended up sticking in so many special cases in the code to handle the conceptual differences, that it would have been better to have separate tests from the beginning. (In reply to Michael Catanzaro from comment #138) > Review of attachment 361514 [details] [review] [review]: > > ::: tls/gnutls/gtlsconnection-gnutls.c > @@ +2257,3 @@ > + "DTLS connection, maximum is %u bytes"), > + total_message_size, > + (guint) gnutls_dtls_get_data_mtu > (gnutls->priv->session)); > > Extra space For the cast? That’s the coding style. This is correct. (In reply to Michael Catanzaro from comment #139) > Review of attachment 361515 [details] [review] [review]: > > ::: tls/gnutls/gtlsconnection-gnutls.c > @@ +758,3 @@ > + !g_cancellable_is_cancelled (cancellable)) > + { > + result = g_poll (fds, nfds, timeout); > > Same problem here, I think the loop never terminates on timeout. Fixed. > But also, now this code is duplicated in two different places: it should be > factored out into a new function. I tried that at the time, and the result was not satisfactory. > @@ +1578,3 @@ > + /* Create a read source. We cannot use g_source_set_ready_time() on > this > + * to combine it with the @timeout_source, as that could mess with the > + * internals of the #GDatagramBased’s #GSource implementation. */ > > It's a bit excessive to use @ and # in comments that are not doc comments, > IMO. I find it makes it easier to identify the variables in the comment. Not going to change it. (In reply to Michael Catanzaro from comment #141) > Review of attachment 361516 [details] [review] [review]: Fixed. (In reply to Michael Catanzaro from comment #143) > Review of attachment 361517 [details] [review] [review]: > > ::: tls/gnutls/gtlsconnection-gnutls.c > @@ +1741,3 @@ > + /* Convert from microseconds to milliseconds, but ensure the > timeout > + * remains positive. */ > + timeout_ms = (timeout + 999) / 1000; > > This can add an extra millisecond to the timeout. Why not just use MIN? Using MIN would give exactly the same result: potentially adding an extra millisecond to the timeout (to avoid 0). I don’t see any benefit to changing. > @@ +1767,3 @@ > g_tls_connection_gnutls_set_handshake_priority (gnutls); > > + /* Adjust the timeout for the next operation in the sequence. */ > > It's a lot of code duplication. Can't it be factored into a new function? This is also somewhere I tried to factor out the common code, and ended up with a result which was not satisfactory. (In reply to Michael Catanzaro from comment #144) > Review of attachment 361518 [details] [review] [review]: > > ::: tls/tests/dtls-connection.c > @@ +65,3 @@ > + gint64 client_timeout; /* microseconds */ > + gboolean server_should_disappear; /* whether the server should stop > responding before sending a message */ > + gboolean server_should_close; /* whether the server should close > gracefully once it’s sent a message */ > > Really, a ’ character...? Yes, we are not living in the 80s. (In reply to Michael Catanzaro from comment #145) > Review of attachment 361938 [details] [review] [review]: Fixed. (In reply to Michael Catanzaro from comment #146) > Review of attachment 361939 [details] [review] [review]: Fixed. (In reply to Michael Catanzaro from comment #147) > Review of attachment 361940 [details] [review] [review]: Fixed. (In reply to Michael Catanzaro from comment #149) > Review of attachment 361941 [details] [review] [review]: > > Well, maybe the comment should explain what the default MTU used by GnuTLS > is, and why that's less reasonable than 1400, which seems arbitrary to me. I > have no clue what's generally used, though. I haven’t fixed this because I don’t know why Olivier chose 1400. Olivier?
(In reply to Michael Catanzaro from comment #124) > Review of attachment 361511 [details] [review] [review]: > > ::: tls/gnutls/gtlsclientconnection-gnutls.c > @@ +142,3 @@ > g_object_unref (remote_addr); > } > + g_clear_object (&base_conn); > > Looks like this is a spurious change; it's a local variable and this is the > bottom of the function, so no need for g_clear_object here. There will also > be a merge conflict, because sadly the patch I just pushed from Martin > removed the constructed function entirely. It should be easy enough to undo > this change when rebasing again. Actually, the g_clear_object() change is needed because base_conn may be NULL now.
Created attachment 362812 [details] [review] gnutls: Add support for timeouts on GnuTLS pulls Set a gnutls_pull_timeout_func for GnuTLS to use. This is necessary for supporting timeouts and for DTLS.
Created attachment 362813 [details] [review] gnutls: Convert MTU-exceeded errors This is necessary for handling DTLS datagrams, as GnuTLS enforces an MTU on them.
Created attachment 362814 [details] [review] gnutls: Add support for DTLS to the GnuTLS backend Implement DTLS support in the GnuTLS backend by adding it to GTlsConnectionGnutls, so that it supports both TLS and DTLS (despite its name). This means the GTlsBackendGnutls returns the same type for client and server connection types — the protocol to use is determined by whether the GTlsConnection:base-io-stream or GDtlsConnection:base-socket property is set on the GTlsConnectionGnutls. Exactly one of the two must be set. This makes the following internal API changes: • Implement GDtlsClientConnection on GTlsClientConnectionGnutls • Implement GDtlsServerConnection on GTlsServerConnectionGnutls • Implement GDatagramBased on GTlsConnectionGnutls • Implement GDtlsConnection on GTlsConnectionGnutls The patch doesn’t support vectored I/O; support will follow in a later commit. It includes contributions by Olivier Crête <olivier.crete@collabora.com>.
Created attachment 362815 [details] [review] gnutls: Implement a vectored push callback function for GnuTLS If operating in DTLS mode, this allows GnuTLS to do vectored writes to the base GDatagramBased, which should improve performance a little. This is not *required* for DTLS support, as the normal non-vectored push function also works. It is not implemented for normal TLS as GOutputStream does not support vectored I/O.
Created attachment 362816 [details] [review] tests: Add unit tests for DTLS support
Created attachment 362817 [details] [review] gnutls: Implement vectored I/O support for TLS and DTLS This bumps the GnuTLS dependency to 3.3.5 for gnutls_record_recv_packet(), which gives us access to the internal plaintext GnuTLS buffer, from which we can copy out to multiple GInputVectors. Similarly, add support for vectored sends, using gnutls_record_cork() (requires GnuTLS 3.1.9) to queue up multiple vectors from a single message before sending the message. Include some trivial modifications of the unit tests to use multiple vectors in a message.
Created attachment 362818 [details] [review] gnutls: Internally support per-operation timeouts For every operation except handshaking (which is complex), convert the per-operation blocking parameter to a timeout parameter, supporting blocking, non-blocking and timeout behaviour. This can be used by the GDatagramBased interface to support its per-operation timeouts.
Created attachment 362819 [details] [review] gnutls: Fix DTLS handshaking when packets are lost During a handshake, the timeout is set to "-1", so blocking forever, this is wrong for DTLS, where the blocking should instead happen inside the pull_timeout function. Also, return EAGAIN during the handshake so that GnuTLS will do the re-tries for us internally and the re-sending as required.
Created attachment 362820 [details] [review] gnutls: Support timeouts for implicit handshakes Add support for timeouts on the implicit handshake; previously it always blocked. This fixes per-operation timeout parameters for operations which trigger an implicit handshake.
Created attachment 362821 [details] [review] tests: Add tests for per-operation and handshaking timeouts for DTLS Expand the existing DTLS tests to cover per-operation timeouts, blocking and non-blocking behaviour, in threaded and non-threaded scenarios.
Created attachment 362822 [details] [review] gnutls: Use non-GClosure dummy callback GClosures are a bit costly, so avoid them.
Created attachment 362823 [details] [review] gnutls: Only call gnutls_strerror if needed Remove the useless vaargs and instead just call gnutls_sterror whenever it is needed.
Created attachment 362824 [details] [review] gnutls: Set reasonable MTU as the default for DTLS only Ignore it for regular TLS
OK, I'm not going to change the individual patch statuses, but I'm happy with your explanations for everything you didn't change (including the reasoning for not running the existing GTlsConnection tests) so everything is accepted-commit-now. Thanks for getting this to the finish lin, Philip! I do have one question, though. How on earth are you typing those ’ characters? You can't seriously be copy/pasting that each time you want an apostrophe? ;)
(In reply to Michael Catanzaro from comment #175) > Thanks for getting this to the finish lin, Philip! finish line
(In reply to Michael Catanzaro from comment #175) > I do have one question, though. How on earth are you typing those ’ > characters? You can't seriously be copy/pasting that each time you want an > apostrophe? ;) I could construct some elaborate bs about a custom input method or emacs key combination. However, it’s actually just Level3+Shift+B for me. I have Level3 bound to AltGr (`gsettings set org.gnome.desktop.input-sources xkb-options ['compose:menu', 'lv3:ralt_switch']`).
All pushed. Thanks very much for the reviews, Michael, Dan, Olivier and Ignacio. As Claudio said in comment #109, you might want to create a stable glib-networking branch from before the merge point here. (I don’t know what your stable branch policy is at the moment.) I’m not sorry that this won’t reach 200 comments. Glad it’s finally over! Attachment 362812 [details] pushed as 0795cd1 - gnutls: Add support for timeouts on GnuTLS pulls Attachment 362813 [details] pushed as 810626e - gnutls: Convert MTU-exceeded errors Attachment 362814 [details] pushed as 9cfa9b9 - gnutls: Add support for DTLS to the GnuTLS backend Attachment 362815 [details] pushed as b2c4450 - gnutls: Implement a vectored push callback function for GnuTLS Attachment 362816 [details] pushed as 0d6ee6f - tests: Add unit tests for DTLS support Attachment 362817 [details] pushed as eb38088 - gnutls: Implement vectored I/O support for TLS and DTLS Attachment 362818 [details] pushed as 5a0b377 - gnutls: Internally support per-operation timeouts Attachment 362819 [details] pushed as 415e737 - gnutls: Fix DTLS handshaking when packets are lost Attachment 362820 [details] pushed as 2df840b - gnutls: Support timeouts for implicit handshakes Attachment 362821 [details] pushed as 83ef278 - tests: Add tests for per-operation and handshaking timeouts for DTLS Attachment 362822 [details] pushed as a3ac39b - gnutls: Use non-GClosure dummy callback Attachment 362823 [details] pushed as 373813f - gnutls: Only call gnutls_strerror if needed Attachment 362824 [details] pushed as 7023df5 - gnutls: Set reasonable MTU as the default for DTLS only
Review of attachment 361941 [details] [review]: I think the default is 65536, which meant that it will never give an error and it will end up not working correctly. Ideally, we would have the required API on GDatagramBased/GSocket to pass the underlying MTU up to the DTLS layer.
(Stable branch was already created before the switch to meson.) (In reply to Olivier Crête from comment #179) > Review of attachment 361941 [details] [review] [review]: > > I think the default is 65536, which meant that it will never give an error > and it will end up not working correctly. Ideally, we would have the > required API on GDatagramBased/GSocket to pass the underlying MTU up to the > DTLS layer. Could you report a bug for this, please?