letsencrypt / unbound_exporter

A Prometheus exporter for Unbound.
Apache License 2.0
179 stars 80 forks source link

Panic occurs when certificates do not exist on disk #44

Open pgporada opened 2 years ago

pgporada commented 2 years ago

Note that I am missing the -unbound.ca flag which then defaults to a non-existant file /etc/unbound/unbound_server.pem. We should detect file existence and not panic.

Systemd unit

[Unit]
Description=Prometheus exporter for Unbound metrics, written in Go with pluggable metric collectors. The metrics exporter converts Unbound metric names to Prometheus metric names and labels by using a set of regular expressions.
Documentation=https://github.com/letsencrypt/unbound_exporter
After=network.target

[Service]
Type=simple
ExecStart=/opt/prometheus/bin/unbound_exporter \
    -unbound.cert "/etc/unbound/unbound_control_ec.pem" \
    -unbound.key "/etc/unbound/unbound_control_ec.key" \
    -unbound.host "tcp://localhost:8953"

[Install]
WantedBy=multi-user.target

Logs

unbound_exporter[5031]: level=info ts=2022-08-09T21:18:30.787Z caller=unbound_exporter.go:492 Startingunbound_exporter=(MISSING)
unbound_exporter[5031]: panic: open /etc/unbound/unbound_server.pem: no such file or directory
unbound_exporter[5031]: goroutine 1 [running]:
unbound_exporter[5031]: main.main()
unbound_exporter[5031]:         /tmp/tmp.pHIIcqYEmU/unbound_exporter.go:495 +0x679
pgporada commented 2 years ago

There's also a panic if you set -unbound.ca ""

unbound_exporter[5031]: level=info ts=2022-08-09T21:22:43.240Z caller=unbound_exporter.go:492 Startingunbound_exporter=(MISSING)
unbound_exporter[5031]: panic: open : no such file or directory
unbound_exporter[5031]: goroutine 1 [running]:
unbound_exporter[5031]: main.main()
unbound_exporter[5031]:         /tmp/tmp.pHIIcqYEmU/unbound_exporter.go:495 +0x679
jsha commented 2 years ago

To be clear, we should do this by handling this subcategory of error using https://pkg.go.dev/os#IsNotExist (i.e. not by statting the file first to see if it exists).

We should also have a separate useful error for the empty string.

chr0mag commented 1 year ago

There's also a panic if you set -unbound.ca ""

unbound_exporter[5031]: level=info ts=2022-08-09T21:22:43.240Z caller=unbound_exporter.go:492 Startingunbound_exporter=(MISSING)
unbound_exporter[5031]: panic: open : no such file or directory
unbound_exporter[5031]: goroutine 1 [running]:
unbound_exporter[5031]: main.main()
unbound_exporter[5031]:         /tmp/tmp.pHIIcqYEmU/unbound_exporter.go:495 +0x679

@pgporada - FWIW setting -unbound.ca "" works if you also set -unbound.cert "" . In this case you get non-TLS to the unbound-control socket.

To be clear, we should do this by handling this subcategory of error using https://pkg.go.dev/os#IsNotExist (i.e. not by statting the file first to see if it exists).

We should also have a separate useful error for the empty string.

@jsha - Perhaps a better way of handling this is simply to not set the ca, cert and key values by default? Changing defaults can break things for sure, but if this exporter is expected to be run on Prometheus targets (ie. hosts running Unbound) then communication between the exporter and unbound-control is local to the host anyway.

If I'm interpreting the docs correctly TLS is ignored even when control-use-cert is set to yes in the remote-control section of unbound.conf, when unbound-control is listening on a local socket. Since listening on a local socket is assumed (-unbound.host defaults to tcp://localhost:8953) the default setup ends up doing this:

unbound-exporter (9167) -------TLS------> unbound-control (8953) ------->no-TLS------> unbound server (53)

...which is a bit odd.

jsha commented 1 year ago

If I'm interpreting the docs correctly TLS is ignored even when control-use-cert is set to yes in the remote-control section of unbound.conf, when unbound-control is listening on a local socket. Since listening on a local socket is assumed (-unbound.host defaults to tcp://localhost:8953) the default setup ends up doing this:

The "socket" terminology is a little confusing because there are both TCP sockets and Unix domain sockets. I believe the docs are saying "If you are using TCP sockets, the TLS options are relevant. If you are using Unix domain sockets, the TLS options are ignored."

I have a slight preference here for being opinionated, and supporting only the mode of operation that uses Unix domain sockets. It is simpler to configure and (broadly speaking) more secure. That would allow us to remove the certificate options entirely from unbound-exporter.

chr0mag commented 1 year ago

The "socket" terminology is a little confusing because there are both TCP sockets and Unix domain sockets. I believe the docs are saying "If you are using TCP sockets, the TLS options are relevant. If you are using Unix domain sockets, the TLS options are ignored."

I finally got around to testing this and you are correct. control-user-cert is only ignored if UNIX domain sockets are used. I have a slight preference here for being opinionated, and supporting only the mode of operation that uses Unix domain sockets. It is simpler to configure and (broadly speaking) more secure. That would allow us to remove the certificate options entirely from unbound-exporter.

Agreed. TLS seems more valuable inbound to the exporter anyway ( #50 ) where the connection is much less likely to be local to the host.