-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
base: develop
Are you sure you want to change the base?
WIP: ssh-agent: Implement destination constraints #10252
Conversation
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 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 |
5ec70c3
to
34d0a9b
Compare
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 |
4edabff
to
58bf74c
Compare
CI fails because However, how should I proceed with this? I can think of either
|
95f9e59
to
55c6cd2
Compare
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. |
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).
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.
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... |
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.
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. |
@droidmonkey what do you think? |
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". Since constraints seem to be written as simple user@destination strings, can we just do that?
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. |
That would work for me, I generally prefer text config files anyway.
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:
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: 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:
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:
You can also use wildcards in the spec:
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. |
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:
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. |
518c2fe
to
6b90958
Compare
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.
Oooh, you add keys over the socket using the agent protocol? I didn't realize 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 :(. |
I'm generally not in favor of this feature given the intense amount of complexity involved.
@pbhenson see OP's original post:
|
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. |
If we drop KeeAgent compatibility, we could get away with two text-based lists: one for the constraints (as in 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 |
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]>
Signed-off-by: Konrad Vité <[email protected]>
6b90958
to
b5845c7
Compare
Signed-off-by: Konrad Vité <[email protected]>
Signed-off-by: Konrad Vité <[email protected]>
b5845c7
to
0625c19
Compare
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:
TODO:
Fixes #9801
Screenshots
Testing strategy
Type of change