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

Feat/remote file #144

Merged
merged 8 commits into from
Apr 28, 2024
Merged

Feat/remote file #144

merged 8 commits into from
Apr 28, 2024

Conversation

jaroug
Copy link
Contributor

@jaroug jaroug commented Aug 2, 2023

Add support for probing a remote PEM file over http/https

This is a corner case to be able to monitor BIMI related certificates

@dragoangel
Copy link
Contributor

Looks nice

Copy link
Owner

@ribbybibby ribbybibby left a comment

Choose a reason for hiding this comment

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

Thank you for this PR and sorry for taking so long to review it!

I have a few comments

  1. I don't think this should export ssl_file_cert_* metrics. We should just read the certificates out of the response body and export the regular ssl_cert_* metrics. We don't need the intermediate step where we write them to disk.
  2. This also exports metrics from the TLS connection because it reuses the tls config used in the https prober:
    return collectConnectionStateMetrics(state, registry)
    . I don't think it should do this. It should only export metrics for the certificates returned in the response body.
  3. Can you please improve the tests so that they actually verify the exported metrics?

@jaroug
Copy link
Contributor Author

jaroug commented Jan 7, 2024

Hi :) no worries I'm glad the project is still alive.

I made the changes requested, at least I hope it matches your expectations. I honestly can't remember why I thought it was a good idea to download the file 😅

@jaroug jaroug requested a review from ribbybibby January 7, 2024 16:12
@dragoangel
Copy link
Contributor

LGTM

@dragoangel
Copy link
Contributor

@ribbybibby can you please review if this PR now is okay?

prober/remote_file.go Outdated Show resolved Hide resolved
prober/remote_file.go Outdated Show resolved Hide resolved
prober/remote_file.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Owner

@ribbybibby ribbybibby left a comment

Choose a reason for hiding this comment

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

Changes suggested/requested in comments.

You can fetch remote content with a lot of different protocols, so I
think it's worth being specific here.

As part of this change I've fixed up some of the logic in the code. I've
also created a separate `http_file` block in the module config.
@ribbybibby
Copy link
Owner

Thank you for the contribution @jaroug. I've made some of my suggested changes and fixed some other things up and now I think this is good to merge.

@ribbybibby ribbybibby merged commit 515b990 into ribbybibby:master Apr 28, 2024
2 checks passed
@dragoangel
Copy link
Contributor

@ribbybibby can we get 2.5.0 released with this feature please? 🙏

@dragoangel
Copy link
Contributor

@ribbybibby kind reminder - this feature not get into 2.4.3 release, as it was been released before merging this branch.

@dragoangel
Copy link
Contributor

dragoangel commented Aug 22, 2024

@jaroug btw, I found out that ssl_verified_cert_not_after not available as metrics for http_file, is this desired behavior?

I get managed to create alert for ssl_cert_not_after only if ssl_verified_cert_not_after not exist for instance, but it was a bit challenging:

    (
      (
        ssl_cert_not_after{namespace="external-ssl-monitor"}
        - time()
      ) < 86400 * 14
    )
    unless
    sum without (chain_no) (
      ssl_verified_cert_not_after{chain_no="0",namespace="external-ssl-monitor"}
    )

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