tang
tang copied to clipboard
latchset
Add decryption authorization for clevis clients
The default behaviour of tangd is to reply correctly to all key recovery requests. In some deployments it may be needed to control when a key recovery should be allowed or not. This patch extends the tangd server with a very simple authorization method.
When tangd is started with a second argument, this need to point at a directory to be used for authorizations. When a key recovery request occurs, the tangd server will check if this directory contains the filename of the client key fingerprint (thp/kid). If a file (which can be empty) exists with the name of a client fingerprint, the request is authorized and the decryption can be performed successfully.
This touches one of the challenges mentioned in issue #20. It doesn't fully solve that issue, as it provides no tooling to alert for requests or to tell the client to "come back later". But it provides a first step to control when a decryption can be allowed.
Codecov Report
Merging #92 (ca82648) into master (e2059ee) will decrease coverage by
1.74%. The diff coverage is38.88%.
@@ Coverage Diff @@
## master #92 +/- ##
==========================================
- Coverage 57.35% 55.60% -1.75%
==========================================
Files 3 3
Lines 401 419 +18
Branches 114 120 +6
==========================================
+ Hits 230 233 +3
- Misses 88 100 +12
- Partials 83 86 +3
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/tangd.c | 44.62% <38.88%> (-4.89%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update e2059ee...ca82648. Read the comment docs.
I've updated this WIP pull-req with the suggested changes. I've also added an additional commit which enables using a configuration file with the sytemd unit file, instead of modifying the unit file directly.
Hello @dsommers. Thanks for your PR. I reviewed your change to help on fixing some typos and related stuff, but I don't have enough technical background regarding tang for this PR to be approved.
I am not sure about this kind of authorization to be something that should be kept inside tang code, or if this should be kept out of tang by leveraging some of the other multiple solutions that are available, being tang an HTTP server.
I will check with other maintainer and will come back to you.
this is exactly what i want! this would allow us to build scripts around placing the file and making our own rules/apps to manage what can decrypt!
@sergio-correia : what's your opinion on this? IMHO, this functionality could be kept out of tang, by selecting other mechanisms that do not affect Tang's code, such as nginx or similar. However, I understand that for some scenarios this kind of filtering could help to avoid an extra message hop.
While the reverse proxy solution could work in many cases, it would for some use cases require something else than nginx; or something in between nginx and tang.
For use cases where you want to allow a key recovery call for only a selected host within a limited time window, the pure nginx approach would mean to update the nginx config, add/remove the appropriate locaiton {} block and reload the configuration. In some environments it would not even be considered secure enough to only filter/limit access based on IP address alone (the host doing the REST call could be behind a NAT gateway). I am not aware of any reverse proxies capable on doing that without requiring configuration file update to enable/disable specific callers..
This would then speak for another reverse proxy in front of tang, which can inspect the REST URL before deciding to reject the request or pass it on to tang. While this would work, it would be a more complicated overall setup, with more parts in the chain having a possibility to fail.
Having this simple check inside tang instead gives a simpler overall setup and can work just as efficient. The check itself is very simple (provided in the new check_rec_authorization() function), which is optionally called (based on the runtime configuration) in src/tang.c:150. The rest of the changes to tangd.c is essentially boilerplate changes, to make this runtime configurable and to reuse some of the already existing code with these related changes. If it would help accepting this change, I can probably split out the boilerplate changes to a separate commit.
The rest of the changes is to make this easily configurable via a "configuration file" which is parsed by systemd when starting tangd, introduction of this new configuration file (meson.build, systemd unit changes + a new file) and finally documentation. So this change set shouldn't be too intrusive or breaking things easily.
@sarroutbi Sorry for the late response. Yes I will look at this. Pretty full plate before the holidays, so I'll probably follow up in august.
Hello @dsommers . Any update on this? Are you planning to resolve conflicts?
Okay, so I've spent some time looking into this. A lot of stuff has happened in the master branch, so I would prefer to re-implement it based on master instead of just resolving the conflict. The needed changes would be better to be split up into more commits, to have a more gradual set of changes.
This will take a bit more time than what I have available today. But this will get some attention a bit later.
That said ... how likely is it that this feature will be accepted?
If I am going to spend time getting this ready I need to know that this feature will be included in the end. It does not mean it must be implemented exactly like this pull-request suggests, we can work on adopting it to the projects interests and goals. But if there is a little chance this feature will be accepted, it's better to just close this pull-request and not waste any more of our time on it.
Okay, so I've spent some time looking into this. A lot of stuff has happened in the master branch, so I would prefer to re-implement it based on master instead of just resolving the conflict. The needed changes would be better to be split up into more commits, to have a more gradual set of changes.
This will take a bit more time than what I have available today. But this will get some attention a bit later.
That said ... how likely is it that this feature will be accepted?
We are asking conflict resolution because we would like to merge this feature. It seems useful to other users of the clevis suite, as it can be observed in the conversation.
If I am going to spend time getting this ready I need to know that this feature will be included in the end. It does not mean it must be implemented exactly like this pull-request suggests, we can work on adopting it to the projects interests and goals. But if there is a little chance this feature will be accepted, it's better to just close this pull-request and not waste any more of our time on it.
We try to be cautious when merging to tang, as it is in, somehow, an stable state. For that reason, we like to take the patches and test them before accepting them. We also like unit tests to be added, documentation to be updated, and so on. Sorry if it takes time for the changes to be reviewed, but we prefer advancing in a slow but sure pace.
@sarroutbi Thanks for the feedback. :+1:
I'll put this into my todo list then, and get it rebased. I'll also look into extending the existing test suite to test this feature and all the related changes needed.