Add mTLS support for Loki monitoring hook #1293

Merged
witten merged 1 commit from maxhamon/borgmatic:loki-mtls into main 2026-04-13 16:07:57 +00:00
Contributor

Add optional tls.cert_file and tls.key_file configuration options to the Loki hook, allowing borgmatic to authenticate with a Loki instance that requires mutual TLS (mTLS).

Reference to #1289

Add optional tls.cert_file and tls.key_file configuration options to the Loki hook, allowing borgmatic to authenticate with a Loki instance that requires mutual TLS (mTLS). Reference to #1289
Add mTLS support for Loki monitoring hook
Some checks are pending
build / test (pull_request) Blocked by required conditions
build / docs (pull_request) Blocked by required conditions
2e7a8a1f6b
resolve conflict from main
Some checks are pending
build / test (pull_request) Blocked by required conditions
build / docs (pull_request) Blocked by required conditions
4ae7642c52
maxhamon force-pushed loki-mtls from 4ae7642c52
Some checks are pending
build / test (pull_request) Blocked by required conditions
build / docs (pull_request) Blocked by required conditions
to b8cf5cbcc3
Some checks failed
build / test (pull_request) Failing after 3m4s
build / docs (pull_request) Has been skipped
2026-04-07 19:35:11 +00:00
Compare
witten left a comment

Thanks for doing this in PR form! I left a few other comments on pretty minor stuff.

I think the main thing missing for me though is any automated test changes. I don't think those have to be extensive, since this is a pretty non-invasive PR without a bunch of logic of its own. Besides getting the existing tests passing, here are a few additional tests I can think of adding:

  • initialize_monitor() when config has tls options set
  • Loki_log_handler.__init__() when config has tls options set
  • Maybe also Loki_log_handler.__init__() when config has only one tls option set—but only if the expected behavior is any different from when there are two.

Let me know if you run into any problems adding these or running tests.

Thanks for doing this in PR form! I left a few other comments on pretty minor stuff. I think the main thing missing for me though is any automated test changes. I don't think those have to be extensive, since this is a pretty non-invasive PR without a bunch of logic of its own. Besides getting the existing tests passing, here are a few additional tests I can think of adding: * `initialize_monitor()` when config has `tls` options set * `Loki_log_handler.__init__()` when config has `tls` options set * Maybe also `Loki_log_handler.__init__()` when config has only one `tls` option set—but only if the expected behavior is any different from when there are two. Let me know if you run into any problems adding these or running tests.
@ -28,3 +28,3 @@
'''
def __init__(self, url, dry_run):
def __init__(self, url, dry_run, tls_cert_file=None, tls_key_file=None):
Owner

Preference to call these *_filename (or *_path) instead of *_file. That's because in Python, something named *_file often refers to a file-like object instead of a path.

Do not feel strongly.

I think the option names as defined in the schema are fine as is, since there's no risk of confusion there.

It might also be nice to add a docstring describing the expected arguments for this __init__() function, since the number of them is growing.

Preference to call these `*_filename` (or `*_path`) instead of `*_file`. That's because in Python, something named `*_file` often refers to a file-like object instead of a path. Do not feel strongly. I think the option names as defined in the schema are fine as is, since there's no risk of confusion there. It might also be nice to add a docstring describing the expected arguments for this `__init__()` function, since the number of them is growing.
maxhamon marked this conversation as resolved
@ -77,6 +79,7 @@ class Loki_log_buffer:
'Content-Type': 'application/json',
'User-Agent': 'borgmatic',
},
cert=(self.tls_cert_file, self.tls_key_file) if self.tls_cert_file else None,
Owner

Since both of these options are optional in the schema, what happens if the cert file is set with a value but the key file isn't? Does this code error cleanly or does it explode?

Since both of these options are optional in the schema, what happens if the cert file is set with a value but the key file isn't? Does this code error cleanly or does it explode?
Author
Contributor

It was crashing, fix by checking both are set and legging a message.

It was crashing, fix by checking both are set and legging a message.
maxhamon marked this conversation as resolved
@ -89,3 +92,3 @@
'''
def __init__(self, url, send_logs, log_level, dry_run):
def __init__(self, url, send_logs, dry_run, tls_cert_file=None, tls_key_file=None):
Owner

It would be good to update the docstring below based on these new arguments.

It would be good to update the docstring below based on these new arguments.
Author
Contributor

done

done
maxhamon marked this conversation as resolved
@ -140,3 +143,3 @@
'''
url = hook_config.get('url')
loki = Loki_log_handler(url, hook_config.get('send_logs', False), monitoring_log_level, dry_run)
tls = hook_config.get('tls', {}) or {}
Owner

The or {} might be unnecessary...? The only way I could see it being needed is if someone does tls: null in config, which the schema might not actually allow since null isn't a YAML/JSON object. 🤷

The `or {}` might be unnecessary...? The only way I could see it being needed is if someone does `tls: null` in config, which the schema might not actually allow since `null` isn't a YAML/JSON `object`. 🤷
Author
Contributor

useless, remove

useless, remove
maxhamon marked this conversation as resolved
@ -92,0 +94,4 @@
### Mutual TLS authentication
<span class="minilink minilink-addedin">New in version **TBD**</span> If your
Loki instance is configured for mTLS authentication, you can provide a client
Owner

Worth mentioning that this would have to be provided by some reverse proxy or whatever that wraps Loki, since Loki doesn't include this functionality?

Worth mentioning that this would have to be provided by some reverse proxy or whatever that wraps Loki, since Loki doesn't include this functionality?
Author
Contributor

done

done
maxhamon marked this conversation as resolved
@ -92,0 +109,4 @@
key_file: /etc/borgmatic/loki-client.key
```
Both `cert_file` and `key_file` must be PEM-encoded. They are passed directly
Owner

I think this is probably sufficient as is, but it might be nice to link to any additional third-party information about what PEM-encoded is, how to generate such cert/key files, etc. Just so a user who's interested in adding mTLS would have a place to start reading up on how to get started with the certs.

I think this is probably sufficient as is, but it might be nice to link to any additional third-party information about what PEM-encoded is, how to generate such cert/key files, etc. Just so a user who's interested in adding mTLS would have a place to start reading up on how to get started with the certs.
Author
Contributor

done

done
maxhamon marked this conversation as resolved
@ -92,0 +111,4 @@
Both `cert_file` and `key_file` must be PEM-encoded. They are passed directly
to the underlying HTTP client, so the standard mutual TLS handshake is
performed for every request borgmatic sends to Loki.
Owner

Great docs! Thanks for adding them.

Great docs! Thanks for adding them.
maxhamon marked this conversation as resolved
fix: rename *_file to *_path
Some checks failed
build / test (pull_request) Failing after 1m59s
build / docs (pull_request) Has been skipped
a6ac52adb6
fix: remove useless {}
Some checks failed
build / test (pull_request) Failing after 1m57s
build / docs (pull_request) Has been skipped
8fe7921b02
maxhamon changed title from Add mTLS support for Loki monitoring hook to WIP: Add mTLS support for Loki monitoring hook 2026-04-10 18:11:03 +00:00
add tests
All checks were successful
build / test (pull_request) Successful in 7m21s
build / docs (pull_request) Has been skipped
7d421ff84d
update docs
All checks were successful
build / test (pull_request) Successful in 7m17s
build / docs (pull_request) Has been skipped
0cd48da74d
maxhamon force-pushed loki-mtls from 0cd48da74d
All checks were successful
build / test (pull_request) Successful in 7m17s
build / docs (pull_request) Has been skipped
to 0931f0928e
All checks were successful
build / test (pull_request) Successful in 7m20s
build / docs (pull_request) Has been skipped
2026-04-10 18:47:30 +00:00
Compare
Author
Contributor

I have :

  • Added some unit tests
  • Update docs with some links to nginx and Traefik documentation, that specify how to handle mTLS, generate certificate, etc.
I have : - Added some unit tests - Update docs with some links to nginx and Traefik documentation, that specify how to handle mTLS, generate certificate, etc.
maxhamon force-pushed loki-mtls from 0931f0928e
All checks were successful
build / test (pull_request) Successful in 7m20s
build / docs (pull_request) Has been skipped
to aa5895eca3
All checks were successful
build / test (pull_request) Successful in 7m18s
build / docs (pull_request) Has been skipped
2026-04-11 06:26:02 +00:00
Compare
maxhamon changed title from WIP: Add mTLS support for Loki monitoring hook to Add mTLS support for Loki monitoring hook 2026-04-11 06:27:46 +00:00
@ -142,1 +152,3 @@
loki = Loki_log_handler(url, hook_config.get('send_logs', False), monitoring_log_level, dry_run)
tls = hook_config.get('tls', {})
if bool(tls.get('cert_path')) != bool(tls.get('key_path')):
Author
Contributor

Not sure about how to handle this

Not sure about how to handle this
Owner

This seems like a reasonable way to do it given that they're strings in the schema (and not, say, boolean values).

This seems like a reasonable way to do it given that they're strings in the schema (and not, say, boolean values).
Author
Contributor

Thanks for checking

Thanks for checking
maxhamon marked this conversation as resolved
maxhamon force-pushed loki-mtls from aa5895eca3
All checks were successful
build / test (pull_request) Successful in 7m18s
build / docs (pull_request) Has been skipped
to edfa708fa3
All checks were successful
build / test (pull_request) Successful in 8m40s
build / docs (pull_request) Has been skipped
2026-04-12 15:57:40 +00:00
Compare
Author
Contributor

I'm good for another review

  • Add unit tests.
  • Rename *_file to *_path.
  • Ensure both cert and key are set to avoid crashing without info.
  • Add docstring to __init__
  • Update docs with links to better explain how to implement mTLS on reverse proxy side. Just need to update for version number
I'm good for another review - Add unit tests. - Rename *_file to *_path. - Ensure both cert and key are set to avoid crashing without info. - Add docstring to `__init__` - Update docs with links to better explain how to implement mTLS on reverse proxy side. Just need to update for version number
witten approved these changes 2026-04-13 16:06:59 +00:00
witten left a comment

Looks great! Thanks so much for all your work here.

Looks great! Thanks so much for all your work here.
@ -143,0 +157,4 @@
)
raise ValueError(
'Invalid Loki TLS configuration: cert_path and key_path must both be set or both be unset'
)
Owner

You don't need to both log and raise. Just raising is fine... the right thing will get logged automatically.

In the interests of moving along this PR though, I can make this change after merging.

You don't need to both log and raise. Just raising is fine... the right thing will get logged automatically. In the interests of moving along this PR though, I can make this change after merging.
witten marked this conversation as resolved
@ -81,0 +124,4 @@
}
with pytest.raises(ValueError):
module.initialize_monitor(hook_config, {}, 'test.yaml', 1, False)
Owner

Nice tests!!

Nice tests!!
witten marked this conversation as resolved
witten merged commit 71e2762aa7 into main 2026-04-13 16:07:57 +00:00
maxhamon deleted branch loki-mtls 2026-04-13 16:50:51 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
borgmatic-collective/borgmatic!1293
No description provided.