Skip to content

Conversation

@SuperQ
Copy link
Contributor

@SuperQ SuperQ commented Dec 31, 2024

Add a basic Prometheus Monitoring metrics endpoint.

  • Include a jetkvm_build_info metric.
  • go mod tidy

@SuperQ SuperQ force-pushed the metrics_endpoint branch 5 times, most recently from bdbe2e7 to 9b3a3d0 Compare January 3, 2025 10:42
@Nevexo
Copy link
Contributor

Nevexo commented Jan 3, 2025

Very nice, would be cool to see the KVM's connection state (maybe even USB/HDMI state?) in the metrics too.

@SuperQ
Copy link
Contributor Author

SuperQ commented Jan 3, 2025

Yup, I plan to add lots more once the basic concept is approved.

@SuperQ SuperQ changed the title Feature: Add a metrics endpoint feat: Add a metrics endpoint Jan 6, 2025
@kashalls
Copy link

@SuperQ Did you verify that the route works? I'm running Nev's next-6 build and the /metrics endpoint returns a 404 from the dashboard.
image

@SuperQ
Copy link
Contributor Author

SuperQ commented Jan 30, 2025

@Nevexo confirmed it worked a while back.

@CLAassistant
Copy link

CLAassistant commented Feb 13, 2025

CLA assistant check
All committers have signed the CLA.

Add a basic Prometheus Monitoring metrics endpoint.
* Include a `jetkvm_build_info` metric.
* `go mod tidy`

Signed-off-by: SuperQ <[email protected]>
@ym
Copy link
Contributor

ym commented Feb 25, 2025

Thank you for your contribution! However, the /metrics endpoint may expose sensitive metrics, and without proper protection, it could pose a security risk.

I’ll go ahead and approve this PR and merge it, but I may add additional code to ensure that the /metrics endpoint is not enabled by default to mitigate any potential security concerns. I guess soon we'll need a super complicated Web UI to manage such things :-D

@ym ym merged commit ba0c937 into jetkvm:dev Feb 25, 2025
1 check passed
@SuperQ
Copy link
Contributor Author

SuperQ commented Feb 25, 2025

There should be no sensitive information on the metrics endpoint. There should be no security concerns here.

@vladak
Copy link

vladak commented Feb 27, 2025

There should be no sensitive information on the metrics endpoint. There should be no security concerns here.

If the metrics exposes e.g. version information about the KVM firmware (which I am not sure it currently does, however it can in the future, along with other data) then this could be sensitive information if there is a security vulnerability.

All in all, I'd rather have a device that has as few services enabled by default as possible.

@kashalls
Copy link

This is definitely something that should be opt-in and by default should only respond to RFC1918 hosts.

@Nevexo
Copy link
Contributor

Nevexo commented Feb 28, 2025

I do think it should be a toggle-able future, especially now we have a nicer settings UI.

However, I generally disagree with this being a security issue, you'd expect an attempt to exploit a vulnerability would just exploit it, rather than wasting time checking the version number. A lot of software reveals it's version number day-to-day.

Thinking it needs to be hidden is very much security through obscurity

This is the kind of device that won't often find itself routable from the internet, however,

by default should only respond to RFC1918 hosts.

I don't think this is a good approach, I suppose it could offer a "allow from these CIDRs" box, but you should be protecting this device from the network side (...a firewall) anyway.

Blindly blocking RFC1918 assumes the device is behind NAT, and doesn't offer security against IPv6 GUAs anyway.

In general I'm quite fond of this feature, it'll be nice once it gets some more metrics, but I do fully agree that it should be toggle-able in the future.

Oh and since this is just an endpoint on the device, there's no extra services. I suppose there's another library or two that does have to be considered for supply-chain type attacks.

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.

7 participants