support HTTPS/TLS. #946

ethanschen · 2023-07-23T10:09:20Z

HTTP

Demo

from prometheus_client import start_http_server

start_http_server(8000)

while True:
    pass

Curl HTTPS Request

curl -k https://127.0.0.1:8000

Response

curl: (35) LibreSSL/3.3.6: error:1404B42E:SSL routines:ST_CONNECT:tlsv1 alert protocol version

Support HTTPS/TLS

Demo

from prometheus_client import start_http_server

start_http_server(8000, certfile="server.crt", keyfile="server.key")

while True:
    pass

Generate server cert and server key.

openssl genrsa -out server.key 2048
openssl req -new -key server.key -out server.csr
openssl x509 -req -in server.csr -signkey server.key -out server.crt

Curl HTTPS Request

curl -k https://127.0.0.1:8000

Response

# HELP python_gc_objects_collected_total Objects collected during gc
# TYPE python_gc_objects_collected_total counter
python_gc_objects_collected_total{generation="0"} 618.0
python_gc_objects_collected_total{generation="1"} 5.0
python_gc_objects_collected_total{generation="2"} 0.0
# HELP python_gc_objects_uncollectable_total Uncollectable objects found during GC
# TYPE python_gc_objects_uncollectable_total counter
python_gc_objects_uncollectable_total{generation="0"} 0.0
python_gc_objects_uncollectable_total{generation="1"} 0.0
python_gc_objects_uncollectable_total{generation="2"} 0.0
# HELP python_gc_collections_total Number of times this generation was collected
# TYPE python_gc_collections_total counter
python_gc_collections_total{generation="0"} 39.0
python_gc_collections_total{generation="1"} 3.0
python_gc_collections_total{generation="2"} 0.0
# HELP python_info Python platform information
# TYPE python_info gauge
python_info{implementation="CPython",major="3",minor="11",patchlevel="5",version="3.11.5"} 1.0

Enabled mTLS

Demo

from prometheus_client import start_http_server

start_http_server(8000, certfile="server.crt", keyfile="server.key", client_cafile="client.crt", client_auth_required=True)

while True:
    pass

Generate client cert and client key.

openssl genrsa -out client.key 2048
openssl req -new -key client.key -out client.csr
openssl x509 -req -in client.csr -signkey client.key -out client.crt

Curl HTTPS Request

curl -k https://127.0.0.1:8000

Response

curl: (56) LibreSSL SSL_read: LibreSSL/3.3.6: error:1404C45C:SSL routines:ST_OK:reason(1116), errno 0

Curl HTTPS Request with client cert and client key

curl -k --cert client.crt --key client.key https://127.0.0.1:8000

Response

# HELP python_gc_objects_collected_total Objects collected during gc
# TYPE python_gc_objects_collected_total counter
python_gc_objects_collected_total{generation="0"} 618.0
python_gc_objects_collected_total{generation="1"} 5.0
python_gc_objects_collected_total{generation="2"} 0.0
# HELP python_gc_objects_uncollectable_total Uncollectable objects found during GC
# TYPE python_gc_objects_uncollectable_total counter
python_gc_objects_uncollectable_total{generation="0"} 0.0
python_gc_objects_uncollectable_total{generation="1"} 0.0
python_gc_objects_uncollectable_total{generation="2"} 0.0
# HELP python_gc_collections_total Number of times this generation was collected
# TYPE python_gc_collections_total counter
python_gc_collections_total{generation="0"} 39.0
python_gc_collections_total{generation="1"} 3.0
python_gc_collections_total{generation="2"} 0.0
# HELP python_info Python platform information
# TYPE python_info gauge
python_info{implementation="CPython",major="3",minor="11",patchlevel="5",version="3.11.5"} 1.0

xiqing-zhang · 2023-07-27T08:23:24Z

@karezachen Hi, the feature is very nice. We are looking for prometheus python client with TLS support. Our project is pending on the feature. Can you pls set reviewer and ask them to review and merge it?

xiqing-zhang

It looks good! Simple and but nice!

roidelapluie · 2023-07-27T20:31:08Z

Thank you. I think this is interesting, however we would need to document it better and make sure that we error if the arguments passed are not correct (e.g. an array of 2). Maybe we should review the API to make it more clear which is which in the array, maybe by using multiple arguments or a custom object? What would be alternate proposals here?

Charles1000Chen · 2023-07-28T04:37:49Z

@karezachen Hi Kareza, thank you for the improvement.

Since Python version 3.10: SSLContext without protocol argument is deprecated, we could use ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) instead.

ssl.PROTOCOL_TLS_SERVER
Auto-negotiate the highest protocol version that both the client and server support, and configure the context server-side connections

About the context.load_cert_chain(tls[0], tls[1], tls[2] if len(tls) == 3 else None), its accepted arguments should be

SSLContext.load_cert_chain(certfile, keyfile=None, password=None)

To use mTLS to verify client cert, we may want to cal the load_verify_locations to set ca cert for the SSLContext object before call wrap_socket.

SSLContext.load_verify_locations(cafile=None, capath=None, cadata=None)

ethanschen · 2023-07-28T16:35:36Z

@Charles1000Chen @roidelapluie Hi,Thank you for the review.

I have wrapped the _get_ssl_ctx method based on the tls_auth_handler to support TLS.

If you have any other suggestions, I am more than willing to make the changes.

ethanschen · 2023-07-30T08:40:40Z

@csmarchbanks PTAL

README.md

xiqing-zhang

Doc improvement suggestion. Pls add the optional parameters into the doc. thanks

prometheus_client/exposition.py

andy-maier

See my comments

prometheus_client/exposition.py

README.md

prometheus_client/exposition.py

andy-maier · 2023-07-31T18:23:46Z

I have started using the PR in its current state for our Z HMC Prometheus exporter (see PR zhmcclient/zhmc-prometheus-exporter#361), and it worked. However, see my comments, above.

README.md

prometheus_client/exposition.py

andy-maier · 2023-08-21T08:49:19Z

@karezachen Hi Kareza. I have reviewed the latest version of the PR and have opened some new review comments. I was not able to mark some of the old ones as resolved, so I updated the comments that they were resolved, and then I hided the comments.

Signed-off-by: kareza <[email protected]>

1.A better description of what's new. 2.Modify the parameter order of the start_wsgi_server method to maintain forward compatibility. 3.Enable authentication mode when safe. Signed-off-by: kareza <[email protected]>

Author: Andreas Maier <[email protected]> Signed-off-by: kareza <[email protected]>

ethanschen · 2023-08-28T16:34:43Z

@roidelapluie @xiqing-zhang @xiqing-zhang

Thank you all for the discussion and review of this PR. What is still missing before it can be merged?

andy-maier · 2023-08-30T03:26:43Z

@karezachen Hello Kareza. Thanks much for integrating the remaining changes. My review comments have all been resolved. From my perspective, the PR could be merged.

@roidelapluie @csmarchbanks Anything left to be done from your perspective?

Charles1000Chen · 2023-09-07T02:59:01Z

@karezachen @roidelapluie @csmarchbanks I have tested by zhmc-prometheus-exporter and the mTLS feature works well. Could you help to get the pr merged? Thanks.

csmarchbanks · 2023-09-07T14:59:23Z

👋 I am just coming back from leave and still catching up. I hope to have time to review this soon though if @roidelapluie does not get to it first.

andy-maier · 2023-09-11T16:16:33Z

Note that I was not able to mark my review comments as resolved, but I consider all of them resolved.

GeluConstantin · 2023-09-15T12:01:23Z

Hi,

I tired the HTTPS option but I get an error, TypeError: start_wsgi_server() got an unexpected keyword argument 'tls'

it seems that the tls=[certfile, keyfile] is not a valid argument for the function.

Can you advise on the topic?

Thank you,

Constantin

ethanschen · 2023-09-15T13:56:39Z

@GeluConstantin
Hello Gelu,

I did use the tls parameter in the commit, but subsequent commits have replaced it with other parameters.

Please refer to README to use HTTPS function.

andy-maier · 2023-09-18T07:07:52Z

@GeluConstantin Hi Constantin. I tried the branch of this PR with HTTPS shortly after its last update and it worked fine. Note that the branch has 7 commits, so you need to use all of them.

GeluConstantin · 2023-09-18T11:37:04Z

@GeluConstantin Hi Constantin. I tried the branch of this PR with HTTPS shortly after its last update and it worked fine. Note that the branch has 7 commits, so you need to use all of them.

Hi Andy,

do you know when the PR is going to be integrated into a release? At the moment I use the latest prometheus-client release 0.17.1 and this release has not yet the HTTPS feature.

Thank you for the feebdack,

Constantin

andy-maier · 2023-09-25T09:19:32Z

@GeluConstantin No, I don't know. I am not a maintainer of the project.

@roidelapluie or @csmarchbanks Any chance to get this PR reviewed and merged?

GeluConstantin · 2023-09-25T09:41:47Z

I used fastAPI to provide HTTPS, so my problem is solved.

Kind regards,

Gelu

csmarchbanks · 2023-09-25T22:27:33Z

I am out this week, so unlikely until next week. I have the open question about mTLS behavior when I reviewed it, but otherwise this looks ok to me. I just either need to hear back from @roidelapluie or investigate the behavior next week when I am back.

Charles1000Chen · 2023-10-09T03:18:08Z

@csmarchbanks Any result on your investigation on the PR? We do need your help to have this PR merged. Thanks.

csmarchbanks

Yes, by default mTLS should not be enabled. You can find more detail in https://github.com/prometheus/exporter-toolkit/blob/master/docs/web-configuration.md, but I believe we should not set ssl_cxt.verify_mode to ssl.CERT_REQUIRED by default, and instead have the user opt in.

To be most like the exporter toolkit we would allow users to pass in any verify mode, but I would also be ok with only the default (CERT_NONE), or a flag to enable mTLS and then set CERT_REQUIRED.

prometheus_client/exposition.py

…ertificates. Signed-off-by: kareza <[email protected]>

andy-maier · 2023-10-17T03:27:32Z

@csmarchbanks @roidelapluie Kareza has updated the PR to implement the suggestion with the renamed client_cafile/path parameters.

It seems the only open comment at this point is the one about the default for mTLS. I suggested a solution in the comment further up. Could you please let us know how you like that solution?

- Update `_get_ssl_ctx` func, default not to set `ssl_ctx.verify_mode=ssl.CERT_REQUIRED`. - Update the description of default in README.md. Signed-off-by: kareza <[email protected]>

ethanschen · 2023-10-22T16:09:10Z

@Charles1000Chen @xiqing-zhang @andy-maier @csmarchbanks @roidelapluie

Thank you very much for your help with this PR, I think our problems have been solved.

Is it possible to merge this PR now? Thanks

andy-maier · 2023-11-02T07:35:32Z

@csmarchbanks @roidelapluie Any chance to merge this PR and release a new version? From the perspective of the contributors and reviewers of the PR, all open review comments have been addressed.

csmarchbanks · 2023-11-02T16:02:15Z

This review comment has not been addressed yet, looks like it was hidden by the Github UI though: #946 (comment). I do think it is reasonable to remove client_auth_required and just toggle the behavior based on if client_cafile is not None.

Charles1000Chen · 2023-11-03T06:32:21Z

This review comment has not been addressed yet, looks like it was hidden by the Github UI though: #946 (comment). I do think it is reasonable to remove client_auth_required and just toggle the behavior based on if client_cafile is not None.

@csmarchbanks Hi Chris, the client_auth_required has two logics. The one is the client_auth_required == True which could be toggled to the behavior based on cafile is not None or capath is not None as what your suggested. The other logic is client_auth_required == True andcafile is None and capath is None, the server will use the certificates from os default certificate store to verify the client. Removing client_auth_required will have the server lose the ability of using os default certificates to verify client. What do you think about this? @roidelapluie

andy-maier · 2023-11-06T05:54:30Z

@csmarchbanks As Charles1000Chen points out, removing client_auth_required would loose the ability to use the default certificate store. Is that what you want?

csmarchbanks · 2023-11-06T16:09:43Z

Apparently I had a pending comment and didn't click the send button :(

I am happy with the current behavior (keep being able to use the default certificate store). I would like @roidelapluie to ok that as well since he brought up the concern originally though.

roidelapluie · 2023-11-06T16:33:11Z

Let's keep the ability to use the system store. This PR LGTM, thank you all.

csmarchbanks

👍 I think that covers it, thanks as well!

andy-maier · 2023-11-09T09:56:08Z

@karezachen Thanks again, Kareza for the contributions!
@csmarchbanks Thanks for merging the PR! We will now release a beta version of our zhmc prometheus exporter from the master branch.

ethanschen mentioned this pull request Jul 23, 2023

Add support for exporters with direct HTTPS/TLS support #930

Closed

xiqing-zhang approved these changes Jul 27, 2023

View reviewed changes

ethanschen closed this Jul 28, 2023

ethanschen reopened this Jul 28, 2023

xiqing-zhang reviewed Jul 31, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

xiqing-zhang suggested changes Jul 31, 2023

View reviewed changes

Charles1000Chen mentioned this pull request Jul 31, 2023

Support HTTPS to Prometheus zhmcclient/zhmc-prometheus-exporter#332

Closed