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

Password and private key file for ssh #78

Closed
wants to merge 1 commit into from
Closed

Conversation

inercia
Copy link
Contributor

@inercia inercia commented Apr 3, 2019

This PR adds a -P<password> and -i<key> arguments for using alternative authentication methods for ssh.

Copy link
Contributor

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

Please, let's focus on the sprint goals. I don't think this is necessary at the moment.

@inercia
Copy link
Contributor Author

inercia commented Apr 3, 2019

Please, let's focus on the sprint goals. I don't think this is necessary at the moment.

Well, this is necessary for deploying the cluster in my environment. If you have any comments relative to the code please go ahead...

@ereslibre
Copy link
Contributor

How so? Just do a ssh-add of your key to a running ssh agent as documented on the prerequisites: https://github.com/SUSE/caaspctl#prerequisites.

@inercia
Copy link
Contributor Author

inercia commented Apr 3, 2019

How so? Just do a ssh-add of your key to a running ssh agent as documented on the prerequisites: https://github.com/SUSE/caaspctl#prerequisites.

I'm automating things and that workflow does not match that automation. So do you think there is something wrong with this code?

@ereslibre
Copy link
Contributor

So do you think there is something wrong with this code?

Well, sure:

  • It's duplicating the shorthand for port as p.
  • It doesn't adapt the README prerequisites to include the new flags.

But most important of all, you don't clearly state why an SSH agent is not enough for your needs. We are trying to keep things as simple as possible and stick to the sprint goals. Having these flags is something we explicitly tried to avoid unless we had a valid story of why they are needed, and so far I haven't read anything like that.

@inercia
Copy link
Contributor Author

inercia commented Apr 3, 2019

It's duplicating the shorthand for port as p.

It is not, password uses capital -P.

It doesn't adapt the README prerequisites to include the new flags.

My intention was not to ducument this as, as you said, the main workflow is with ssh-add.

But most important of all, you don't clearly state why an SSH agent is not enough for your needs.

I've already told you that I have my automation scripts (that I will not dump in this discussion for sake of brevity) that do not match that workflow.

We are trying to keep things as simple as possible and stick to the sprint goals. Having these flags is something we explicitly tried to avoid unless we had a valid story of why they are needed, and so far I haven't read anything like that.

There are always changes that appear as side-effects of the work in a sprint, things that must be fixed or small improvements that make things easier when developing. It is a developer's dutty to keep focus on his main task, so they don't need to be babysitted.

@ereslibre
Copy link
Contributor

ereslibre commented Apr 3, 2019

It is not, password uses capital -P.

You changed this after my comment 🚀 (SUSE/caaspctl@c1b4d93 - SUSE/caaspctl@74746dc)

Cannot you do ssh agent forwarding? I still don't see enough evidence to not block this, I'm sorry.

@inercia
Copy link
Contributor Author

inercia commented Apr 3, 2019

Maybe @flavio could break the tie here.

@NunoFilipeSantos
Copy link

@inercia please pick up the tasks that were assigned to you during sprint planning.

Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

It's fine to work on little things that were not planned as part of the sprint as long as the end goals of the sprint are matches.

I like the idea to be able to specify the ssh key via cli, I've run into the lack of this feature at the SUSECON booth and I wanted to file an issue about it.

I don't like the idea to supply a password via the command line. ssh authentication via password is a bad security practice and we should not encourage it. Plus it would add issues on our side like, the password would be visible inside of the history and via ps.

Just remove the code that deals with providing the password from the cli.

@ereslibre
Copy link
Contributor

ereslibre commented Apr 10, 2019

I like the idea to be able to specify the ssh key via cli, I've run into the lack of this feature at the SUSECON booth and I wanted to file an issue about it.

Just to have more context; why was this required? What use cases would this cover that the use of a ssh-agent cannot cover?

@ereslibre
Copy link
Contributor

I believe this adds extra complexity on our side, e.g. if the private key has a passphrase this change is not enough and ssh.ParsePrivateKey is not going to succeed: ssh.ParsePrivateKeyWithPassphrase is required in that case, and the passphrase needs to be passed somehow to caaspctl. Go has issues open using private keys with passphrases: golang/go#18692

Using an ssh-agent abstracts all this from us: they have to do ssh-add and we don't care if the key has passphrase or not, they add the keys they need to the agent and that's it. I don't see the need to add arguments to point to a specific key or adding passwords or handling this complexity at all.

I'm a -1 to this change altogether.

@flavio
Copy link
Member

flavio commented Apr 15, 2019

All right, I didn't know about the limitations of Go when it comes to managing ssh keys. Let's leave everything aside until we get clear feedback from our stakeholders.

@inercia let's close this PR and re-open it if someone asks for one of these features.

In the meantime I've pinged @r0ckarong to understand how much we document about ssh-agent (for example the secret forwarding feature is pretty cool and answers one of the scenarios that was concerning me).

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