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

Add decryption authorization for clevis clients #92

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dsommers
Copy link

@dsommers dsommers commented Jun 30, 2022

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

Codecov Report

Merging #92 (ca82648) into master (e2059ee) will decrease coverage by 1.74%.
The diff coverage is 38.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%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2059ee...ca82648. Read the comment docs.

doc/tang.8.adoc Outdated

By default, tang will respond to any recovery requests. This can be
controlled on a per client key by adding a second argument to the *tang*
command line. This argument need to be a path to an existing directory.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This argument needs ...

doc/tang.8.adoc Outdated

# touch /var/db/tang/authorized/EyIEfKd-_3UFMI5PSAp64UAAKeQ

6. Decrypting this will now works:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will now work

src/tangd.c Show resolved Hide resolved
doc/tang.8.adoc Outdated
directory.

ifdef::freebsd[]
# TODO
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will need some help here, how to best enable this on FreeBSD.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FreeBSD equivalent is really not much different (see units/tangd.rc.in where the tangd config is built from):

Alter the command_args line in the tangd file in the rc.d directory to be something like:
command_args="${_tangd_listen_args} SYSTEM:"${tangd_executable} ${tangd_jwkdir} /var/db/tang/authorized 2>> ${tangd_logfile} " &"
and possibly add that directory to the required dirs line in the same file, i.e.,
required_dirs="${tangd_jwkdir} /var/db/tang/authorized"

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.

Signed-off-by: David Sommerseth <[email protected]>
To enable the client authorization directory before this change, the
main systemd service unit file needed to be modified.  This is less
ideal and we can use EnvironmentFile= feature in the unit file to read a
file with environmental variables the [email protected] will use for the
command line.

Signed-off-by: David Sommerseth <[email protected]>
@dsommers
Copy link
Author

dsommers commented Aug 8, 2022

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.

@dsommers dsommers marked this pull request as ready for review August 8, 2022 11:16
@dsommers dsommers changed the title WIP: Add decryption authorization Add decryption authorization for clevis clients Aug 8, 2022
@dsommers
Copy link
Author

ping @sarroutbi

Any chance to get this pull request reviewed?

@sarroutbi
Copy link
Collaborator

sarroutbi commented Sep 14, 2022

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.

@krzee
Copy link

krzee commented Nov 21, 2022

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!

@sarroutbi
Copy link
Collaborator

@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.

@dsommers
Copy link
Author

dsommers commented Nov 22, 2022

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

sarroutbi commented Jun 19, 2023

Hello @dsommers : are you trying to resolve the conflicting files?

@dsommers
Copy link
Author

@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.

@sarroutbi
Copy link
Collaborator

Hello @dsommers . Any update on this? Are you planning to resolve conflicts?

@dsommers
Copy link
Author

Ouch! Sorry about dropping the ball here. Looking into this now.

@dsommers
Copy link
Author

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.

@sarroutbi
Copy link
Collaborator

sarroutbi commented Feb 15, 2024

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.

@dsommers
Copy link
Author

@sarroutbi Thanks for the feedback. 👍

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.

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

Successfully merging this pull request may close these issues.

5 participants