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

WIP: ssh-agent: Implement destination constraints #10252

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

kgraefe
Copy link
Contributor

@kgraefe kgraefe commented Feb 1, 2024

This change implements loading ssh-agent destination constraints from KeeAgent.settings into the ssh-agent. For now there is no UI so configuration must be done in KeePass2/KeeAgent.

The ssh-agent constrain extension is described at 1. However, I found it partly misleading:

  • in the constaint array each constraint is enveloped where in the keyspec arrays the keyspec are just appended to the constraint.
  • each constraint and host has an additional string field reserved for future use. The actual structure has been obtained from openssh ssh-add source code 2.

TODO:

  • GUI
  • Test against a SSH host with host key signed by a CA
  • Update this PR message

Fixes #9801

Screenshots

Testing strategy

Type of change

  • ✅ New feature (change that adds functionality)

@droidmonkey
Copy link
Member

droidmonkey commented Feb 1, 2024

What does this actually do / why does this matter?

@kgraefe
Copy link
Contributor Author

kgraefe commented Feb 1, 2024

What does this actually do / why does this matter?

It allows restricting access to the SSH agent when forwarded another host.

Let's say I ssh into a development machine on a client side client-dev and I need to forward the agent to access client's Git server client-git. Without the restriction an admin on the dev machine could use my forwarded agent to ssh into my Github account. With the restriction I can configure the SSH agent forwarded to client-dev to only sign requests from client-git and to disallow signing request from everything else.

Currently I'm using multiple SSH agents with different keys per client. But that's cumbersome and of course it's not supported by KeepassXC so it prevents me from keeping my SSH keys in KeepassXC.

see also the specification

@kgraefe kgraefe force-pushed the feature/ssh-agent-destination-contraints branch from 5ec70c3 to 34d0a9b Compare February 8, 2024 22:30
@droidmonkey
Copy link
Member

Does does the ca in "is_ca" refer to Certificate Authority?

@kgraefe
Copy link
Contributor Author

kgraefe commented Feb 9, 2024

Does does the ca in "is_ca" refer to Certificate Authority?

yes. I have not done this myself but apparently instead of relying on long-living host keys that are manually confirmed on first contact, you can use short-living host keys that are signed by a CA. In order to verify the host key you need the CA's public key. So if is_ca == true the keyblob does not contain the host key but the CA key.

@kgraefe kgraefe force-pushed the feature/ssh-agent-destination-contraints branch 3 times, most recently from 4edabff to 58bf74c Compare February 9, 2024 14:11
@kgraefe
Copy link
Contributor Author

kgraefe commented Feb 12, 2024

CI fails because openssh-client in Ubuntu 18.04 is too old and does not support destination constraints (requires OpenSSH 8.9, has 8.2). :-\ In general it is the desired behavior to fail instead of loading the key without the constraints in place.

However, how should I proceed with this? I can think of either

  • omitting the tests or somehow guard them against the SSH client version,
  • updating the Docker container to Ubuntu 22.04 LTS or
  • manually install newer openssh-client into the Docker container.

@kgraefe kgraefe force-pushed the feature/ssh-agent-destination-contraints branch 3 times, most recently from 95f9e59 to 55c6cd2 Compare February 20, 2024 09:24
@phoerious phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024
@pbhenson
Copy link

After years of procrastination I'm finally following up on storing my ssh keys in KeypassXC :), and would definitely like this feature. I was just wondering what the current status is and if there were any thoughts on when it might get merged?

It looks like the last update was an issue where Ubuntu 18.04 had too old a version of openssh, that version of Ubuntu went EOS in June 2023 so it seems upgrading to 22.04 would be the way to go :).

Thanks for working on this new feature. Let me know if I can help in any way.

@kgraefe
Copy link
Contributor Author

kgraefe commented Dec 27, 2024

After years of procrastination I'm finally following up on storing my ssh keys in KeypassXC :), and would definitely like this feature. I was just wondering what the current status is and if there were any thoughts on when it might get merged?

The UI is missing. I don't think it makes sense to merge with no way view or edit the constraints in KeePassXC as you would end up in situation where the key does not work (because it's restricted) but you cannnot see why. Current state is that the implementation is functional but you'd need to open the database in KeePass with KeeAgent to view and edit the restrictions (or edit the XML attachment manually).

It looks like the last update was an issue where Ubuntu 18.04 had too old a version of openssh, that version of Ubuntu went EOS in June 2023 so it seems upgrading to 22.04 would be the way to go :).

The automated tests were running in Ubuntu 18.04. Not sure if that has changed now, but even if not I'd see that out of scope of that particular PR.

Thanks for working on this new feature. Let me know if I can help in any way.

I don't know when I will have time to finish the PR. If you want to take over and build the UI part I can share what I had in mind. (If you do you may want to throw away the WIP commits.)

That being said, I'm still eager to get this into KeepassXC as not having destination constraints prevents me from putting my SSH keys into the database. Instead I'm currently stuck with some hand-crafted SSH wrapper scripts...

@pbhenson
Copy link

The UI is missing. I don't think it makes sense to merge with no way view or edit the constraints in KeePassXC as you would end up in situation where the key does not work (because it's restricted) but you cannnot see why. Current state is that the implementation is functional but you'd need to open the database in KeePass with KeeAgent to view and edit the restrictions (or edit the XML attachment manually).

Hmm; well, the only way you'd end up in that situation is if you put yourself in it ;). I'd rather have the underlying functionality with a "text edit" based UI than not have it at all, but I can understand the position of not wanting to confuse users.

I don't know when I will have time to finish the PR. If you want to take over and build the UI part I can share what I had in mind. (If you do you may want to throw away the WIP commits.)

UI is not my strong suite :(, my day job doesn't let me do that anymore :). I tend towards very simple very functional UI's which don't necessarily click with average users. I would actually be perfectly happy just writing XML for this with no fancy widgets...

Thanks for the update.

@kgraefe
Copy link
Contributor Author

kgraefe commented Dec 29, 2024

The UI is missing. I don't think it makes sense to merge with no way view or edit the constraints in KeePassXC as you would end up in situation where the key does not work (because it's restricted) but you cannnot see why. Current state is that the implementation is functional but you'd need to open the database in KeePass with KeeAgent to view and edit the restrictions (or edit the XML attachment manually).

Hmm; well, the only way you'd end up in that situation is if you put yourself in it ;). I'd rather have the underlying functionality with a "text edit" based UI than not have it at all, but I can understand the position of not wanting to confuse users.

@droidmonkey what do you think?

@droidmonkey
Copy link
Member

droidmonkey commented Dec 31, 2024

A UI is necessary, but the WIP one is way too complex. This feature is niche to begin with and should at least be marked as "advanced".

image

Since constraints seem to be written as simple user@destination strings, can we just do that?

$ ssh-add -h "[email protected]" \
          -h "scylla.example.org" \
          -h "scylla.example.org>[email protected]" \
          ~/.ssh/id_ed25519

I still don't fully understand the full scope of this feature and don't have time to dive into the docs or ssh-agent source code.
https://www.openssh.com/agent-restrict.html

@pbhenson
Copy link

pbhenson commented Jan 1, 2025

Since constraints seem to be written as simple user@destination strings, can we just do that?

That would work for me, I generally prefer text config files anyway.

I still don't fully understand the full scope of this feature and don't have time to dive into the docs or ssh-agent source code.

The concept is fairly simple. Basically, the key constraints limit what the key is allowed to be used for. The simplest one would be just:

box.example.com

and would mean that the key could be used only to authenticate to that system from the local system, as any user, and nothing else. Then you can also specify a username:

[email protected]

that further restricts the key to only be allowed to authenticate as that user at that system.

To allow a key to be forwarded and used by a host other than the local one, the syntax is extended:

box.example.com>otherbox.example.org

This would allow the key to be forward to box, and used from box to authenticate as any user on otherbox. Again, you can further restrict by username:

box.example.com>[email protected]

You can also use wildcards in the spec:

jdoe@*.example.com

That's pretty much it. From the perspective of keypass, all it needs to do is store 1 or more of these strings with a key and when it passes the key to ssh-add include each one as a separate -h option when calling it.

@droidmonkey
Copy link
Member

droidmonkey commented Jan 1, 2025

Seems super simple, why keeagent made the configuration for this god awfully complex is beyond me.

Upon further reflection, I still want this to be a simple string list (exactly like how we do extra urls in the browser tab). Then we will need to do a couple things on the GUI side:

  1. Syntax checks, marks entries with red background that don't conform to the spec
  2. Parse the entry into its constituent parts for use in sending it to ssh-agent using the protocol and storing it in the KeeAgent.settings file
  3. Likewise, when reading KeeAgent.settings file, we will need to reconstitute them into appropriate strings to place in the gui

We don't directly call ssh-agent binary, so passing the -h parameters isn't an option unless we completely overhaul how we have implemented SSH Agent integration.

For origin and destination host keys, is that expecting a fingerprint of the public key? It is unclear what should be put there. Do we even need to support that? Seems superfluous and extremely niche. We could support it in the backend but not the gui.

@droidmonkey droidmonkey force-pushed the feature/ssh-agent-destination-contraints branch from 518c2fe to 6b90958 Compare January 1, 2025 14:47
@pbhenson
Copy link

pbhenson commented Jan 2, 2025

Seems super simple, why keeagent made the configuration for this god awfully complex is beyond me.

I've never used keeagent. Unless I'm looking in the wrong place, I don't see support for key destination constraints documented?

https://keeagent.readthedocs.io/en/latest/usage/options.html

It only mentions "confirm constraint", requiring explicit ok everytime a key is used, and "lifetime constraint", auto unloading a key after a period of time.

We don't directly call ssh-agent binary, so passing the -h parameters isn't an option unless we completely overhaul how we have implemented SSH Agent integration.

Oooh, you add keys over the socket using the agent protocol? I didn't realize that.

For origin and destination host keys, is that expecting a fingerprint of the public key? It is unclear what should be put there. Do we even need to support that?

Per the documentation:

https://www.openssh.com/agent-restrict.html

"When adding a key with destination constraints, ssh-add will use the local known_hosts files to map the host names given on the command-line to hostkeys before passing them to ssh-agent"

When the agent evaluates the constraints at key use time, it does so based on host keys, not names. The "name to host key" mapping is done once when the key/constraints are added, not every time a key is used.

If you're adding the keys via protocol, not leveraging ssh-add, then the host keys for the host names in the constraints would need to be included in the operation, either by doing the normal ssh thing of dredging through known_hosts files, or by requiring the matching host key for the constraint to be defined inside keepassxc alongside the constraint.

You can find the ssh-add implementation of it here:

https://github.com/openssh/openssh-portable/blob/master/ssh-add.c#L758

Hmm. I'm not sure what the best approach would be, both options have undesirable issues :(.

@droidmonkey
Copy link
Member

droidmonkey commented Jan 2, 2025

I'm generally not in favor of this feature given the intense amount of complexity involved.

I've never used keeagent. Unless I'm looking in the wrong place, I don't see support for key destination constraints documented?

@pbhenson see OP's original post:

For now there is no UI so configuration must be done in KeePass2/KeeAgent.

image

@pbhenson
Copy link

pbhenson commented Jan 2, 2025

I'm generally not in favor of this feature given the intense amount of complexity involved.

Yah, I assumed keepass would just be storing the text description of the constraints and letting ssh-add do the heavy lifting. Replacing the parsing and key finding performed by ssh-add is definitely more complicated.

@kgraefe
Copy link
Contributor Author

kgraefe commented Jan 3, 2025

If we drop KeeAgent compatibility, we could get away with two text-based lists: one for the constraints (as in ssh-add cli) and one for the host keys. The second one could be replaced by parsing the local known_hosts file but I think it makes more sense to have it in the database to avoid external dependencies. (Maybe we can also drop the "CA" support on host keys for now. At least I have not seen this being used in the wild.)

tbh, this is what I was expecting before looking into KeeAgent. I was quite surprised that they made it so complicated. As a result KeeAgent's UI makes it very hard to copy constraints (e.g. for building chains) or update host keys.

Otherwise, could you image merging this feature with no UI but only a checkbox in Settings > SSH-Agent > Advanced and a link to some documentation?

@droidmonkey
Copy link
Member

droidmonkey commented Jan 3, 2025

For stage 1 I can absolutely get behind the checkbox and documentation pointing to using keeagent or building your own custom keeagent.settings file.

This will eventually need a UI if people care, but let's not invest too much time in one until a good number of people show interest.

Let's not break compatibility with keeagent. After reading the full sshagent writeup of this feature, I totally get why this is complicated. They reduced complexity as much as possible in ssh-add implementation. They are bounded by some functional but terrible implementation details of ssh.

This change adds testing all KeeAgentSettings fields including their XML
conversions by separately:
- verifying the default value,
- change the current to something else,
- convert the KeeAgentSettings object to XML,
- convert it back to a second KeeAgentSettings object,
- compare both objects to be equal and
- verify that the new value landed in the field of the second
  KeeAgentSettings object.

Signed-off-by: Konrad Vité <[email protected]>
This change implements loading ssh-agent destination constraints from
KeeAgent.settings into the ssh-agent. For now there is no UI so
configuration must be done in KeePass2/KeeAgent or manually.

The ssh-agent constrain extension is described at [1]. However, I found
it partly misleading:
- in the constaint array each constraint is enveloped where in the
  keyspec arrays the keyspec are just appended to the constraint.
- each constraint and host has an additional string field reserved for
  future use.
The actual structure has been obtained from openssh ssh-add source code [2].

[1]: https://www.openssh.com/agent-restrict.html
[2]: https://github.com/openssh/openssh-portable/blob/3ad669f81aabbd2ba9fbd472903f680f598e1e99/authfd.c#L538

Signed-off-by: Konrad Vité <[email protected]>
@kgraefe kgraefe force-pushed the feature/ssh-agent-destination-contraints branch from 6b90958 to b5845c7 Compare February 6, 2025 21:10
@kgraefe kgraefe force-pushed the feature/ssh-agent-destination-contraints branch from b5845c7 to 0625c19 Compare February 7, 2025 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: SSH agent pr: new feature Pull request that adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow setting destination constraints for each ssh key
5 participants