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

Add leaf certificate details in probe_tls_certificate_info metric #943

Merged
merged 3 commits into from
Aug 9, 2022

Conversation

djcode
Copy link
Contributor

@djcode djcode commented Jul 6, 2022

I tried implementing the RFE mentioned in #892 as I also need something like this for a project.

}

func getDNSNames(state *tls.ConnectionState) string {
cert := state.PeerCertificates[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I understand correctly, PeerCertificates[0] will return leaf certificate.

do we want to only export details of leaf certificate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question! The details of this certificate in my opinion are the most important, if we were to add others, that wil require rethink in how these values are extracted.

I reused the logic behind the "probe_ssl_last_chain_info" metric and in hindsight, that metric name is more succinct and applicable for the info the RFE requested so maybe it would be better to just add these labels there?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, probe_ssl_last_chain_info makes it explicit that this is info of last (i.e leaf) certificate with the metric name.

I think we can proceed with one of the following way:

  1. keep the new metric, and update the PR to add details of all certificates in the chain.
  2. rename the new metric to include last in the metric name, and only add last cert. info.
  3. skip this new metric, and add the labels into probe_ssl_last_chain_info metric.

I am fine with any of the above solution.

@SuperQ @roidelapluie @mem what do you folks think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt option 1 would need a pretty significant rewrite of the logic to iterate through all the certificates.
Option 2 might cause confusion for end users between the metric names so I opted to try option 3 and moved these labels into the probe_ssl_last_chain_info metric.
I also expanded the subject and issuer labels to return all their respective information, not just the CN of both.
Example

$ curl -s 'http://172.17.0.2:9115/probe?target=twitter.com' | grep probe_ssl_last_chain_info
# HELP probe_ssl_last_chain_info Contains SSL leaf certificate information
# TYPE probe_ssl_last_chain_info gauge
probe_ssl_last_chain_info{fingerprint_sha256="1d0020d1d0b632e78625219e9e2fdcf3c3a0f641240c4ecc46768cd0644884e3",issuer="CN=DigiCert TLS RSA SHA256 2020 CA1,O=DigiCert Inc,C=US",subject="CN=twitter.com,O=Twitter\\, Inc.,L=San Francisco,ST=California,C=US",subjectalternative="twitter.com,www.twitter.com"} 1

Copy link
Member

@electron0zero electron0zero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍

@wwalters12
Copy link

@electron0zero @djcode Is there anything stopping this from being merged in? Would be great to see this feature get released!

@electron0zero electron0zero changed the title Added probe_tls_certificate_info metric with basic certificate details Added leaf certificate details in probe_tls_certificate_info metric Aug 9, 2022
@electron0zero electron0zero changed the title Added leaf certificate details in probe_tls_certificate_info metric Add leaf certificate details in probe_tls_certificate_info metric Aug 9, 2022
@electron0zero electron0zero merged commit cc533f1 into prometheus:master Aug 9, 2022
@electron0zero
Copy link
Member

@electron0zero @djcode Is there anything stopping this from being merged in? Would be great to see this feature get released!

merged, @djcode thanks for the PR 🎉

@latere-a-latere latere-a-latere mentioned this pull request Oct 5, 2022
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

Successfully merging this pull request may close these issues.

3 participants