transport: add possibility to use TLS secured transport layer #236
Conversation
cmd/buildkitd/main.go
Outdated
| certPool := x509.NewCertPool() | ||
| ca, err := ioutil.ReadFile(caFile) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not read ca certificate: %s", err) |
|
Sorry for the test failures, I did simply forgot to run them locally. Will do many manual testing + do the simple |
…er- and clientside;
trusch
force-pushed
the
feature/tls-transport
branch
from
316c849 to
03f717b
Compare
|
@AkihiroSuda all checks passed, and I rebased to master for cleaner history. Hope to get this merged soon, especially because client auth is very urgent for my usecase ;) |
There was a problem hiding this comment.
I wonder if these options should be defined per listener instead of per daemon. Is there ever a need to use this for unix sockets? It would be somewhat clearer to me if tls would be used if you specify tls:// on the client/server --addr flag and not if tcp or unix sockets are used there. And if you listen/dial on tls:// and don't provide these values it would error.
Even if you have a use case for tls on local connections I don't think we need --server-name on the client. It should be determined from the address and in case of local connection, it doesn't matter.
@vdemeester PTAL
cmd/buildctl/main.go
Outdated
| Value: "", | ||
| }, | ||
| cli.StringFlag{ | ||
| Name: "ca-cert", |
There was a problem hiding this comment.
Use the same flags as docker for consistency. tlscacert etc.
…sistency with docker + guess tls-server-name from target address in client if not specified explicitly;
|
I renamed the commandline flags to be consistent with the docker flags and I get the name of the server from the address of not specified explicit. That way the user must not supply it, but can do so. TLS itself is agnostic about the used underlying transport, so I would say that a tls:// scheme wouln't be consistent with tcp:// and unix:// schemes because it means something different. @tonistiigi |
There was a problem hiding this comment.
Same as @trusch on tls://it feels weird. Other than that, looks good to me 😉
| Value: defaultAddress, | ||
| }, | ||
| cli.StringFlag{ |
There was a problem hiding this comment.
What would be the use case of this one ? a certificate issued for another address than the specified addr ?
There was a problem hiding this comment.
Imagine you have your certificates generated for a specific addressd and your buildserver is running, but you need to access it directly via IP because your DNS is broken or you have setup some port-forwarding to debug things in for example a k8s deployment.
Then it would be usefull to be able to tell the client which name it should expect in the servers certificate.
What
This PR adds logic and commandline flags for buildkitd and buildctl to configure TLS certificates, keys and CA certificates.
There are now the following three options:
From the API perspective this is implemented as the ClientOption WithCredentials() which takes pathes to the requested files as arguments.
Why
To be able to effectively prevent man in the middle attacks or unauthorized access in unsecure networks, using TLS is the way to go in the current gRPC setup.