x509: certificate is valid for drand.example.com, not drand.example.com:443

[mahrud]

In this line, the serverNameOverride parameter is set to be the host:port address, which is different from the server name:
https://github.com/dedis/drand/blob/5aae894956265da27653de203863f4f045915226/net/client_grpc.go#L122
In particular, I got a pretty strange error when testing drand run:
dkg: failed to send deal to drand.example.com:443: rpc error: code = Unavailable desc = all SubConns are in TransientFailure, latest connection error: connection error: desc = "transport: authentication handshake failed: x509: certificate is valid for *.example.com, drand.example.com, not drand.example.com:443"

I think this could also be a side effect (if not bug) of my load balancer setup, but I realized that the docs for NewClientTLSFromFile suggest that this parameter be used only for testing:

// NewClientTLSFromFile constructs TLS credentials from the input certificate file for client.
// serverNameOverride is for testing only. If set to a non empty string,
// it will override the virtual host name of authority (e.g. :authority header field) in requests.
func NewClientTLSFromFile(certFile, serverNameOverride string) (TransportCredentials, error) {

Is p.Address() needed there? If not, removing it will relieve some headache on my end.

[nikkolasg]

So when I put the serverNameOverride to "", the tests fail with

authentication handshake failed: x509: cannot validate certificate for 127.0.0.1 because it doesn't contain any IP SANs

Since it's explicitely stated as a testing method, I figure we need to remove that call to NewClientTLSFromFile and separate that "testing logic" from the main logic, while still being reachable from the main command line, because I need to be able to indicate custom certificates on the cli flags to run the docker test.

Basically, that would require separating the method creating new connections into two, and have two ways of a creating a GRPC client: regular one that will not even need TransportCredentials since it will use the OS root trust store, and one that uses this NewClientTLSFromFile.

[nikkolasg]

I'll try to see what I can come up with this week, but that's gonna take a little bit of time though, so I can't promise anything.

[mahrud]

I think only serverNameOverride is for testing, not the method itself.

I can think of a couple of simpler ways that don't require two separate calls. One is to separate the port number as its own parameter for each node, the other is to have an optional parameter that is the SNI.

I'm working on a branch with a few fixes, I can implement this and let me know if it doesn't pass the tests.

[nikkolasg]

I've come up with a way to disable the serverNameOverride ( = ""): I found out that the third party library I use to generate on the fly certificates did not add correctly the IP SANS when I gave the IP address of a node so it was added as a regular DNS name. I'm working on a PR to fix that, should be done soon.

[nikkolasg]

Hopefully #44 should have solved the problem, let me know if you have still issues with it otherwise feel free to close it.

[mahrud]

It seems to be working, thanks!