feat: allow to configure TLS for gRPC servers #303
Conversation
| @@ -15,7 +15,7 @@ | |||
|
|
|||
| // Code generated by protoc-gen-go. DO NOT EDIT. | |||
| // versions: | |||
| // protoc-gen-go v1.30.0 | |||
| // protoc-gen-go v1.31.0 | |||
There was a problem hiding this comment.
Because of the version change of protoc, I have other files that have this kind of modification.
Would you like for me to add all the files that were re-generated with the new version? Or only keep the ones that are relevant to the configuration only?
There was a problem hiding this comment.
Thanks for asking. I'd keep it like this, meaning just adding to the patch what's relevant, otherwise the review will get too much noise.
In another patch, we can bump the generated files if we want, wdyt?
| @@ -43,10 +43,16 @@ message Server { | |||
| string addr = 2; | |||
| google.protobuf.Duration timeout = 3; | |||
| } | |||
| message TLS { | |||
| // path to certificate and private key | |||
| string Certificate = 1; | |||
There was a problem hiding this comment.
this should be lowercase, I believe that the linter didn't pick it up in the CI because there is a bug and buf lint doesn't pick the config directory.
Note in local how make lint or buf lint app/controlplane/internal/conf actually should complain. I'll fix the CI issue.
There was a problem hiding this comment.
Thx. I've updated the file.
| @@ -93,6 +94,18 @@ func NewGRPCServer(c *conf.Server, authConf *conf.Auth, byteService *service.Byt | |||
| if c.Grpc.Timeout != nil { | |||
| opts = append(opts, grpc.Timeout(c.Grpc.Timeout.AsDuration())) | |||
| } | |||
| cert := c.Grpc.TlsConfig.Certificate | |||
There was a problem hiding this comment.
it might make sense to use the secure getter getTLSConfig.GetCertificate this will prevent panics if the TLSConfig is not present, which will not in currently deployed instances.
There was a problem hiding this comment.
or something like if tlsConf := c.Grpc.GetTLSConfig(); tlsConf != nil { /load and append of the key / }
| if cert != "" && privKey != "" { | ||
| cert, err := tls.LoadX509KeyPair(cert, privKey) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
it might make sense to return wrapped context in the error i.e fmt.Errorf("loading gRPC server TLS certificate: %w", err)
| @@ -71,7 +72,7 @@ type Opts struct { | |||
| } | |||
|
|
|||
| // NewGRPCServer new a gRPC server. | |||
| func NewGRPCServer(opts *Opts) *grpc.Server { | |||
| func NewGRPCServer(opts *Opts) (*grpc.Server, error) { | |||
There was a problem hiding this comment.
nice! so you are familiar with wire's dep injection!
| @@ -9,6 +9,10 @@ server: | |||
| grpc: | |||
| addr: 0.0.0.0:9001 | |||
| timeout: 1s | |||
| grpc: | |||
| } | ||
| opts = append(opts, grpc.TLSConfig(&tls.Config{ | ||
| Certificates: []tls.Certificate{cert}, | ||
| MinVersion: tls.VersionTLS12, // gosec complains about insecure minumum version we use default value |
There was a problem hiding this comment.
ptal at the linter issuer. Thanks!
zaibon
force-pushed
the
grpc_tls
branch
3 times, most recently
from
99d2d6e
to
04de714
Compare
Allow to add path to files containing TLS server certificate and private key for gRPC servers. The files must contain PEM encoded data. fixes chainloop-dev#302 Signed-off-by: Christophe de Carvalho <christophe@archipelo.co>
Allow to add path to files containing TLS server certificate and private key for gRPC servers. The files must contain PEM encoded data.
fixes #302