login: support ssh-agent for certificates

[Foxboron]

Implement support for opkssh to append created certificates to an available ssh-agent instead of writing files into ~/.ssh.

Certificates are added with a lifetime to the ssh-agent that should correspond to the lifetime of the OIDC token.

Fixes: #6

Note I haven't added tests yet as it will probably take a bit more time then the feature itself, should probably look over the general idea for starters.

Note that this does not reuse private keys from an ssh-agent, that requires a different approach. See #72 (comment)

[EthanHeilman]

This expiration is a little tricky because ID Tokens expire pretty quickly (~1 hour). So by default we don't use the ID Token expiry, but instead use IssuedAt + 24hours. However the server can be configured to use other options, including the exp claim and we don't know what expiry the server will use.

The ID Token expiration works nicely in LoginWithRefresh tho.

Unless you see a compelling reason otherwise, it is probably simplest to set this to IssuedAt + 23 hours.

[Foxboron]

Ah, this makes sense.

But are we getting a new certificate every hour?

Also, would it make sense to include this expiry into the certificate ValidBefore/ValidAfter as well?

[EthanHeilman]

But are we getting a new certificate every hour?

Only if auto-refresh is used and that requires that the server be configured to accept refreshed ID Tokens which isn't configured by default.

I believe OpenSSH will enforce certificate expiry. The problem here is that the client will never know what expiration configuration the server has set. I debated not making this configurable and just hardcoding expiry to 24hours or auto-refresh, but decided to make this configurable at the server. If we had a client config, then companies could tell the client we could add a field to give the client a hint on what expiry time they set in the server.

One idea I've played is with some sort of way for the client and server to communicate prior to authentication. This is possible using force-command in the cert, but that adds attack surface and doesn't seem worth it.

I think for now we should just set 23 hour expiry for certs when not running in --auto-refresh and then solve this problem more completely when opkssh can act as the ssh agent. Do you think that is reasonable? If we go down this path we should make sure to document it in docs/sshagent.md

Also, would it make sense to include this expiry into the certificate ValidBefore/ValidAfter as well?

ValidAfter might help. It wouldn't provide security since non of the fields in the cert are authenticated other than the public key, but may caught bugs and result better error messages if the client's clock is wrong.

[Foxboron]

I think for now we should just set 23 hour expiry for certs when not running in --auto-refresh and then solve this problem more completely when opkssh can act as the ssh agent. Do you think that is reasonable? If we go down this path we should make sure to document it in docs/sshagent.md

I think that is reasonable.

ValidAfter might help. It wouldn't provide security since non of the fields in the cert are authenticated other than the public key, but may caught bugs and result better error messages if the client's clock is wrong.

We can do that in another PR then.

[Foxboron]

Took at stab at this with 5e527b3

[EthanHeilman]

So psyched to see ssh agent support happening.

[Foxboron]

Also, how do we want the tests to be written? Do we want to depend on the ssh-agent for the test suite, or write our own minimal agent to utilize for the tests?

[EthanHeilman]

Also, how do we want the tests to be written? Do we want to depend on the ssh-agent for the test suite, or write our own minimal agent to utilize for the tests?

Unittest that mock out the sshagent using an interface and use our mock OP client. Right now that is only possible in the login function but not the Login struct. You are welcome to make the changes to the Login struct and it would take that task off my todo list but you don't need to. You can just add it to login function something like:

login(ctx context.Context, provider client.OpenIdProvider, sshagent Agent, printIdToken bool)

Then write an integration test which checks sure this works with the real ssh agent in regular and auto-refresh mode. You are welcome to just make duplicate versions of these tests but adding ssh-agent:

opkssh/test/integration/ssh_test.go

Line 305 in 65254bc

func TestEndToEndSSH(t *testing.T) {

https://github.com/openpubkey/opkssh/blob/65254bc99e4641bd5a5dd4fe63bc10d9d6ff61ab/test/integration/ssh_test.go#L484C6-L484C32

Add a cli flag to login to disable using ssh agent and a console message to specify if we are using ssh agent or writing to the file system. This will make debugging the integration tests and opkssh easier.

[Foxboron]

I've added a new test. Currently only using the goph client as writing out an agent socket for the sftp test is a bit annoying.

Tried dealing with ssh-add lifetime for certificate. This also makes an attempt at removing the old certificate upon expiry.

Should probably do tests on the latter part though.

[EthanHeilman]

@Foxboron Feel free to drop the sftp test. I don't think it is providing much value and it is in a awkward place. If we support ssh we support sftp.

[andrewheberle]

Just a comment on adding keys with a particular lifetime, but on Windows the native OpenSSH agent doesn’t support this. Also on Windows communication to the ssh-agent is via named pipes.

I’ve written a wrapper for “opkssh login” where I work around this limitation by having the key/certificate written to a temporary directory, adding those to the ssh-agent, removing the existing key from the agent then moving the new key/certificate into place in ~/.ssh

If it helps the repo is here:

https://github.com/andrewheberle/opkssh-renewer

Happy to contribute if I can.

[EthanHeilman]

@andrewheberle This is awesome. ssh-agent work got a bit derailed by the policy features, really glad to see progress on this. I've been planning to try to get momentum again on ssh-agent. Great to see that users can use ssh-agent with opkssh, today.

Questions:

1. Does storing ~/.ssh/id_opkssh have any security issues because it is exposed on the filesystem? Or is this file just a signaling mechanism.

2. Does anything bad happen if you don't remove the old certificates? Is the value here that the ssh-agent support and that this is a better UX because there is always an unexpired ssh key in the ssh-agent?

3. It really odd that OpenSSH on windows does have this lifetime feature. Any idea why?

[andrewheberle]

1. Does storing ~/.ssh/id_opkssh have any security issues because it is exposed on the filesystem? Or is this file just a signaling mechanism.

I keep the files for a few reasons, which may or may not be good ones :)

Firstly it allows checking if a renewal needs to happen by looking at the age, so in a normal situation renewal is skipped if the files are less than 23-hours old.

It also means that the keys can be re-added to the ssh-agent the next day while they are still valid (this is more of a consideration on *nix OS’s as on Windows the ssh-agent persists its loaded keys in an encrypted form that survives reboots/restarts…which is itself a little surprising).

Finally keeping at least the public key around allows removing just the old identity from the ssh-agent after the new one is added.

2. Does anything bad happen if you don't remove the old certificates? Is the value here that the ssh-agent support and that this is a better UX because there is always an unexpired ssh key in the ssh-agent?

I mainly implemented the removal logic to work around the limitation on Windows with key/cert lifetimes in the “ssh-agent”. On other platforms adding the identity with a lifetime is probably a better solution.

3. It really odd that OpenSSH on windows does have this lifetime feature. Any idea why?

It is strange for sure. There’s been a issue open for a long time to add that missing functionality and allow it to more closely mimic the “standard” unix behaviour:

PowerShell/Win32-OpenSSH#1056

[Foxboron]

fwiw, I have not forgotten this PR. I've just been less motivated recently (pluss vacation) and it's been on my backburner. Every time I pick this up the code has diverged sufficiently from HEAD that it's bit of a struggle to get it up to snuff again 🙃