Add go 1.19 #224
Add go 1.19 #224
Conversation
MathieuBordere
force-pushed
the
go-1.19
branch
5 times, most recently
from
e332e6a
to
c3d1a84
Compare
- add go 1.19.{5,6}
- remove Ubuntu 18.04 and add 22.04
…eters Signed-off-by: Mathieu Borderé <mathieu.bordere@canonical.com>
Signed-off-by: Mathieu Borderé <mathieu.bordere@canonical.com>
Signed-off-by: Mathieu Borderé <mathieu.bordere@canonical.com>
seems incompatible with go 1.19 Signed-off-by: Mathieu Borderé <mathieu.bordere@canonical.com>
Signed-off-by: Mathieu Borderé <mathieu.bordere@canonical.com>
|
@tomponline @cole-miller Mostly just asking feedback on the updated |
|
My worry is a bit that current users could use certificates that are incompatible with TLS 1.3 I could do a sanity check on the provided cert and certpool, like setup a server and client with the provided config and see if I can setup a connection. If not, revert to TLS 1.2 or err out. |
|
CC @simondeziel may have a comment on this . |
| @@ -44,7 +44,7 @@ const ( | |||
| ) | |||
|
|
|||
| func signalChannel() chan os.Signal { | |||
| ch := make(chan os.Signal) | |||
| ch := make(chan os.Signal, 32) | |||
There was a problem hiding this comment.
It can probably be a lot smaller, just took a large enough value. I don't think the overhead is significant as it's only in used in the demo and benchmark commands.
There was a problem hiding this comment.
Its fine, just I always wonder about "magic numbers" :)
I think I would be leaning towards removing as many of the explicitly set options as possible and relying on the defaults from Go as they change from version to version (thinking MinVersion, MaxVersion, CurvePreferences, PreferServerCipherSuites (deprecated), CipherSuites). And then make that clear in the comment, and if a user needs specific config then they can create a custom config. |
Let go manage sane defaults, except MinVersion, a server can still use TLS 1.0 which seems a bit too old imo. - value from `curvePreferences` breaks go 1.19.6 so removed it. - `PreferServerCipherSuites` is deprecated. - Let go manage `CipherSuites` as it will take a safe default list.
There was a problem hiding this comment.
I quite like the simplification for the TLS server and client.
LXD 5.0+ requires TLS 1.3 unless LXD_INSECURE_TLS is set and that applies to both the server and the client.
|
@freeekanayaka maybe you have an opinion on the TLS configs? |
|
I am very far from being a TLS guru, but I agree with @tomponline that removing explicitly set options (especially very technical stuff like preferred elliptic curves) from the config is a good idea, since we can trust the Go folks to pick solid defaults. |
| @@ -28,7 +28,6 @@ func Example() { | |||
| } | |||
|
|
|||
| fmt.Printf("0x%x %s\n", node.ID(), node.Address()) | |||
| // Output: 0x2dc171858c3155be 127.0.0.1:9001 | |||
There was a problem hiding this comment.
Why was this change needed? Has format of testable examples changed in Go 1.19? I would be very surprised.
There was a problem hiding this comment.
go 1.19 only allows a single Output comment block and there is already one at the end of the function see e.g. this test run.
# github.com/canonical/go-dqlite/app_test
Error: app/example_test.go:31:2: there can only be one output comment block per example
Error: app/example_test.go:95:2: output comment block must be the last comment block
|
Looks good to me. Those settings were the "recommended" ones in terms of tightening TLS parameters, but it seems reasonable to just use Go's defaults, which might be a bit more permissive, but clearly good enough. |
This PR introduces:
go 1.19.6making connections fail withwrite handshake: local error: tls: bad record MACsee e.g. here, don't know root cause yet and it's probably not a good idea to wait for a new go release if it would indeed be a bug in go (which it is probably not).The config in this PR fixes the issue. Anyone has an opinion on the proposed config in this PR?
go 1.19.xdeadcode,overallsedit: Just removing the curve preferences from the config seems to also fix the issue, this is probably a safer bet than also forcing TLS 1.3