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

feat: ssh configurable number of public key attempts before failing #3739

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

rocketeerbkw
Copy link
Member

@rocketeerbkw rocketeerbkw commented Jun 12, 2024

General Checklist

  • Affected Issues have been mentioned in the Closing issues section
  • Documentation has been written/updated
  • PR title is ready for inclusion in changelog

Problem

When using lagoon-cli to SSH or login, an error is returned: error: maximum authentication attempts exceeded.

Solution

Increase the MaxAuthTries of the SSH server. I added the setting as an environment variable MAX_AUTH_TRIES so that it can be changed on a per-case basis.

Background

Edit: the below issues with lagoon-cli are mostly fixed with uselagoon/lagoon-cli#355, but this can still be triggered when using non-lagoon tools like ddev.

lagoon-cli will iterate over all of the keys in the system ssh-agent, but the order of the keys is non-deterministic. In the case where a user has more than six SSH keys, it's possible that the correct one is at the end of the list and the max auth attempts is exceeded.

Now that lagoon-cli and lagoon-sync have integrated SSH go libraries, they no longer call out to the system ssh binary. The side effect is that ssh client config files are no longer used. Previously, a user could set the IdentityFile in their ~/.ssh/config and avoid this problem.

The lagoon-cli can be configured with a specific ssh key (either in ~/.lagoon.yml or by passing -i) but then the user will be asked to enter their password for every command.

@shreddedbacon
Copy link
Member

shreddedbacon commented Jun 12, 2024

Can you make it configurable? That way if someone comes around later asking for 19, or 32 retries, they can adjust a chart value?

Edit: Use ep to envplate the sshd_config if it isn't already done, and use the same variable in the ssh-portal, chart can use global to configure both, or remote portals could then be configured independently

@smlx
Copy link
Member

smlx commented Jun 13, 2024

Rather than hacking around a client limitation in the server, can we add support for parsing the config file to lagoon-cli? e.g. I found https://github.com/kevinburke/ssh_config

edit: even simpler would be to add an identityfiles directive to lagoon-cli's config file with equivalent functionality to ssh_config's IdentityFile.

Some problems I see with the change in this PR:

  • it will slow down SSH connections if you try 17 keys before finding the right one.
  • people will still have trouble using multiple keys because there is no way to set the order. It is standard practice in SSH usage to have multiple keys. We should support that.
  • eventually someone will come along with 19 keys in their agent and it will still break.

@shreddedbacon
Copy link
Member

shreddedbacon commented Jun 13, 2024

edit: even simpler would be to add an identityfiles directive to lagoon-cli's config file with equivalent functionality to ssh_config's IdentityFile.

This would be easy enough to do
Edit: already has the -i flag on lagoon-cli, but this bypasses the agent

@rocketeerbkw
Copy link
Member Author

Rather than hacking around a client limitation in the server

IMHO it's the other way around, the server is limited and the previous "hack" was to set an IdentityFile directive for the lagoon ssh servers. Now we have to "hack" the lagoon-cli and lagoon-sync to also get around this limitation? We can just increase the number instead/in addition.

We've already had complaints from other users about this error, our response was "learn how to use SSH config." With that solution now gone (and let's be honest, now that I'm personally impacted), this very minor fix went to the top of my list.

edit: even simpler would be to add an identityfiles directive to lagoon-cli's config file with equivalent functionality to ssh_config's IdentityFile.

I addressed this in the OP. The cli has this option, but every command requires that you enter your password because it's no longer using the ssh-agent. It's the worst of all options.

can we add support for parsing the config file to lagoon-cli

Maybe? I looked at some solutions, there were a lot of questions about getting hostname globbing working correctly for example. While that's getting worked out, I can't SSH into environments until I reboot my computer enough times to get a order in ssh-agent that works.

it will slow down SSH connections

Source? I noticed no difference, maybe it delays by 10's of milliseconds? And it only slows down for users that have lots of keys.

This SO answer suggest that the expensive part is the initial connection, and each "auth try" is cheap.

It is standard practice in SSH usage to have multiple keys. We should support that.

This PR is to add support for more than 6 ssh keys, maybe I'm misunderstanding this comment?

@smlx would your concerns would be mitigated by a configurable MaxAuthTries that @shreddedbacon suggested, such that it can be set back to the default value at some point in the future if needed?

@smlx
Copy link
Member

smlx commented Jun 13, 2024

IMHO it's the other way around, the server is limited and the previous "hack" was to set an IdentityFile directive for the lagoon ssh servers. Now we have to "hack" the lagoon-cli and lagoon-sync to also get around this limitation? We can just increase the number instead/in addition.

MaxAuthTries 6 is the OpenSSH server default, so I don't think using IdentityFile in the client is really a hack. Also, increasing the number of tries doesn't address the use-case of multiple keys in the ssh-agent being used for different Lagoon accounts.

I addressed this in the OP. The cli has this option, but every command requires that you enter your password because it's no longer using the ssh-agent. It's the worst of all options.

What I meant is that in your ~/.lagoon.yml you could put something like:

lagoons:
    test:
        identityfiles:
        - ~/.ssh/id_ed25519_lagoon1.pub
        - ~/.ssh/id_ed25519_lagoon2.pub

And then, just like ssh does, lagoon-cli can prefer to use the keys from the identityfiles when using the agent. Would that work for your use-case?

@shreddedbacon
Copy link
Member

shreddedbacon commented Jun 13, 2024

What I meant is that in your ~/.lagoon.yml you could put something like:

lagoons:
    test:
        identityfiles:
        - ~/.ssh/id_ed25519_lagoon1.pub
        - ~/.ssh/id_ed25519_lagoon2.pub

And then, just like ssh does, lagoon-cli can prefer to use the keys from the identityfiles when using the agent. Would that work for your use-case?

How do tell the agent library in go to use a specific key in the agent? I couldn't find anything that described this clearly?

Edit: the CLI already has a flag to specify a key to use, but atm this is a forced bypass of the agent. So if the key is encrypted it will prompt a password. But if there is a way to pass this to the client to use when the agent is present, that would be a simple fix in the CLI.

Edit2: I think I see how it could work. Yep, I have a POC that can do this

@rocketeerbkw
Copy link
Member Author

I'm all for also improving the cli experience, created an issue to discuss those details separately uselagoon/lagoon-cli#354.

@smlx
Copy link
Member

smlx commented Jul 2, 2024

Is this now addressed by uselagoon/lagoon-cli#355?

@rocketeerbkw
Copy link
Member Author

This mostly doesn't bother me anymore with the ability to set a public key in lagoon-cli, but there are still edge cases. For example, in ddev, you have to add all ssh keys to it's ssh-agent, you can't add just one (like in pygmy), so this still is annoying there. I can fix that one by duplicating my ssh config and lagoon-cli config in my ddev projects... or we can just increase the limit on the server.

I've updated this PR so that the max tries is still 6 by default but can be overridden by environment variable.

@rocketeerbkw rocketeerbkw changed the title feat: increase number of public key attempts before failing ssh feat: ssh configurable number of public key attempts before failing Oct 31, 2024
@rocketeerbkw rocketeerbkw added this to the 2.22.0 milestone Oct 31, 2024
Copy link
Member

@shreddedbacon shreddedbacon left a comment

Choose a reason for hiding this comment

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

As controversial as this appeared to be. I'm fine with this change, if you don't set the variable, nothing changes.

@tobybellwood tobybellwood merged commit f13dd62 into main Nov 7, 2024
1 check passed
@tobybellwood tobybellwood deleted the ssh-max-auth-retries branch December 29, 2024 23:50
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.

4 participants