-
Notifications
You must be signed in to change notification settings - Fork 48
NETOBSERV-1101: TLS/mTLS hardening #2482
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
base: main
Are you sure you want to change the base?
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 |
|---|---|---|
|
|
@@ -423,7 +423,8 @@ type FlowCollectorKafka struct { | |
| // Kafka topic to use. It must exist. NetObserv does not create it. | ||
| Topic string `json:"topic"` | ||
|
|
||
| // TLS client configuration. When using TLS, verify that the address matches the Kafka port used for TLS, generally 9093. | ||
| // TLS and mTLS client configuration. When using TLS, verify that the address matches the Kafka port used for TLS, generally 9093. | ||
| // We recommend the use of mTLS for higher security standards. | ||
| // +optional | ||
| TLS ClientTLS `json:"tls"` | ||
|
|
||
|
|
@@ -516,12 +517,13 @@ type FlowCollectorOpenTelemetry struct { | |
| Metrics FlowCollectorOpenTelemetryMetrics `json:"metrics"` | ||
| } | ||
|
|
||
| type ServerTLSConfigType string | ||
| type TLSConfigType string | ||
|
|
||
| const ( | ||
| ServerTLSDisabled ServerTLSConfigType = "Disabled" | ||
| ServerTLSProvided ServerTLSConfigType = "Provided" | ||
| ServerTLSAuto ServerTLSConfigType = "Auto" | ||
| TLSDisabled TLSConfigType = "Disabled" | ||
| TLSProvided TLSConfigType = "Provided" | ||
| TLSAuto TLSConfigType = "Auto" | ||
| TLSAutoMTLS TLSConfigType = "Auto-mTLS" | ||
| ) | ||
|
|
||
| // `ServerTLS` define the TLS configuration, server side | ||
|
|
@@ -534,7 +536,7 @@ type ServerTLS struct { | |
| // +kubebuilder:validation:Enum:="Disabled";"Provided";"Auto" | ||
| // +kubebuilder:validation:Required | ||
| //+kubebuilder:default:="Disabled" | ||
| Type ServerTLSConfigType `json:"type,omitempty"` | ||
| Type TLSConfigType `json:"type,omitempty"` | ||
|
|
||
| // TLS configuration when `type` is set to `Provided`. | ||
| // +optional | ||
|
|
@@ -547,7 +549,22 @@ type ServerTLS struct { | |
|
|
||
| // Reference to the CA file when `type` is set to `Provided`. | ||
| // +optional | ||
| ProvidedCaFile *FileReference `json:"providedCaFile,omitempty"` | ||
| ProvidedCAFile *FileReference `json:"providedCaFile,omitempty"` | ||
| } | ||
|
|
||
| // `ClientServerTLS` define the TLS configuration for both client and server sides | ||
| type ClientServerTLS struct { | ||
|
Member
Author
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. Hey @OlivierCazade - you introduced recently "ServerTLS", and now I need yet something slightly different, to handle both sides in 1 config |
||
| // TLS client certificate reference. | ||
| // +optional | ||
| ClientCert *CertificateReference `json:"clientCert,omitempty"` | ||
|
|
||
| // TLS server certificate reference. | ||
| // +optional | ||
| ServerCert *CertificateReference `json:"serverCert,omitempty"` | ||
|
|
||
| // Reference to the CA file. | ||
| // +optional | ||
| CAFile *FileReference `json:"caFile,omitempty"` | ||
| } | ||
|
|
||
| // `MetricsServerConfig` define the metrics server endpoint configuration for Prometheus scraper | ||
|
|
@@ -707,6 +724,10 @@ type FlowCollectorFLP struct { | |
| //+optional | ||
| SlicesConfig *SlicesConfig `json:"slicesConfig,omitempty"` | ||
|
|
||
| // Service configuration, only used when `spec.deploymentModel` is `Service`. | ||
| // +optional | ||
| Service *ProcessorServiceConfig `json:"service,omitempty"` | ||
|
|
||
| // `advanced` allows setting some aspects of the internal configuration of the flow processor. | ||
| // This section is aimed mostly for debugging and fine-grained performance optimizations, | ||
| // such as `GOGC` and `GOMAXPROCS` environment variables. Set these values at your own risk. | ||
|
|
@@ -1510,6 +1531,22 @@ type SubnetLabel struct { | |
| Name string `json:"name,omitempty"` | ||
| } | ||
|
|
||
| type ProcessorServiceConfig struct { | ||
| // Select the type of TLS configuration:<br> | ||
| // - `Disabled` to not configure TLS for the endpoint. | ||
| // - `Provided` to manually provide cert file and a key file. [Unsupported (*)]. | ||
| // - `Auto` (default) to try to determine if TLS can be enabled based on the running environment. | ||
| // - `Auto-mTLS` to preconfigure mTLS. [Unsupported (*)]. | ||
| // +kubebuilder:validation:Enum:="Disabled";"Provided";"Auto";"Auto-mTLS" | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:default:="Auto" | ||
| TLSType TLSConfigType `json:"tlsType,omitempty"` | ||
|
|
||
| // TLS or mTLS configuration when `type` is set to `Provided`. | ||
| // +optional | ||
| ProvidedCertificates *ClientServerTLS `json:"providedCertificates,omitempty"` | ||
|
Contributor
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. Should we add webhooks validations for the tls feature such as checking for these certs if provided is configured?.
Member
Author
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. I think that would go against good practices, although I'm right now trying to find a reference and don't find it. Basically, since validation webhooks live in the You can note that we're already not the good guys here, because we have a dependency on
Member
Author
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. hey, I'm not sure if that's what you was thinking about, but I added a simple check that the Provided config is not empty (it won't check live the presence of certificates, but at least, it makes sure the configuration is consistent)
Contributor
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. Yes, I was thinking about that, thanks! |
||
| } | ||
|
|
||
| // Add more exporter types below | ||
| type ExporterType string | ||
|
|
||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Can we keep this yaml snippet here too? I think it's useful for a quick view.
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.
yes, np - I hesitated to remove it (for less duplication) - but I agree it's useful here as well