-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support mTLS in gRPC exporters #927
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,8 +16,11 @@ | |
package configgrpc | ||
|
||
import ( | ||
"crypto/tls" | ||
"crypto/x509" | ||
"fmt" | ||
"io/ioutil" | ||
"path/filepath" | ||
"time" | ||
|
||
"google.golang.org/grpc" | ||
|
@@ -42,16 +45,8 @@ type GRPCSettings struct { | |
// collector. Currently the only supported mode is `gzip`. | ||
Compression string `mapstructure:"compression"` | ||
|
||
// Certificate file for TLS credentials of gRPC client. Should | ||
// only be used if `secure` is set to true. | ||
CertPemFile string `mapstructure:"cert_pem_file"` | ||
|
||
// Whether to enable client transport security for the exporter's gRPC | ||
// connection. See https://godoc.org/google.golang.org/grpc#WithInsecure. | ||
UseSecure bool `mapstructure:"secure"` | ||
|
||
// Authority to check against when doing TLS verification | ||
ServerNameOverride string `mapstructure:"server_name_override"` | ||
// TLSConfig struct exposes TLS client configuration. | ||
TLSConfig TLSConfig `mapstructure:",squash"` | ||
|
||
// The keepalive parameters for client gRPC. See grpc.WithKeepaliveParams | ||
// (https://godoc.org/google.golang.org/grpc#WithKeepaliveParams). | ||
|
@@ -62,6 +57,26 @@ type GRPCSettings struct { | |
WaitForReady bool `mapstructure:"wait_for_ready"` | ||
} | ||
|
||
// TLSConfig exposes client TLS configuration. | ||
type TLSConfig struct { | ||
// Root CA certificate file for TLS credentials of gRPC client. Should | ||
// only be used if `secure` is set to true. | ||
CaCert string `mapstructure:"cert_pem_file"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The config property name hasn't been changed. Can we make a breaking change and change it to include There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can probably do that in a separate PR. This is a breaking change when it comes to the code (not configuration) so you will need to followup with a PR in the collector-contrib where you update otel version (using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dmitryax updated OTEL collector in the contrib in PR https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/220/files#diff-993db3ecf423549b9fcb5b7e4d195efeR10. That corresponds to 80ee563 that landed to master after this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bogdandrutu ping me if there is anything I can do more. The refactoring/unifying of the TLS spec will be done in #933 |
||
|
||
// Client certificate file for TLS credentials of gRPC client. | ||
ClientCert string `mapstructure:"client_cert_pem_file"` | ||
|
||
// Client key file for TLS credentials of gRPC client. | ||
ClientKey string `mapstructure:"client_cert_key_file"` | ||
|
||
// Whether to enable client transport security for the exporter's gRPC | ||
// connection. See https://godoc.org/google.golang.org/grpc#WithInsecure. | ||
UseSecure bool `mapstructure:"secure"` | ||
|
||
// Authority to check against when doing TLS verification | ||
ServerNameOverride string `mapstructure:"server_name_override"` | ||
} | ||
|
||
// KeepaliveConfig exposes the keepalive.ClientParameters to be used by the exporter. | ||
// Refer to the original data-structure for the meaning of each parameter. | ||
type KeepaliveConfig struct { | ||
|
@@ -82,18 +97,18 @@ func GrpcSettingsToDialOptions(settings GRPCSettings) ([]grpc.DialOption, error) | |
} | ||
} | ||
|
||
if settings.CertPemFile != "" { | ||
creds, err := credentials.NewClientTLSFromFile(settings.CertPemFile, settings.ServerNameOverride) | ||
if settings.TLSConfig.CaCert != "" && !settings.TLSConfig.UseSecure { | ||
creds, err := credentials.NewClientTLSFromFile(settings.TLSConfig.CaCert, settings.TLSConfig.ServerNameOverride) | ||
if err != nil { | ||
return nil, err | ||
} | ||
opts = append(opts, grpc.WithTransportCredentials(creds)) | ||
} else if settings.UseSecure { | ||
certPool, err := x509.SystemCertPool() | ||
} else if settings.TLSConfig.UseSecure { | ||
tlsConf, err := settings.TLSConfig.LoadTLSConfig() | ||
if err != nil { | ||
return nil, err | ||
return nil, fmt.Errorf("failed to load TLS config: %w", err) | ||
} | ||
creds := credentials.NewClientTLSFromCert(certPool, settings.ServerNameOverride) | ||
creds := credentials.NewTLS(tlsConf) | ||
opts = append(opts, grpc.WithTransportCredentials(creds)) | ||
} else { | ||
opts = append(opts, grpc.WithInsecure()) | ||
|
@@ -110,3 +125,56 @@ func GrpcSettingsToDialOptions(settings GRPCSettings) ([]grpc.DialOption, error) | |
|
||
return opts, nil | ||
} | ||
|
||
// LoadTLSConfig loads TLS certificates and returns a tls.Config. | ||
func (c TLSConfig) LoadTLSConfig() (*tls.Config, error) { | ||
certPool, err := c.loadCertPool() | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to load CA CertPool: %w", err) | ||
} | ||
// #nosec G402 | ||
tlsCfg := &tls.Config{ | ||
RootCAs: certPool, | ||
ServerName: c.ServerNameOverride, | ||
} | ||
|
||
if (c.ClientCert == "" && c.ClientKey != "") || (c.ClientCert != "" && c.ClientKey == "") { | ||
return nil, fmt.Errorf("for client auth via TLS, either both client certificate and key must be supplied, or neither") | ||
} | ||
if c.ClientCert != "" && c.ClientKey != "" { | ||
tlsCert, err := tls.LoadX509KeyPair(filepath.Clean(c.ClientCert), filepath.Clean(c.ClientKey)) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to load server TLS cert and key: %w", err) | ||
} | ||
tlsCfg.Certificates = append(tlsCfg.Certificates, tlsCert) | ||
} | ||
|
||
return tlsCfg, nil | ||
} | ||
|
||
var systemCertPool = x509.SystemCertPool // to allow overriding in unit test | ||
|
||
func (c TLSConfig) loadCertPool() (*x509.CertPool, error) { | ||
if len(c.CaCert) == 0 { // no truststore given, use SystemCertPool | ||
certPool, err := systemCertPool() | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to load SystemCertPool: %w", err) | ||
} | ||
return certPool, nil | ||
} | ||
// setup user specified truststore | ||
return c.loadCert(c.CaCert) | ||
} | ||
|
||
func (c TLSConfig) loadCert(caPath string) (*x509.CertPool, error) { | ||
caPEM, err := ioutil.ReadFile(filepath.Clean(caPath)) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to load CA %s: %w", caPath, err) | ||
} | ||
|
||
certPool := x509.NewCertPool() | ||
if !certPool.AppendCertsFromPEM(caPEM) { | ||
return nil, fmt.Errorf("failed to parse CA %s", caPath) | ||
} | ||
return certPool, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QQ: should this be moved in a package configtls in order to be shared with other protocols that support tls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I feel that we are inconsistent between calling the structs settings vs config. May be done in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to this request - I'm working on adding mTLS for receivers/exporters that use HTTP.