QUIC Server Minimal Demo #24037

hlandau · 2024-04-04T11:38:43Z

This PR, which incorporates all the changes in #23995, makes further refinements and refactoring sufficient to support a minimal server API usage demo in blocking mode.

A demo is included and works with s_client.

Fixes openssl/project#523

nhorman · 2024-04-04T13:36:18Z

ssl/quic/quic_engine.c

+     * the engine level in future when we can have multiple ports. This is not
+     * important currently as the port list has a single entry.
+     */
+    LIST_FOREACH(port, port, &qeng->port_list)


LIST_FOREACH is generally available I think, but are we sure its available on our older platforms?

This is defined in internal/list.h and is universally available...

Oh, wow, thats interesting. I called attention to this because LIST_FOREACH is defiend in sys/queue.h as part of glibc. I'm kind of suprised we haven't run afoul of namespace conflicts here.

I would have not used those names here - but this isn't adding the names in - as Hugo points out it was done previously - so if that is an issue a separate PR could address that - as internals we can change.
Looking at the current implementation it is a mix of list/LIST/ossl_list/OSSL_LIST which seems more than a little strange in how it was done. I'd have gone for a clear prefix there across the board,

include/internal/quic_stream_map.h

hlandau · 2024-04-09T07:59:08Z

Updated, ready for review.

hlandau · 2024-04-09T08:44:14Z

Doc fix

t-j-h · 2024-04-09T08:54:43Z

demos/quic/server/server.c

+
+    /* Load certificate and corresponding private key. */
+    if (SSL_CTX_use_certificate_chain_file(ctx, cert_path) <= 0) {
+        fprintf(stderr, "couldn't load certificate file: %s\n", cert_path);


This should be SSL_CTX_use_certificate_file ... with SSL_FILETYPE_PEM I would assume.
Using the chain_file function would be rather strange to see.

Although I see this has made it into a pile of the demos - it is really not the right approach IMHO.

SSL_CTX_use_certificate_file() loads the first certificate stored in file into ctx. The
formatting type of the certificate must be specified from the known types SSL_FILETYPE_PEM,
SSL_FILETYPE_ASN1. SSL_use_certificate_file() loads the certificate from file into ssl. See
the NOTES section on why SSL_CTX_use_certificate_chain_file() should be preferred.

SSL_CTX_use_certificate_chain_file() should be used instead of the
SSL_CTX_use_certificate_file() function in order to allow the use of complete certificate
chains even when no trusted CA storage is used or when the CA issuing the certificate shall not
be added to the trusted CA storage.

Using SSL_CTX_use_certificate_file would offer no way to serve intermediate certificates... basically guaranteeing incorrect behaviour when using any certificate issued by a public CA.

t-j-h · 2024-04-09T08:55:19Z

demos/quic/server/server.c

+    int fd = -1;
+    struct sockaddr_in6 sa = {0};
+
+    if ((fd = socket(AF_INET6, SOCK_DGRAM, IPPROTO_UDP)) < 0) {


An IPv6 only demo?

Most OSes enable v4-mapped addresses by default so this is dual-stack. There are some exceptions like OpenBSD — but handling those correctly would require multiple sockets here, significantly increasing the complexity of the demo and reducing its expository value.

I'm not massively bothered either way though.

Windows doesn't work that way ... you need two listeners to get both IPv4 and IPv6 support and as that is one of the primary platforms making this choice here wasn't what I was expecting.

It does support it from Vista onwards, though you do have to enable it explicitly.

Anyway, it doesn't really matter either way. AF_INET works for me.

ssl/quic/quic_impl.c

hlandau · 2024-04-09T09:07:01Z

Updated

demos/quic/server/server.c

hlandau · 2024-04-09T14:37:54Z

Updated

ssl/quic/quic_impl.c

ssl/quic/quic_obj.c

hlandau · 2024-04-10T07:21:28Z

Updated

Sashan

Thanks, changes look good to me now. The introduction of expect_quic_*() really improves the code readability. What I'm still struggling with is to reason about the state of the lock which is associated with SSL instance s. I don't have answer for this and I think the answer is not simple (because of abstraction). Hence I'm approving those changes.

Just question:
If I understand things right we manipulate engine lock in expect_quic_as(), right?

nhorman · 2024-04-10T13:13:56Z

my approval holds