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 a way to export labels with content matched by the probe #1284

Merged
merged 7 commits into from
Sep 1, 2024

Conversation

lapo-luchini
Copy link
Contributor

I wanted to create an exporter to check if all my hosts had an updated sshd (due to recent CVEs) then I resorted to using blackbock exporter by matching the "fixed" version and the timeout:

  ssh_freebsd_secure:
    prober: tcp
    timeout: 5s
    tcp:
      query_response:
      - expect: "^SSH-2.0-OpenSSH_[0-9.]+ FreeBSD-20240806$"

but then again I'd prefer to capture the value as a label and do my checks by coloring the value in Grafana table later on or in alertmanager rules so I did this change instead.

I tried to do it in a generic way so that it can be used also for very different cases when matching part of the protocol chat is useful.

This produces scraping values like these:

# HELP probe_content Explicit content matched
# TYPE probe_content gauge
probe_content{ssh_comments="FreeBSD-20240806",ssh_version="OpenSSH_9.7"} 1

BTW: I know no Go, except a advent-of-code example, these are the first Go lines that I wrote.

Feel free to close if uninteresting for the project (though I hope it isn't, to avoid the need to always compile my branch) or suggest what I should change in the PR to have it accepted. Thanks!

Produces scraping values like these:

```
probe_content{ssh_comments="FreeBSD-20240806",ssh_version="OpenSSH_9.7"} 1
```

Signed-off-by: Lapo Luchini <lapo@lapo.it>
@lapo-luchini
Copy link
Contributor Author

I wonder if calling prometheus.NewGaugeVec inside the loop is fine, normally in a language that I know I would do it only once during configuration parsing, but I'm not sure how to do that in Go.
Doesn't seems to produce problems anyways.

@electron0zero
Copy link
Member

Hey, this opens up blackbox_exporter to export data out from the check results, and that feel like something that we should not be doing because it can be used to exfiltrate data using blackbox_exporter.

I share the same concern that @SuperQ mentioned here

This use-case feel like more like something that the should be solved by the software itself by exporting the version label in build_info metric. or with https://github.com/prometheus/node_exporter processes or Textfile Collector of the node_exporter. I also recommend look into something like osquery for this use-case.

@lapo-luchini
Copy link
Contributor Author

Those are valid concerns. On the other hand, an attacker would need local access to modify the yaml file to exfiltrate anything but what was "explicitly matched" by the template, an attacker with only network access to blackbox_exporter couldn't do much (but it could use remote targets to query server outside the expected domain, that's true).

A node_exporter textfile collector would be another solution, right.
(but it would require a new software to be installed on each and every host, not only on one centralized host)

I didn't know of osquery, looks interesting, thanks!

I'll close this PR for now, maybe I'll write a "security-oriented" more all-around exporter from scratch.

@SuperQ
Copy link
Member

SuperQ commented Aug 22, 2024

I'm not too concerned about the data exposure here. Like @lapo-luchini says, the exposure values would be up to the admin's regexp.

The concern #1098 and similar are the ability to modify the behavior of the probe from the URL params.

Since this proposal is entirely contained in the config file, it's safe enough. I think this is actually a reasonably good idea.

@lapo-luchini lapo-luchini reopened this Aug 22, 2024
@electron0zero
Copy link
Member

exposure values would be up to the admin's regexp.

ah I see, I think then it's a non- issue.

(but it would require a new software to be installed on each and every host, not only on one centralized host)

that's true, I was under the assumption that node_exporter is already running to monitor these nodes. which might not be the case for everyone so my assumption doesn't hold true. :)

let me find some time to give it a through review :)

@lapo-luchini
Copy link
Contributor Author

that's true, I was under the assumption that node_exporter is already running to monitor these nodes. which might not be the case for everyone so my assumption doesn't hold true. :)

It is my case too, but it does not export the running ssh version, so it would need "something else" to add that to a textfile (to the bare minimum, a shell script in cron).

Anyways… that's just my use-case, returning to the feature proposal of this PR (which tries to be useful to use-cases different than mine too) I'll wait for a review and make adjustments if needed. Thanks!

prober/tcp.go Outdated Show resolved Hide resolved
```
# HELP probe_expect_info Explicit content matched
# TYPE probe_expect_info gauge
probe_expect_info{ssh_comments="FreeBSD-20240806",ssh_version="OpenSSH_9.7"} 1
```

Signed-off-by: Lapo Luchini <lapo@lapo.it>
Signed-off-by: Lapo Luchini <lapo@lapo.it>
@lapo-luchini
Copy link
Contributor Author

I renamed the metric as requested, and also added some documentation.

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.

needs some tests, and then we are good to merge.

Signed-off-by: Lapo Luchini <lapo@lapo.it>
@lapo-luchini
Copy link
Contributor Author

Will do tests tomorrow.

Signed-off-by: Lapo Luchini <lapo@lapo.it>
This involves an unnecessary copy, but I didn't find the proper way to avoid it.

Signed-off-by: Lapo Luchini <lapo@lapo.it>
Signed-off-by: Lapo Luchini <lapo@lapo.it>
@lapo-luchini
Copy link
Contributor Author

OK, should be good for review!

@electron0zero electron0zero merged commit cbfad77 into prometheus:master Sep 1, 2024
5 checks passed
@lapo-luchini lapo-luchini deleted the export_match branch September 1, 2024 16:41
@lapo-luchini lapo-luchini restored the export_match branch September 1, 2024 16:47
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