Created attachment 307244 [details] [review]
gio: Add DTLS interfaces

Add a new GDtlsConnection interface, plus derived GDtlsClientConnection
and GDtlsServerConnection interfaces, for implementing Datagram TLS
support in glib-networking.

A GDtlsConnection is a GCommunicable, so may be used as a normal
datagram socket, wrapping all datagrams from a base GCommunicable in
DTLS segments.

Test cases are included in the implementation in glib-networking.

Created attachment 307245 [details] [review]
gio: Add G_IO_ERROR_MESSAGE_TOO_LARGE

Corresponding to EMSGSIZE, for when UDP datagrams are rejected due to
being too big.

Comment on attachment 307245 [details] [review]
gio: Add G_IO_ERROR_MESSAGE_TOO_LARGE

>+ * @G_IO_ERROR_MESSAGE_TOO_LARGE: Message too large. Since 2.44.

2.46 now. Otherwise fine

Created attachment 307980 [details] [review]
gio: Add G_IO_ERROR_MESSAGE_TOO_LARGE

Corresponding to EMSGSIZE, for when UDP datagrams are rejected due to
being too big.

Comment on attachment 307980 [details] [review]
gio: Add G_IO_ERROR_MESSAGE_TOO_LARGE

Marking as reviewed as per comment #3.

Created attachment 307998 [details] [review]
gio: Add DTLS interfaces

Add a new GDtlsConnection interface, plus derived GDtlsClientConnection
and GDtlsServerConnection interfaces, for implementing Datagram TLS
support in glib-networking.

A GDtlsConnection is a GDatagramBased, so may be used as a normal
datagram socket, wrapping all datagrams from a base GDatagramBased in
DTLS segments.

Test cases are included in the implementation in glib-networking.

---

Updated to use GDatagramBased rather than GCommunicable.

Created attachment 308540 [details] [review]
gio: Add DTLS interfaces

Add a new GDtlsConnection interface, plus derived GDtlsClientConnection
and GDtlsServerConnection interfaces, for implementing Datagram TLS
support in glib-networking.

A GDtlsConnection is a GDatagramBased, so may be used as a normal
datagram socket, wrapping all datagrams from a base GDatagramBased in
DTLS segments.

Test cases are included in the implementation in glib-networking.

Created attachment 308541 [details] [review]
gio: Add G_IO_ERROR_MESSAGE_TOO_LARGE

Corresponding to EMSGSIZE, for when UDP datagrams are rejected due to
being too big.

Trivial update to keep the patchset up to date with bug #751924 and bug #697907.

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.

Created attachment 312299 [details] [review]
gio: Add DTLS interfaces

Add a new GDtlsConnection interface, plus derived GDtlsClientConnection
and GDtlsServerConnection interfaces, for implementing Datagram TLS
support in glib-networking.

A GDtlsConnection is a GDatagramBased, so may be used as a normal
datagram socket, wrapping all datagrams from a base GDatagramBased in
DTLS segments.

Test cases are included in the implementation in glib-networking.

Created attachment 312300 [details] [review]
gio: Add G_IO_ERROR_MESSAGE_TOO_LARGE

Corresponding to EMSGSIZE, for when UDP datagrams are rejected due to
being too big.

Rebased, and the changes from comment #10 incorporated. Branch at https://git.collabora.com/cgit/user/pwith/glib.git/log/?h=g-socket-interface.

Comment on attachment 312300 [details] [review]
gio: Add G_IO_ERROR_MESSAGE_TOO_LARGE

Reviewed in https://bugzilla.gnome.org/show_bug.cgi?id=752240#c3; pushed to master with an updated version of 2.48.

Attachment 312300 [details] pushed as be73267 - gio: Add G_IO_ERROR_MESSAGE_TOO_LARGE

Created attachment 312486 [details] [review]
gio: Add DTLS interfaces

Add a new GDtlsConnection interface, plus derived GDtlsClientConnection
and GDtlsServerConnection interfaces, for implementing Datagram TLS
support in glib-networking.

A GDtlsConnection is a GDatagramBased, so may be used as a normal
datagram socket, wrapping all datagrams from a base GDatagramBased in
DTLS segments.

Test cases are included in the implementation in glib-networking.

---

 • Rebased on master now that we’ve branched for 2.48.
 • Changed the versions for new APIs to 2.48.

Created attachment 312824 [details] [review]
• Added g_dtls_connection_close[_async|_finish]()

• Added g_dtls_connection_shutdown[_async|_finish]()
 • Removed GDatagramBased implementations of the above due to the vfuncs being removed from GDatagramBased

---

gio: Add DTLS interfaces

Add a new GDtlsConnection interface, plus derived GDtlsClientConnection
and GDtlsServerConnection interfaces, for implementing Datagram TLS
support in glib-networking.

A GDtlsConnection is a GDatagramBased, so may be used as a normal
datagram socket, wrapping all datagrams from a base GDatagramBased in
DTLS segments.

Test cases are included in the implementation in glib-networking.

Comment on attachment 312824 [details] [review]
• Added g_dtls_connection_close[_async|_finish]()

>+   * GDtlsClientConnection:enable-negotiation:

Drop this. It exists in GTlsClientConnection only because of ancient badly-written SSL 3.0 servers. DTLS servers should not have the same issues.

>+ * g_dtls_client_connection_copy_session_state:

This basically only exists to support FTP over TLS. Unless we know of something that needs this in the DTLS case, drop it.

>+ * To close a DTLS connection, use g_dtls_connection_close(). This is in
>+ * contrast to TLS connections, which are closed using g_io_stream_close().

Not sure this really needs to be said, since GDtlsConnection isn't a GIOStream, so obviously g_io_stream_close() is wrong.

>+ * Neither #GDtlsServerConnection or #GDtlsClientConnection set the peer address
>+ * on their base #GDatagramBased if it is a #GSocket â it is up to the caller to
>+ * do that if they wish. If they do not, the #GDtlsConnection will not raise a
>+ * %G_IO_ERROR_NOT_CONNECTED error when the connection is closed (for example,
>+ * by calling g_socket_close() if the #GDatagramBased is a #GSocket).

I don't understand what this paragraph is saying.

>+   * GDtlsConnection:database:
>+   *
>+   * The certificate database to use when verifying this TLS connection.
>+   * If no cerificate database is set, then the default database will be

typo "cerificate"

>+   * GDtlsConnection:rehandshake-mode:

consider defaulting this to G_TLS_REHANDSHAKE_NEVER? Unless you know of situations where rehandshaking is used with DTLS...

>+ * g_dtls_connection_set_require_close_notify:

>+ * redundant and sometimes omitted. (TLS 1.1 explicitly allows this;
>+ * in TLS 1.0 it is technically an error, but often done anyway.)

DTLS 1.0 is based on TLS 1.1, so this is always allowed in DTLS and you can remove the parenthetical.

>+ * will show up as a 0-length read, as in a non-TLS
>+ * #GSocketConnection, and it is up to the application to check that

#GDatagramBased

>+ * g_dtls_connection_handshake:

>+ * On the client side, it is never necessary to call this method;

Implicit handshaking turned out to be a huge pain. I haven't looked at the glib-networking side yet so I don't know what code gets shared, but if implicit handshaking doesn't come for free, you may want to drop it.

>+ * g_dtls_connection_shutdown:
>+ * @conn: a #GDtlsConnection
>+ * @cancellable: (nullable): a #GCancellable, or %NULL
>+ * @error: a #GError, or %NULL

missing @shutdown_read and @shutdown_write (likewise in async)

>+ * g_dtls_connection_close:

>+ * Closing a #GDtlsConnection waits for all buffered but untransmitted data to
>+ * be sent before it completes.

Is that actually true?

>    It does not close the underlying
>+ * #GDtlsConnection:base-socket; that must be closed separately.

You may want to clarify what it actually does do then (in terms of DTLS)

Created attachment 314970 [details] [review]
gio: Add DTLS interfaces

Add a new GDtlsConnection interface, plus derived GDtlsClientConnection
and GDtlsServerConnection interfaces, for implementing Datagram TLS
support in glib-networking.

A GDtlsConnection is a GDatagramBased, so may be used as a normal
datagram socket, wrapping all datagrams from a base GDatagramBased in
DTLS segments.

Test cases are included in the implementation in glib-networking.

(In reply to Dan Winship from comment #17)
> Comment on attachment 312824 [details] [review] [review]
> • Added g_dtls_connection_close[_async|_finish]()
> 
> >+   * GDtlsClientConnection:enable-negotiation:
> 
> Drop this. It exists in GTlsClientConnection only because of ancient
> badly-written SSL 3.0 servers. DTLS servers should not have the same issues.

Dropped.

> >+ * g_dtls_client_connection_copy_session_state:
> 
> This basically only exists to support FTP over TLS. Unless we know of
> something that needs this in the DTLS case, drop it.

Dropped.

> >+ * To close a DTLS connection, use g_dtls_connection_close(). This is in
> >+ * contrast to TLS connections, which are closed using g_io_stream_close().
> 
> Not sure this really needs to be said, since GDtlsConnection isn't a
> GIOStream, so obviously g_io_stream_close() is wrong.

Removed.

> >+ * Neither #GDtlsServerConnection or #GDtlsClientConnection set the peer address
> >+ * on their base #GDatagramBased if it is a #GSocket â it is up to the caller to
> >+ * do that if they wish. If they do not, the #GDtlsConnection will not raise a
> >+ * %G_IO_ERROR_NOT_CONNECTED error when the connection is closed (for example,
> >+ * by calling g_socket_close() if the #GDatagramBased is a #GSocket).
> 
> I don't understand what this paragraph is saying.

I've tweaked the wording — see if it makes more sense now?

> >+   * GDtlsConnection:database:
> >+   *
> >+   * The certificate database to use when verifying this TLS connection.
> >+   * If no cerificate database is set, then the default database will be
> 
> typo "cerificate"

Fixed (and in GTlsConnection, which it was copypasta-ed from).

> >+   * GDtlsConnection:rehandshake-mode:
> 
> consider defaulting this to G_TLS_REHANDSHAKE_NEVER? Unless you know of
> situations where rehandshaking is used with DTLS...

Rehandshaking may be used with DTLS, but is never needed because the sequence number space is just too big to ever run out. Some people want to force rehandshaking to (for example) verify that the client still has access to a certificate on a smart card. That's a sufficiently odd use case to not merit the default.

Changed to G_TLS_REHANDSHAKE_NEVER.

> >+ * g_dtls_connection_set_require_close_notify:
> 
> >+ * redundant and sometimes omitted. (TLS 1.1 explicitly allows this;
> >+ * in TLS 1.0 it is technically an error, but often done anyway.)
> 
> DTLS 1.0 is based on TLS 1.1, so this is always allowed in DTLS and you can
> remove the parenthetical.

Removed.

> >+ * will show up as a 0-length read, as in a non-TLS
> >+ * #GSocketConnection, and it is up to the application to check that
> 
> #GDatagramBased

Fixed.

> >+ * g_dtls_connection_handshake:
> 
> >+ * On the client side, it is never necessary to call this method;
> 
> Implicit handshaking turned out to be a huge pain. I haven't looked at the
> glib-networking side yet so I don't know what code gets shared, but if
> implicit handshaking doesn't come for free, you may want to drop it.

It basically came for free.

> >+ * g_dtls_connection_shutdown:
> >+ * @conn: a #GDtlsConnection
> >+ * @cancellable: (nullable): a #GCancellable, or %NULL
> >+ * @error: a #GError, or %NULL
> 
> missing @shutdown_read and @shutdown_write (likewise in async)

Whoops, fixed in both.

> >+ * g_dtls_connection_close:
> 
> >+ * Closing a #GDtlsConnection waits for all buffered but untransmitted data to
> >+ * be sent before it completes.
> 
> Is that actually true?

Yes; it uses the same internal blocking shutdown code as GTlsConnection in GTlsConnectionGnutls.

> >    It does not close the underlying
> >+ * #GDtlsConnection:base-socket; that must be closed separately.
> 
> You may want to clarify what it actually does do then (in terms of DTLS)

Clarified: it sends a close_notify alert, then potentially waits for the peer to send a close_notify alert, then is closed. i.e. It's exactly the same as TLS.

I say 'potentially' waits for a peer because it depends on the backend used in glib-networking. I haven't checked what GnuTLS actually does, but it has the option of waiting or not, according to the spec.

Comment on attachment 314970 [details] [review]
gio: Add DTLS interfaces

OK to commit assuming that the glib-networking side is also OK... (reviewing that next)

(In reply to Dan Winship from comment #20)
> OK to commit assuming that the glib-networking side is also OK... (reviewing
> that next)

(i.e. wait until the reviews for bug #697908 are done)

Committed in time for the release (as acked by danw on IRC).

Attachment 314970 [details] pushed as c3d6934 - gio: Add DTLS interfaces

This is just a tease until bug 697908 is done, right? And worse than that, it just crashes when I try to use it. 

g_tls_backend_get_dtls_client_connection_type() will follow the NULL function pointer without checking it.

I could try to check it in my application but g_tls_backend_supports_dtls() is lying to me: bug 787485.

(In reply to David Woodhouse from comment #23)
> This is just a tease until bug 697908 is done, right? And worse than that,
> it just crashes when I try to use it. 

The idea with having this as two separate bugs was to keep the patch sets manageable, not to end up with this one getting committed and bug #697908 remaining unreviewed for 1.5 years. :-(

> g_tls_backend_get_dtls_client_connection_type() will follow the NULL
> function pointer without checking it.

Patch upcoming here.

> I could try to check it in my application but g_tls_backend_supports_dtls()
> is lying to me: bug 787485.

Reply upcoming there.

Created attachment 359513 [details] [review]
gtlsbackend: Add missing preconditions for DTLS virtual methods

Make it a bit more obvious when the DTLS methods aren’t implemented by a
particular TLS backend; return an invalid type rather than crashing.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Review of attachment 359513 [details] [review]:

It looks good, but maybe you want to reduce the amount of GET_INTERFACE() calls.

::: gio/gtlsbackend.c
@@ +240,3 @@
+  g_return_val_if_fail (G_IS_TLS_BACKEND (backend), G_TYPE_INVALID);
+
+  if (G_TLS_BACKEND_GET_INTERFACE (backend)->get_dtls_client_connection_type == NULL)

That's a lot of type checks, here…

@@ +262,3 @@
+  g_return_val_if_fail (G_IS_TLS_BACKEND (backend), G_TYPE_INVALID);
+
+  if (G_TLS_BACKEND_GET_INTERFACE (backend)->get_dtls_server_connection_type == NULL)

Same here…

Review of attachment 359513 [details] [review]:

Thanks for the review. I’ll make those two minor changes and push.

::: gio/gtlsbackend.c
@@ +240,3 @@
+  g_return_val_if_fail (G_IS_TLS_BACKEND (backend), G_TYPE_INVALID);
+
+  if (G_TLS_BACKEND_GET_INTERFACE (backend)->get_dtls_client_connection_type == NULL)

It’s not a critical path (it’s called once per new DTLS connection), but I’ll factor out the two GET_INTERFACE() calls.

Ta for the review.

Attachment 359513 [details] pushed as 52dd984 - gtlsbackend: Add missing preconditions for DTLS virtual methods