Added proposal to Enable SSL for Metrics Reporter#201
Added proposal to Enable SSL for Metrics Reporter#201antonio-pedro99 wants to merge 2 commits intostrimzi:mainfrom
Conversation
Signed-off-by: Antonio Pedro <tonio.pedro99@gmail.com>
scholzj
left a comment
There was a problem hiding this comment.
Thanks for the proposal.
Pro-tip: You should write proposals as one sentence per line. That makes it much easier for reviewers to comment on a selected sentence. And for you to actually much better understand which comment belonged to which sentence.
|
|
||
| ## 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. No newline at end of file |
There was a problem hiding this comment.
That is not really how monitoring works. You usually collect the data locally and if needed, forward it remotely through central mechanisms. You do not scrape or expose them remotely. So in the rare cases where TLS is actually used, encrypting it somewhere in some gateway would be even much more rare.
We do not support at the same time, it's "either or." |
im-konge
left a comment
There was a problem hiding this comment.
Thanks for the proposal. As Jakub already mentioned, please have one sentence per line. It's better for giving some comments (I started with first paragraph, but it applies to all other paragraphs).
Signed-off-by: Antonio Pedro <antonio.pedro@devexperts.com>
95ff362 to
716870a
Compare
| 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. |
There was a problem hiding this comment.
This is wrong. This is the path of the keystore storing the key.
There was a problem hiding this comment.
Ehh, no. It is key.location - so it would not be a keystore. That would be keystore.location.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The Inline part is wrong ... so is certificate or certificate chain ... and the as a string as well. But it is not a path to a keystore but a path to the key. That is what I was trying to add to your comment. Sorry.
There was a problem hiding this comment.
it is not a path to a keystore but a path to the key
That's not what we had on the bridge. Taking the HTTP bridge proposal it says:
the HTTP Bridge server starts with SSL enabled and the new configurations will allow users to define locations of the keystore files via
http.ssl.keystore.certificate.locationandhttp.ssl.keystore.key.locationor certificate chain and key viahttp.ssl.keystore.certificate.chainandhttp.ssl.keystore.keyin PEM format
so the ssl.keystore.key.location is representative of "location of the keystore file" storing the key, as well as ssl.keystore.certificate.location is the "location of the keystore file" storing the certificate.
While ssl.keystore.certificate.chain and ssl.keystore.key are the inline PEM representation as strings of cert and key. Of course, even in this case I see the "keystore" part in the name being misleading :-(
There was a problem hiding this comment.
My overall understanding of the settings(as I was initially planning):
keystore.location: path to the keystore(containing the key and certificate)
keystore.password: password of the keystore
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:
prometheus.metrics.reporter.listener.ssl.keystore.location: The path of the keystore file(containing both the key and the certificate)prometheus.metrics.reporter.listener.ssl.keystore.password: Password of the specified keystore.
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
prometheus.metrics.reporter.listener.ssl.keystore.key: Inline keystore key in PEM format.prometheus.metrics.reporter.listener.ssl.keystore.certificateorprometheus.metrics.reporter.listener.ssl.keystore.certificate.chain: Inline keystore certificate, or a set of certificate. Since PEM are usually stored as mult-line base 64 string, the value of this option can be passed as multiple line starting with " and end with ".
the keystore.certificate.chain name makes more sense to me though.
There was a problem hiding this comment.
My overall understanding of the settings(as I was initially planning):
keystore.location: path to the keystore(containing the key and certificate)
keystore.password: password of the keystoreThis 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:
prometheus.metrics.reporter.listener.ssl.keystore.location: The path of the keystore file(containing both the key and the certificate)prometheus.metrics.reporter.listener.ssl.keystore.password: Password of the specified keystore.If we bother to trust who is trying to connect a truststore and its password would be needed.
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?
- Keystore
- PEM files through location
- PEM files inlined
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.
There was a problem hiding this comment.
I got the idea now, thanks for clarifying.
It seems like you guys still need to decide about the parameters. Both ways could work for me though
There was a problem hiding this comment.
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.
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 ssl.keystore.type and ssl.truststore.type. Then certificates and keys can be provided via a file, config provider or inline. See https://cwiki.apache.org/confluence/display/KAFKA/KIP-651+-+Support+PEM+format+for+SSL+certificates+and+private+key for more details
So I think that would work for both worlds.
There was a problem hiding this comment.
The Kafka configuration is a bit weird when handling PEM files as paths because it mixes it with the keystore and truststore options. The ssl.trustore.location is kind of ok - while PEM is not a certificate store, it is at least commonly passed as a single file. But ssl.keystore.location does not work very well because in most cases, the server certificate would be two file - private key and public key. And not a single file. So, assuming we want to support PEMs as file path, not sure we should blindly copy the options from Kafka.
So I think we should first make the decision what do we want to support:
- JKS/PKCS12 stores
- Path to PEM files
- Inlined PEM files
And then figure out the best configuration options.
|
|
||
| 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. |
There was a problem hiding this comment.
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.
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.
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).
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.
It does. Keystores either hold the private-public-key bundle ... or multiple public keys.
| - `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. | ||
| - `prometheus.metrics.reporter.listener.ssl.keystore.certificate.chain`: The certificate chain in PEM format. |
There was a problem hiding this comment.
I would mention "inline" as for the key below.
|
Hi @scholzj @ppatierno I still have not seen in which direction we are moving with this. |
This proposal addresses 65: Add support for https: