Add mTLS support for Loki monitoring hook #1293
No reviewers
Labels
No labels
blocked
breaking
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
borgmatic-collective/borgmatic!1293
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "maxhamon/borgmatic:loki-mtls"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
4ae7642c52b8cf5cbcc3Thanks 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 hastlsoptions setLoki_log_handler.__init__()when config hastlsoptions setLoki_log_handler.__init__()when config has only onetlsoption 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):Preference to call these
*_filename(or*_path) instead of*_file. That's because in Python, something named*_fileoften 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.@ -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,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?
It was crashing, fix by checking both are set and legging a message.
@ -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):It would be good to update the docstring below based on these new arguments.
done
@ -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 {}The
or {}might be unnecessary...? The only way I could see it being needed is if someone doestls: nullin config, which the schema might not actually allow sincenullisn't a YAML/JSONobject. 🤷useless, remove
@ -92,0 +94,4 @@### Mutual TLS authentication<span class="minilink minilink-addedin">New in version **TBD**</span> If yourLoki instance is configured for mTLS authentication, you can provide a clientWorth mentioning that this would have to be provided by some reverse proxy or whatever that wraps Loki, since Loki doesn't include this functionality?
done
@ -92,0 +109,4 @@key_file: /etc/borgmatic/loki-client.key```Both `cert_file` and `key_file` must be PEM-encoded. They are passed directlyI 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.
done
@ -92,0 +111,4 @@Both `cert_file` and `key_file` must be PEM-encoded. They are passed directlyto the underlying HTTP client, so the standard mutual TLS handshake isperformed for every request borgmatic sends to Loki.Great docs! Thanks for adding them.
Add mTLS support for Loki monitoring hookto WIP: Add mTLS support for Loki monitoring hook0cd48da74d0931f0928eI have :
0931f0928eaa5895eca3WIP: Add mTLS support for Loki monitoring hookto Add mTLS support for Loki monitoring hook@ -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')):Not sure about how to handle this
This seems like a reasonable way to do it given that they're strings in the schema (and not, say, boolean values).
Thanks for checking
aa5895eca3edfa708fa3I'm good for another review
__init__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')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.
@ -81,0 +124,4 @@}with pytest.raises(ValueError):module.initialize_monitor(hook_config, {}, 'test.yaml', 1, False)Nice tests!!