shimataro / ssh-key-action

GitHub Action that installs SSH key to .ssh
https://github.com/marketplace/actions/install-ssh-key
MIT License
578 stars 87 forks source link

Known hosts should be optional #143

Closed kurtwheeler closed 3 years ago

kurtwheeler commented 4 years ago

I'm using this action to deploy my server. Some of my deploys require sshing onto the instance. That's the only reason I need this action and I don't know the IP of my server before I create it, therefore I have nothing to pass to known_hosts. It seems a valid use case, so it seems like making the parameter optional makes sense.

shimataro commented 4 years ago

@kurtwheeler It might be valid case, but known_hosts will be necessary in many case for security reason.

If you have no way to know server key/address and are sure there are no security risk (such as MITM attack), please set dummy server key as workaround.

- name: Install SSH key
  uses: shimataro/ssh-key-action@v2
  with:
    key: ${{ secrets.SSH_KEY }}
    known_hosts: github.com ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAq2A7hRGmdnm9tUDbO9IDSwBK6TbQa+PXYPCPy6rbTrTtw7PHkccKrpp0yVhp5HdEIcKr6pLlVDBfOLX9QUsyCOV0wzfjIJNlGEYsdlLJizHhbn2mUjvSAHQqZETYP81eFzLQNnPHt4EVVUh7VfDESU84KezmD5QlWpXLmvU31/yMf+Se8xhHTvKSCZIFImWwoG6mbUoWf9nzpIoaSjB+weqqUUmpaaasXVal72J+UX2B+2RPW3RcT0eOzQgqlJL3RKrTJvdsjE3JEAvGq3lGHSZXy28G3skua2SmVi/w4yCE6gbODqnTWlg7+wC604ydGXA8VJiS5ap43JXiUFFAaQ==
muesli commented 4 years ago

@shimataro What if I only need the SSH key locally during the test? I'm not even trying to connect to anything, but use it to test software that depends on a valid SSH key being installed / available.

shimataro commented 4 years ago

Hi, @muesli. Sorry for the late reply.

You can use dummy server key, such as github.com. If you want to check whether SSH key is valid or not, try SSH to github.com.

Please refer https://github.com/shimataro/ssh-key-action/issues/143#issuecomment-653393172.

muesli commented 4 years ago

@shimataro Yeah, I use that as a hacky workaround. Why not solve this properly tho?

shimataro commented 4 years ago

@muesli It's for security reason. (and I didn't expect your case, no need to connect to anywhere)

Direct connection without known_hosts is dangerous. Therefore this action enforces to specify it.

If you don't have the way to know server key or IP address in advance, please use multistage SSH via "bastion server".

[GitHub]  <-- Internet --> [bastion] <-- LAN --> [target]

If LAN is secure enough, it only needs to specify the key of bastion server to known_hosts.

muesli commented 4 years ago

Maybe I'm overlooking something, but what's the danger of not having a known_hosts file?

shimataro commented 4 years ago

@muesli SSH without known_hosts allows MITM attack.

Of course, there are no risks if you don't connect to anywhere.

muesli commented 4 years ago

@shimataro I'm still a bit puzzled by that. Which MITM attack are you referring to? Not having a known_hosts file is only really a problem if you disable StrictHostKeyChecking, which would be a bad idea in the first place.

shimataro commented 4 years ago

@muesli Missing known_hosts means that SSH cannot verify server. That is, it allows MITM attack.

SSH with CA can verify server without known_hosts, but it isn't a popular method.

muesli commented 4 years ago

I think you're mistaken here. A missing known_hosts file would intercept any connection, asking you if you trust the remote host before proceeding, unless you have disabled StrictHostKeyChecking.

As such it would be absolutely fine to make this feature optional. There's no MITM-scenario and no security issue I can see.

shimataro commented 4 years ago

A missing known_hosts file would intercept any connection, asking you if you trust the remote host before proceeding,

Yes, you are right. But if asked during CI process, the process will stop and fail at the point.

In addition, when you decide whether or not to trust a server, what would you base on? The only way is to check the fingerprint of host key, and it's identical to use known_hosts. known_hosts is to check host key.

muesli commented 4 years ago

But if asked during CI process, the process will stop and fail at the point.

Correct, just like it is the case when you try to connect to any host that you have forgot to add to known_hosts.

In addition, when you decide whether or not to trust a server, what would you base on?

known_hosts is the right place. But remember my original point: there are situations where you don't connect to any host with your SSH key and instead have a different usecase for it. In such cases known_hosts should be optional. There's no security issue as we've elaborated.

shimataro commented 4 years ago

Sorry for late response.

But remember my original point: there are situations where you don't connect to any host with your SSH key and instead have a different usecase for it.

Yes I remember, but it is not an expected usage. This action is for SSH connection; therefore I made known_hosts required parameter.

till commented 3 years ago

I wanted to use this action to do the following:

The known_host parameter is a bit odd in this case. With an elastic environment, known_hosts seems to be a bit less useful anyway? I understand that for deployments and what not, I can use a static bastion host, but even then, I would probably use SSHFP records?

Long story short, I wouldn't mind if you made known_hosts optional and printed a fat warning that people are aware of it? Maybe another option like, insecure: yes, which would be no by default? So people can use this without workarounds like supplying a bogus known_hosts?

onedr0p commented 3 years ago

@till I do the same thing and choose to use webfactory/ssh-agent which works fine.

    - name: Install SSH key
      uses: webfactory/ssh-agent@v0.5.0
      with:
        ssh-private-key: ${{ secrets.DIGITALOCEAN_SSH_KEY }}
shimataro commented 3 years ago

@till Thank you for your proposal, insecure option may be a good idea. Let me think about it.

muesli commented 3 years ago

While I'd still prefer an insecure option over a fake known_hosts file, I still don't see a reason to actually introduce that option. There's nothing insecure about not supplying any known hosts whatsoever.

till commented 3 years ago

Author can do as he pleases. This is their open source. Best is to fork if you really don't like it. Or use the alternative ☝🏼

muesli commented 3 years ago

Of course, I'd just like to point out that the reasoning is rather flawed.

shimataro commented 3 years ago

@muesli

There's nothing insecure about not supplying any known hosts whatsoever.

Could you tell me which one do you mean?

  1. Not providing known_hosts is secure enough in any case
  2. Not providingknown_hosts without thinking is insecure, but there are some methods to make SSH secure without known_hosts
  3. Not providing known_hosts might be insecure, but We don't have to take care because attack is rare
  4. or other meaning

I agree with you if 2 (but these methods are not so popular).

muesli commented 3 years ago

Definitely 1.

The worst thing that can happen if you don't specify any known hosts is ssh refusing to connect to a remote machine before you verify it and add it to the known hosts. There's no security issue here and no MITM attack if you don't have a known_hosts file.

As such I don't understand why an option called insecure would be helpful. If anything it's rather misleading.

shimataro commented 3 years ago

The worst thing that can happen if you don't specify any known hosts is ssh refusing to connect to a remote machine before you verify it and add it to the known hosts. There's no security issue here and no MITM attack if you don't have a known_hosts file.

Yes, you are right, but I think CI will stop and fail at that point.

Or, I am misunderstanding something?

muesli commented 3 years ago

Yes, you are right, but I think CI will stop and fail at that point.

Just like it would if you try to connect to any host that you forgot to add to known_hosts. This should be documented in the README accordingly. There is no security issue however, and no reason to enforce having a known_hosts, when there are good reasons to use this without ever connecting to a remote host.

I hope you understand my reasoning, and I would greatly appreciate it if you could reconsider your decision :heart:

shimataro commented 3 years ago

@muesli I think I get what you mean. Is this OK?

Unfortunately, this action is designed for SSH connection. So known_hosts is expected to exist, and not providing known_hosts is insecure in many case.

But I'm considering a proposal https://github.com/shimataro/ssh-key-action/issues/143#issuecomment-784990726. Please wait for a decision!

muesli commented 3 years ago

No, that's not what I'm saying.

I get it you don't want to support that, I just don't understand your reasoning. At this point I think I'll just go ahead and fork this action since this seems to be the more constructive approach.

shimataro commented 3 years ago

Hello everyone, Since there are some secure methods that don't need known_hosts, such as SSHFP and signed server key, I decided to provide a special value that enables known_hosts to be omitted. It is known_hosts: unnecessary. known_hosts parameter itself remains required and needs to be set to unnecessary explicitly.

DO NOT use it with StrictHostKeyChecking=no option, otherwise you might get MITM attack. Make sure that you are using secure methods. HANDLE WITH CARE!!

It will be released in a few days or weeks. Thank you for many discussion😆

shimataro commented 3 years ago

v2.3.0 has been released, now known_hosts: unnecessary is available! closing.

mxcl commented 3 years ago

@muesli is king