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

Panic occurs when certificates do not exist on disk #44

Open
pgporada opened this issue Aug 9, 2022 · 5 comments
Open

Panic occurs when certificates do not exist on disk #44

pgporada opened this issue Aug 9, 2022 · 5 comments

Comments

@pgporada
Copy link
Member

pgporada commented Aug 9, 2022

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
Copy link
Member Author

pgporada commented Aug 9, 2022

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
Copy link
Collaborator

jsha commented Aug 22, 2022

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
Copy link

chr0mag commented Dec 27, 2022

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
Copy link
Collaborator

jsha commented Jan 4, 2023

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
Copy link

chr0mag commented Mar 11, 2023

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.

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

No branches or pull requests

3 participants