quic-go / quic-go

A QUIC implementation in pure Go
https://quic-go.net
MIT License
9.77k stars 1.28k forks source link

"tls: internal error" when using GetConfigForClient #2089

Closed mholt closed 4 years ago

mholt commented 4 years ago

First off, just wanted to thank you again for working on this lib. I've been fiddling around with integrating it into Caddy 2 this weekend, with moderate success!

Unlike in Caddy 1 with gQUIC, I will be treating IETF HTTP/3 as a first-class feature in Caddy 2 -- not experimental (well, once it gets standardized). But the HTTP/3 support will get a lot more love and attention than the gQUIC support did in Caddy 1. We intend to use the master branch -- not gQUIC -- for Caddy 2.

Anyway, I seem to have a problem that appears to be a bit of a showstopper, unless you know a good workaround or have a fix in mind.

I've been using the client example in this repo to test the HTTP/3 support.

It seems that when I provide a TLS config to the HTTP/3 server with a static certificate and key file, it works fine:

$ GO111MODULE=on go run main.go -insecure https://localhost:2443
GET https://localhost:2443
client Starting new connection to localhost ([::]:59085 -> 127.0.0.1:2443), source connection ID (empty), destination connection ID 0x99986f151ffc92d4019ba5, version QUIC WG draft-22
Got response for https://localhost:2443: &http.Response{Status:"200 OK", StatusCode:200, Proto:"HTTP/3", ProtoMajor:3, ProtoMinor:0, Header:http.Header{}, Body:(*http3.body)(0xc0001adf40), ContentLength:0, TransferEncoding:[]string(nil), Close:false, Uncompressed:false, Trailer:http.Header(nil), Request:(*http.Request)(nil), TLS:(*tls.ConnectionState)(nil)}
Request Body:
Hello, HTTP/3. Caddy at your service.
client Closing session.
client Connection (empty) closed.

Yay!

But certificates expire and stuff.

So, Caddy dynamically handles certificates using the tls.Config.GetConfigForClient field. In other words, we don't populate the Certificates field at all. But all other things being the same, not filling the Certificates field leads to this error:

GO111MODULE=on go run main.go -insecure https://localhost:2443
GET https://localhost:2443
client Starting new connection to localhost ([::]:59084 -> 127.0.0.1:2443), source connection ID (empty), destination connection ID 0xa1cb57c374c0e25eaf34d8bd951ae6be, version QUIC WG draft-22
client Peer closed session with error: CRYPTO_ERROR: tls: internal error
client Connection (empty) closed.
panic: Get https://localhost:2443: CRYPTO_ERROR: tls: internal error

goroutine 18 [running]:
main.main.func1(0xc0000a2450, 0x153f960, 0xc0000c4ab0, 0xc0000ce591, 0xc0000b4370, 0x7ffeefbffa1b, 0x16)
    /Users/matt/Dev/src/github.com/lucas-clemente/quic-go/example/client/main.go:50 +0x435
created by main.main
    /Users/matt/Dev/src/github.com/lucas-clemente/quic-go/example/client/main.go:47 +0x43b
exit status 2

Any chance it can work with GetConfigForClient? Pretty please? :smile:

PS. I'd also be happy to make Caddy 2 a proper test bed for production deployments of HTTP/3, once we get this sorted out. Integrating it seems easy enough otherwise! (We've had some issues with reusing listeners in the past, but maybe that'll be resolved with https://godoc.org/github.com/lucas-clemente/quic-go/http3#Server.CloseGracefully.)

marten-seemann commented 4 years ago

Hello @mholt, first of all, this is great news! I'm really excited about rolling out QUIC and HTTP/3. While the final RFC is still some time away (IETF procedures, you know...), people are starting to deploy draft versions already: Facebook recently enabled draft-22 on their servers, and curl now also support QUIC using the same version. Now we'd just need a browser that has QUIC support...

The problem you're seeing there definitely sounds like a bug. Just specifying a tls.Config.GetConfigForClient and no tls.Config.Certificates should definitely work. Do you have any log from the server side (maybe captured with QUIC_GO_LOG_LEVEL=debug)? That should contain a more detailed error message than "internal error".

mholt commented 4 years ago

Thanks for the debug tip. Here's the output from the server:

$ GO111MODULE=on QUIC_GO_LOG_LEVEL=debug go run main.go run --config ../caddy2.json 
2019/08/23 20:49:18 Caddy 2 admin endpoint listening on localhost:2019
2019/08/23 20:49:18 [INFO][cache:0xc00025a0f0] Started certificate maintenance routine
2019/08/23 20:49:18 [INFO][test.example.com] Skipping automatic certificate management because a certificate with that SAN is already loaded
2019/08/23 20:49:18 [INFO] Enabling automatic HTTPS certificates for []
2019/08/23 20:49:18 [INFO] Enabling automatic HTTP->HTTPS redirects for [test.example.com]
2019/08/23 20:49:18 [DEBUG] Started HTTP/3 listener on :2443
2019/08/23 20:49:18 Caddy 2 serving initial configuration
2019/08/23 20:49:18 server Listening for udp connections on [::]:2443
2019/08/23 20:49:41 server <- Received Initial packet.
2019/08/23 20:49:41 server      Long Header{Type: Initial, DestConnectionID: 0x3207a8e42a321f197d7d9e, SrcConnectionID: (empty), Token: (empty), PacketNumber: 0x0, PacketNumberLen: 0, Length: 1179, Version: QUIC WG draft-22}
2019/08/23 20:49:41 server Changing connection ID to 0x200649ec.
2019/08/23 20:49:41 server -> Sending Retry
2019/08/23 20:49:41 server      Long Header{Type: Retry, DestConnectionID: (empty), SrcConnectionID: 0x200649ec, Token: 0x995e49e0b63d561b20219173754dda8cf78d31732cf8b5bc7dc19a3358fb9cc1210bb3f507435c8b33ac7abd91d1b01cee406952b6289883533f2ddf40bacb80e5b39ae0d146e1e5c37d4304f8eaa4e8acb79aa2817abda0e1b4d335559f1d, OrigDestConnectionID: 0x3207a8e42a321f197d7d9e, Version: QUIC WG draft-22}
2019/08/23 20:49:41 server <- Received Initial packet.
2019/08/23 20:49:41 server Changing connection ID to 0x2823742f.
2019/08/23 20:49:41 server <- Reading packet 0x1 (1200 bytes) for connection 0x200649ec, Initial
2019/08/23 20:49:41 server      Long Header{Type: Initial, DestConnectionID: 0x200649ec, SrcConnectionID: (empty), Token: 0x995e49e0b63d561b20219173754dda8cf78d31732cf8b5bc7dc19a3358fb9cc1210bb3f507435c8b33ac7abd91d1b01cee406952b6289883533f2ddf40bacb80e5b39ae0d146e1e5c37d4304f8eaa4e8acb79aa2817abda0e1b4d335559f1d, PacketNumber: 0x1, PacketNumberLen: 4, Length: 1090, Version: QUIC WG draft-22}
2019/08/23 20:49:41 server      <- &wire.CryptoFrame{Offset: 0x0, Data length: 0x150, Offset + Data length: 0x150}
2019/08/23 20:49:41 server Received ClientHello message (336 bytes, encryption level: Initial)
2019/08/23 20:49:41 server Received Transport Parameters: &handshake.TransportParameters{OriginalConnectionID: (empty), InitialMaxStreamDataBidiLocal: 0x80000, InitialMaxStreamDataBidiRemote: 0x80000, InitialMaxStreamDataUni: 0x80000, InitialMaxData: 0xc0000, MaxBidiStreamNum: 0, MaxUniStreamNum: 100, IdleTimeout: 30s, AckDelayExponent: 3, MaxAckDelay: 26ms}
2019/08/23 20:49:41 server Handled crypto frame at level Initial. encLevelChanged: false
2019/08/23 20:49:41 server      Queueing ACK because the first packet should be acknowledged.
2019/08/23 20:49:41 server Closing session with error: CRYPTO_ERROR: no certificate available for 'localhost'
2019/08/23 20:49:41 server -> Sending packet 0x0 (38 bytes) for connection 0x2823742f, Initial
2019/08/23 20:49:41 server      Long Header{Type: Initial, DestConnectionID: (empty), SrcConnectionID: 0x2823742f, Token: (empty), PacketNumber: 0x0, PacketNumberLen: 4, Length: 25, Version: QUIC WG draft-22}
2019/08/23 20:49:41 server      -> &wire.AckFrame{LargestAcked: 0x1, LowestAcked: 0x1, DelayTime: 0s}
2019/08/23 20:49:41 server -> Sending packet 0x1 (38 bytes) for connection 0x2823742f, Initial
2019/08/23 20:49:41 server      Long Header{Type: Initial, DestConnectionID: (empty), SrcConnectionID: 0x2823742f, Token: (empty), PacketNumber: 0x1, PacketNumberLen: 4, Length: 25, Version: QUIC WG draft-22}
2019/08/23 20:49:41 server      -> &wire.ConnectionCloseFrame{IsApplicationError:false, ErrorCode:0x150, ReasonPhrase:""}
2019/08/23 20:49:41 server Connection 0x2823742f closed.
2019/08/23 20:49:41 server Received 1 packets after sending CONNECTION_CLOSE. Retransmitting.

Hope this helps!

(And yes, haha, Caddy 2 is well ahead of web browsers in several regards -- HTTP/3 will be one of them; Zstd compression is another... it's okay though. Server stuff usually takes longer to deploy so I'm good with starting early.)

EDIT: After reading the error: "No certificate available for 'localhost'" -- this is technically true, this might be the error string that my function returned during the TLS handshake? 😅 Lemme look into that

marten-seemann commented 4 years ago

This is the relevant line

Closing session with error: CRYPTO_ERROR: no certificate available for 'localhost'

Does the tls.Config you're using work over TCP? I'm using a fork of crypto/tls, so I'd expect this to behave the same way. Unless of course there's a bug in qtls...

mholt commented 4 years ago

Yeah, I think you're onto something.

While I'm at it, does my dynamic TLS config that is returned during the handshake need to set something special in the ALPN?

marten-seemann commented 4 years ago

EDIT: After reading the error: "No certificate available for 'localhost'" -- this is technically true, this might be the error string that my function returned during the TLS handshake? 😅 Lemme look into that

This might be due the workaround we once implemented because quic-go was checking for the presence of certificates in the tls.Config when starting the listener. I recently (#2024) removed that check, so we can get rid of that workaround.

marten-seemann commented 4 years ago

While I'm at it, does my dynamic TLS config that is returned during the handshake need to set something special in the ALPN?

While QUIC mandates the use of ALPN, the H3 server takes care of adding the value required for negotiation H3: https://github.com/lucas-clemente/quic-go/blob/4247c2f06decf32d84c90a82db8de441f3b13cb7/http3/server.go#L99-L101 So you don't need to do anything regarding ALPN.

mholt commented 4 years ago

Aha, found it: https://github.com/lucas-clemente/quic-go/blob/f0a62c05dfadbb6412613e21b69e969a646287fc/http3/server.go#L30 h3-22

And it works!!

So, it was a bug on my end. Sorry to rubber-ducky you like this.

Next question... is there a way to get the currently-required value for the ALPN (h3-22) in this case? I can hard-code it, but the HTTP/3 server doesn't seem to be adding it when using GetConfigForClient.

marten-seemann commented 4 years ago

Next question... is there a way to get the currently-required value for the ALPN (h3-22) in this case? I can hard-code it, but the HTTP/3 server doesn't seem to be adding it when using GetConfigForClient.

Isn't this something the H3 server should take care of, i.e. by wrapping GetConfigForClient and automatically adding the ALPN?

mholt commented 4 years ago

Isn't this something the H3 server should take care of, i.e. by wrapping GetConfigForClient and automatically adding the ALPN?

That's what I was thinking. However, I don't see NextProtos being appended to here: https://github.com/lucas-clemente/quic-go/blob/1e148c20c5cbb387e4019332ce3ee284d4304716/internal/handshake/qtls.go (is that the right place to look?)

marten-seemann commented 4 years ago

We have a strict separation between the transport layer and the application layer, so the place where this would need to happen would be in the http3 package, probably around https://github.com/lucas-clemente/quic-go/blob/4247c2f06decf32d84c90a82db8de441f3b13cb7/http3/server.go#L80 And equivalently for the client.

Speaking about it, I'm beginning to wonder if https://github.com/lucas-clemente/quic-go/blob/4247c2f06decf32d84c90a82db8de441f3b13cb7/http3/server.go#L99-L101 is correct. If we get a tls.Config that already specifies the ALPN for HTTP/2, we might end up in the situation where we negotiate HTTP/2 on top of QUIC, which obviously doesn't make any sense.

mholt commented 4 years ago

Ah, okay.

Caddy adds h2 to the ALPN by default, so if that would mess up the negotiation for HTTP/3, then it should be removed. If the http3 wrapper can do that for me, that would be really convenient and elegant I think...

marten-seemann commented 4 years ago

I think this is something that belongs into http3.

Thinking a bit more about it, I think we need to replace the whole NextProtos slice in the tls.Config, instead of appending to it (or filtering out certain other values). Since the http3 package runs directly on top of quic-go, any other negotiation result than H3 would lead to a connection failure anyway.

mholt commented 4 years ago

Makes sense. I'll leave that up to the http3 package then. Let me know if you need anything from me -- and let me know if you want me to try any changes! :smile: