Skip to content
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

Include the root certificate? #46

Closed
rgl opened this issue Aug 13, 2020 · 6 comments
Closed

Include the root certificate? #46

rgl opened this issue Aug 13, 2020 · 6 comments

Comments

@rgl
Copy link
Contributor

rgl commented Aug 13, 2020

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
Copy link
Owner

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

@rgl
Copy link
Contributor Author

rgl commented Aug 13, 2020

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
Copy link
Owner

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
Copy link
Contributor Author

rgl commented Aug 20, 2020

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
Copy link
Owner

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.

prometheus/blackbox_exporter#681 - I think it's a bug.

@ribbybibby
Copy link
Owner

See: #48

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants