search

ribbybibby / ssl_exporter

Exports Prometheus metrics for TLS certificates
Apache License 2.0
525 stars 99 forks source link

Include the root certificate? #46

Closed rgl closed 4 years ago

rgl commented 4 years ago

Currently ssl_exporter is generating a metric over each PeerCertificates.

It would be useful to also include the root certificates (i.e. full chain), something akin to what the blackbox_exporter does.

What do you think?

ribbybibby commented 4 years ago

Thanks for the issues @rgl! Including the root from the verified chain sounds like a good idea to me.

rgl commented 4 years ago

My idea was to include all the certificates from VerifiedChains, but doing so will break this query from the README:

Number of certificates in the chain:

count(ssl_cert_not_after) by (instance, serial_no, issuer_cn)

Because the number of certificates will now account for the certificates in all verified chains, not just a single one.

I'm seeing a couple of possibilities:

  1. assume that as a required change and brake that query
  2. include a chain_no label which means there will be repeated certificates in the result 2.1. NB I'm not sure if this number will ever be a stable one, I mean, I have no idea if chain_no changes between different requests
  3. introduce a new metric, ssl_chain_cert_not_after 3.1. we have to decide if it will have a chain_no label 3.2. we have to decide if it will include any of the peer certificates, either by omitting them, or by adding a label like peer=1, or just include the entire chains?
  4. introduce a parameter somewhere to modify the behavior of the ssl_cert_not_after metric, that, when true, it creates a set of certificates from VerifiedChains.
  5. any other idea :-)
ribbybibby commented 4 years ago

If I understand it correctly, PeerCertificates contains the certificates presented by the server, whereasVerifiedChains contains the chains of trust that can be established between the server certificate and the root CAs of the client. Therefore, they represent different things and I think they warrant separate metrics. Perhaps ssl_verified_cert_not_after?

3.2. we have to decide if it will include any of the peer certificates, either by omitting them, or by adding a label like peer=1, or just include the entire chains?

As it's a separate metric, for a separate thing, I don't see a problem with it duplicating certificates that may appear in the peer certificates.

  1. include a chain_no label which means there will be repeated certificates in the result 2.1. NB I'm not sure if this number will ever be a stable one, I mean, I have no idea if chain_no changes between different requests

3.1. we have to decide if it will have a chain_no label It seems to me that the chains would need to be represented in some form in order to achieve what the blackbox_exporter metric achieves in allowing you to get the date until the last valid chain is valid until. That seems like the most obviously useful application of examining the verified chain, so it would be nice to support it.

Perhaps we could order the chains by the length of their validity. For instance, 0 would be the chain that will be valid for the longest and therefore ssl_verified_cert_not_after{chain_no="0"} - time() would give you what the blackbox_exporter metric provides.

rgl commented 4 years ago

ssl_verified_cert_not_after sounds pretty good, lets use that and the chain_no label.

Ordering the chain like you mentioned sounds good too. But I'm not sure if that is what is already being done by the go TLS stack. Anyways, I guess we should have tests for that :-)

BTW, if I understood correctly, blackbox_exporter is not working like what you've described. It seems to be returning the certificate that expires the soonest among all the chains. Which leaves me wondering why its doing that.

ribbybibby commented 4 years ago

BTW, if I understood correctly, blackbox_exporter is not working like what you've described. It seems to be returning the certificate that expires the soonest among all the chains. Which leaves me wondering why its doing that.

https://github.com/prometheus/blackbox_exporter/pull/681 - I think it's a bug.

ribbybibby commented 4 years ago

See: https://github.com/ribbybibby/ssl_exporter/pull/48