-
Notifications
You must be signed in to change notification settings - Fork 78
Added proposal to Enable SSL for Metrics Reporter #201
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 |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| # Enable SSL for Metrics Reporter | ||
|
|
||
| This proposal is to enable SSL for the Metrics Reporter in Strimzi. | ||
| This would allow for exporting metrics over HTTPs. | ||
|
|
||
| ## Current situation | ||
|
|
||
| The current metrics reporter in Strimzi does not support SSL, which means that metrics are exported over HTTP. | ||
| This can be a security concern in some environments, as it allows for potential interception of sensitive data. | ||
|
|
||
| ## Motivation | ||
|
|
||
| Enabling SSL for the Metrics Reporter would provide an additional layer of security for users who need to export metrics over HTTPs. | ||
| This would allow for secure communication between the Metrics Reporter and any monitoring tools that are consuming the metrics, such as Prometheus or Grafana. | ||
| Additionally, it would align with best practices for securing data in transit, especially in environments where sensitive information may be included in the metrics. | ||
|
|
||
| ## Proposal | ||
|
|
||
| I propose to add support for SSL in the Metrics Reporter by allowing users to configure SSL settings, such as providing PEM file and certificates. | ||
| This would involve updating the Metrics Reporter to use an HTTPs server instead of an HTTP server, and ensuring that the necessary SSL configurations are properly handled. | ||
|
|
||
| ### Configurations | ||
|
|
||
| We will start by adding new configuration options for the Metrics Reporter, such as: | ||
| - `prometheus.metrics.reporter.listener`: if set to an address starting with `https://`, the Metrics Reporter will use an HTTPs server instead of an HTTP server. | ||
| - `prometheus.metrics.reporter.listener.ssl.keystore.certificate.location`: The path of the PEM file containing the certificate/certificate chain for the keystore. | ||
| - `prometheus.metrics.reporter.listener.ssl.keystore.key.location`: Inline PEM certificate or certificate chain as a string. | ||
|
Member
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. This is wrong. This is the path of the keystore storing the key.
Member
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. Ehh, no. It is
Member
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 am lost here. The definition says "Inline PEM certificate or certificate chain as a string." which for me it means that I am putting the PEM "string" into this parameter, while the name is a "location" which is a path to a file. So what is this parameter really?
Member
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
Member
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.
That's not what we had on the bridge. Taking the HTTP bridge proposal it says:
so the While
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. My overall understanding of the settings(as I was initially planning):
This is the base settings to the KeyStoreManagerFactory would need. If we just bother about identifying the HTTPserver to the clients, the first config would look like:
If we bother to trust who is trying to connect a truststore and its password would be needed. Keeping the trustore out for now, if we are interested in set the above configs directly in server/client.properties` in PEM formats(because we do not want to pass files path), the final config would look like
the
Member
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.
Don't get me wrong. This is not a bad assumption. And actually, these might be the options to ask for by many users as using keystores such as JKS or PKCS12 is pretty common in Java land. I do not have a problem if you want to keep it. But at the same time, using keystores is pretty uncommon in the cloud-native land. That is where you use PEM files. And that is where Strimzi and Prometheus live. So, for the integration into the rest of Strimzi, I think we definitely want PEM files support. If we one day integrate it directly into the Strimzi API (I have some doubts about it, but not 100% sure), we will likely do it through the inline options as that is what we use everywhere else. If the integration is done by some opaque user configuration (i.e. some configuration map), it is IMHO more likely to use paths to PEM files. So, we possibly might need/want all three ways?
But that is what this proposal needs to decide. I guess the first thing we need is that we all understand the differences and then as maintainers + Mickael we need to decide what is the right direction.
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 got the idea now, thanks for clarifying.
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.
Thanks for that insight, it's not something I was considering. The configs Kafka offers allow users to use JKS, PKCS12 and PEM. The type is specified via the So I think that would work for both worlds.
Member
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 Kafka configuration is a bit weird when handling PEM files as paths because it mixes it with the keystore and truststore options. The So I think we should first make the decision what do we want to support:
And then figure out the best configuration options. |
||
| - `prometheus.metrics.reporter.listener.ssl.keystore.certificate.chain`: The certificate chain in PEM format. | ||
antonio-pedro99 marked this conversation as resolved.
Show resolved
Hide resolved
Member
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 would mention "inline" as for the key below. |
||
| - `prometheus.metrics.reporter.listener.ssl.keystore.key`: Inline PEM key as a string. | ||
| - `prometheus.metrics.reporter.listener.ssl.enabled.protocols`: Comma separated list of enabled secure transport protocols. | ||
| - `prometheus.metrics.reporter.listener.ssl.enabled.cipher.suites`: Comma separated list of cipher suites that the server will support. | ||
|
|
||
| When SSL is enabled, the Metrics Reporter will use the provided settings to establish secure connections with clients. | ||
| The implementation will ensure that the necessary SSL configurations are properly handled, and that the Metrics Reporter can still function as expected when SSL is enabled. | ||
|
|
||
| Kafka already has support for SSL, so one would suggest reusing the existing SSL config options for the Metrics Reporter. | ||
| However, I trust that the metrics reporter should have its own set of SSL config options. | ||
| This is because it can have different requirements and configurations than the Kafka components, and it allows for more flexibility in how SSL is configured for the Metrics Reporter specifically. | ||
| Additionally, it would avoid any potential conflicts or confusion that could arise from sharing SSL config options between the Metrics Reporter and Kafka components. | ||
|
|
||
| ### Implementation | ||
|
|
||
| We will start the implementation by adding the support of the new configuration options for SSL in the metric reporter. | ||
|
|
||
| Prometheus HttpServer will be modified with a HttpsConfigurator that will be responsible for configuring the SSL settings based on the provided configuration options. | ||
| This will involve setting up the SSL context, loading the keys and certificates, and later configuring the HTTPs server to use the SSL context for secure communication. | ||
|
|
||
| The implementation will also include error handling to ensure that any issues with SSL configuration are properly logged and do not cause the Metrics Reporter to fail unexpectedly. | ||
| Additionally, we will ensure that the Metrics Reporter can still function as expected when SSL is enabled, and that it can handle either HTTP and HTTPs requests based on the configuration provided by the user. | ||
|
|
||
| ## Affected/not affected projects | ||
|
|
||
| Primarily, this change will affect the metrics reporter. | ||
| This proposal does not cover configuring SSL from the Strimzi Operators, when the Metrics Reporter is used in a Strimzi deployment. | ||
| Such implementation shall be covered in a separate proposal. | ||
|
|
||
| ## Compatibility | ||
|
|
||
| This proposal is designed to be backward compatible, as it will not change the existing behavior of the Metrics Reporter when SSL is not enabled. | ||
| Users who do not wish to use SSL can continue to use the Metrics Reporter as they currently do, while those who want to enable SSL can do so by providing the necessary configuration options. | ||
| Users already using the Metrics Reporter will not be affected by this change, as it will only impact those who choose to enable SSL. | ||
|
|
||
| ## Rejected alternatives | ||
|
|
||
| Not rejected, but considered alternatives include making use of Firewalls and API Gateways to secure the communication between the Metrics Reporter and monitoring tools. | ||
| However, these alternatives would require additional and complex configuration, and may not be feasible for all users. | ||
| Enabling SSL directly in the Metrics Reporter provides a more straightforward and integrated solution for securing metrics export and is more industrial standard for securing data in transit. | ||
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.
It's not a path to a PEM file. It's a path to a keystore including certificates in PEM format.
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.
Same as above. Also, if you would have a keystore, you would have just one location as it would have the certificate and public key in one file (e.g. PKCS12 or JKS store).
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.
I do not think a keystore holds any public key, right?
A keystore primarly holds private key, secret key and trusted certificate
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.
It does. Keystores either hold the private-public-key bundle ... or multiple public keys.