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

Enable ssh key by default #126

Closed
wants to merge 3 commits into from
Closed

Enable ssh key by default #126

wants to merge 3 commits into from

Conversation

m-kuhn
Copy link

@m-kuhn m-kuhn commented Jul 16, 2022

Better safe than sorry

Resolves #124

@dscho
Copy link
Collaborator

dscho commented Jul 17, 2022

What happens when the actor has no SSH key uploaded to their GitHub profile? Does the action still continue to run, but without authorized_keys? If it does not continue, does it offer helpful advice for the actor so they know how to go forward?

@m-kuhn
Copy link
Author

m-kuhn commented Jul 17, 2022

What happens when the actor has no SSH key uploaded to their GitHub profile? Does the action still continue to run, but without authorized_keys? If it does not continue, does it offer helpful advice for the actor so they know how to go forward?

Fair concern.
It will fallback to

throw new Error(`No public SSH keys registered with ${actor}'s GitHub profile`)

which could be expanded to:

No public SSH keys registered with ${actor}'s GitHub profile, add an SSH key or set limit-access-to-actor: false to allow anyone to connect without authentication.

to make it even more helpful

@dscho
Copy link
Collaborator

dscho commented Aug 8, 2022

@m-kuhn thank you for adding that advice. That's useful.

Having said that, I am still unconvinced that we should change the default to limit-access-to-actor=true. It makes the Action more cumbersome to use, after all, and at least for me it prohibits simply copy/pasting the indicated command-line (I always have to specify the private key with -i /path/to/private.key because I have so many of 'em).

@m-kuhn
Copy link
Author

m-kuhn commented Aug 8, 2022

I can see that point.

On the other hand, I think the implications of allowing anyone to connect are hard to see for newcomers. This can leak sensitive information (e.g. secrets used as env vars as suggested by github docs for some scenarios)

For your case, it could also be an option

  • to tune your sshconfig to match an IP range of tmate servers
  • to have a snippet that disables limit-access-to-actor readily available early on in the readme of the repo. My workflow is usually to copy-paste a config from the readme. If you do the same, then it would merely be a question of copying the correct sample.

@dscho
Copy link
Collaborator

dscho commented Aug 8, 2022

It's a big change, and I defer to @mxschmitt to make the call. Iff we change the default, I would recommend a major version bump, to notify existing users of that change.

@mxschmitt
Copy link
Owner

mxschmitt commented Aug 8, 2022

I think similar to @dscho and would not go that far to enforce it. I'm totally fine with adding a warning somewhere in the README, that we recommend setting it to true, and otherwise people can "hijack connections".

@m-kuhn
Copy link
Author

m-kuhn commented Aug 8, 2022

I think similar to @scho and would not go that far to enforce it.

It's probably a typo, but just to be sure: the proposal here is to "enable by default with the possibility to opt-out", not to "enforce".

But if there's no plan to merge this, I'll abandon this work and come up with an alternative PR to propose an update to the readme, that suggests opting into limit-access-to-actor for security reasons.

@dscho
Copy link
Collaborator

dscho commented Aug 9, 2022

I have another idea: how about introducing a new mode limit-access-to-actor: auto that uses the registered SSH keys if there are any, and falls back to false if there aren't any (with a loud warning and a concise and helpful explanation how to fix this)? I would have no problems with making that the default.

@m-kuhn
Copy link
Author

m-kuhn commented Aug 9, 2022

My concern is that other developers with privileged access to repositories I work on will run this action according to the first sample they run into (just like I did myself the first time I used it). Setting this to auto will increase the likelihood they are using ssh but if someone did not set up an ssh key this still doesn't hold.
I also wonder if this fixes a problem for your scenario. You (probably?) have set up an ssh key in github and will still be forced to use -i.
Summary, it will partially address my concerns but I'm not completely convinced.

@dscho
Copy link
Collaborator

dscho commented Aug 9, 2022

My concern is that other developers with privileged access to repositories I work on will run this action according to the first sample they run into (just like I did myself the first time I used it).

I think that you're right and #127 is the best direction to address that.

Setting this to auto will increase the likelihood they are using ssh but if someone did not set up an ssh key this still doesn't hold.

Indeed, that's why I also want that big warning if the fall-back is used, but I want to avoid the friction when some user does not even know how to set up private keys.

I also wonder if this fixes a problem for your scenario. You (probably?) have set up an ssh key in github and will still be forced to use -i.

Yes. But we do not really have to address this concern in this PR.

@m-kuhn
Copy link
Author

m-kuhn commented Aug 9, 2022

My concern is that other developers with privileged access to repositories I work on will run this action according to the first sample they run into (just like I did myself the first time I used it).

I think that you're right and #127 is the best direction to address that.

Should we move it to the top and suggest ssh also for other examples like if: failure()?

Setting this to auto will increase the likelihood they are using ssh but if someone did not set up an ssh key this still doesn't hold.

Indeed, that's why I also want that big warning if the fall-back is used, but I want to avoid the friction when some user does not even know how to set up private keys.

That's easily addressed with a warning pointing to the two options, (unsafe usage or adding ssh). People will discover that if the action fails.

I also wonder if this fixes a problem for your scenario. You (probably?) have set up an ssh key in github and will still be forced to use -i.

Yes. But we do not really have to address this concern in this PR.

I was just mentioning that because it seemed yo be a concern earlier in this PR

@dscho
Copy link
Collaborator

dscho commented Aug 9, 2022

I guess I am starting to come around to agree that it makes sense to change the default. @mxschmitt what's your take?

@mxschmitt
Copy link
Owner

limit-access-to-actor=true

Sorry for the slow response. Basically having it required to have SSH keys configured to use this GitHub Action is in my opinion definitely the right way to go if we think about security, but this breaks most of the users workflows, since not everyone has their private SSH key on GitHub, added it to the SSH agent etc.

The basic example should also not contain the limit option, but we can add it as a comment to the snippet, that everyone who has access to the GitHub Actions job logs, has access to the GitHub Action runner and it's exposed secrets.

I think that's an middleground, so people who read can remove the # and have the option and people who don't, oversee it and lead into that "security issue".

@m-kuhn
Copy link
Author

m-kuhn commented Aug 13, 2022

having it required to have SSH keys configured to use this GitHub Action

At the risk of repeating myself, this change does not make it required to have an SSH key configured. It's setting security by default.

Someone who does not have an ssh key set up can still use this action. They will just have to comment the configuration line. And they will realize that they need to because they are advised by the action failure. In addition, they are also advised how to setup an ssh key and improve their security but it's just an alternative.

The effort to disable the security layer is equal to uncommenting the line in the middleground proposal.

I can abandon this as mentioned before but I still wanted to make sure we all have the same understanding of the effects it would have had.

@tbodt
Copy link

tbodt commented Aug 13, 2022

I'm with @m-kuhn on this. It's like if someone was renting apartments with no locks on the doors unless you ask for them. And when people realize this and suggest that perhaps doors should have locks, the landlord says it's too hard to give keys to tenants, so maybe they could add a section to the contract notifying the tenant that they can request installation of a lock if they have strong security needs and accept the responsibility of carrying the keys.

Security should be default! It shouldn't need an extra line of config (even if that config is in the sample code, not everyone will copy the sample code.) If no ssh keys can be found, simply link to a section in the readme that tells you how to add an ssh key or disable the key check. Changing the default might need a major version bump, but that's fine.

@dscho
Copy link
Collaborator

dscho commented Aug 14, 2022

I agree that things should be secure by default insofar possible and practical.

If the first thing the Action does is to fail for the vast majority, then it does not constitute "secure by default" but "broken by default".

Keeping with the analogy with the apartment, you talk about the landlord causing an unnecessary ruckus, but that's misrepresenting what the issue is about. It would be more like a contractor refusing to help you fix your dishwasher because you, the tenant, had been offered an extra entrance to your apartment for an extra fee, which you declined.

It is important to keep in mind just how much of a price you're asking of users who never generate any SSH keys because they never needed them. Especially when the displayed ssh command-line might then not even work as-is because it might need adding -i <path-to-private-key>.

Besides, tmate only allows one active tmux window, across all connected clients. So if a malicious actor connects and starts typing, all other connected users will see it.

Calling the Action in the default way won't expose any secret variables, they would have to be made available explicitly by the user (e.g. via an env directive or by writing a secret to disk in a previous step). And by default, Actions' GitHub token has read-only permissions.

Therefore it is easy to see that there are many scenarios where running the Action without a private SSH key is totally safe.

Yes, there are unsafe scenarios. It is the users' responsibility to make the judgement call whether additional security is required.

@m-kuhn
Copy link
Author

m-kuhn commented Aug 14, 2022

I agree that things should be secure by default insofar possible and practical.

If the first thing the Action does is to fail for the vast majority, then it does not constitute "secure by default" but "broken by default".

I'd expect gh users who make use of actions and ssh to be comfortable with ssh keys normally and would doubt the vast majority will be overwhelmed by this.

Keeping with the analogy with the apartment, you talk about the landlord causing an unnecessary ruckus, but that's misrepresenting what the issue is about. It would be more like a contractor refusing to help you fix your dishwasher because you, the tenant, had been offered an extra entrance to your apartment for an extra fee, which you declined.

Let's leave the door unlocked in all new flats, someone might have to come to fix the dishwasher one day.

It is important to keep in mind just how much of a price you're asking of users who never generate any SSH keys because they never needed them. Especially when the displayed ssh command-line might then not even work as-is because it might need adding -i <path-to-private-key>.

It's a line of config that's asked. It can be included and commented by default.

Besides, tmate only allows one active tmux window, across all connected clients. So if a malicious actor connects and starts typing, all other connected users will see it.

If there are other users. There might be a matrix of jubs running and you connect to just one of them. And observing someone who is stealing your TV is also not so much fun ;-)

Calling the Action in the default way won't expose any secret variables, they would have to be made available explicitly by the user (e.g. via an env directive or by writing a secret to disk in a previous step). And by default, Actions' GitHub token has read-only permissions.

It's not uncommon to install keys for deployment or as linked to earlier to expose secure vars as env vars. Not that I want to encourage people to do that. Still it happens more often than one could think.

Therefore it is easy to see that there are many scenarios where running the Action without a private SSH key is totally safe.

Yes, there are unsafe scenarios. It is the users' responsibility to make the judgement call whether additional security is required.

:-/

@dscho
Copy link
Collaborator

dscho commented Aug 15, 2022

If the first thing the Action does is to fail for the vast majority, then it does not constitute "secure by default" but "broken by default".

I'd expect gh users who make use of actions and ssh to be comfortable with ssh keys normally and would doubt the vast majority will be overwhelmed by this.

I have the vague impression that you underestimate the amount of action-tmate users who use ssh for the first time just to debug their GitHub workflow.

From my (admittedly biased) personal experience, one of the two dozen or so action-tmate users I personally know was a proficient ssh user. One. You seem to expect pretty much 100% of all users to be very familiar with ssh, or you'd like to force them to become familiar.

It is your prerogative as an individual user to demand that action-tmate be changed to suit your preferences and tastes. The responsibility of the maintainer, however, is to counteract such an individual bias and to cater to all users, not just to a few power users.

And do not underestimate how much harm it can do to "choose the right default" without properly informing the users. If users need to learn about the security implications of using action-tmate, then trying to let the Action "Do The Right Thing" instead might actually lead to even worse outcomes because the most likely course of action would be for users to curse, do a web search, then curse some more at the redacted-expletive action-tmate maintainers and simply turn off limit-access-to-actor without even understanding the implications. That's pretty much the opposite of what we're trying to achieve here. It's like that office xerox machine that has been "helpfully" clamped down by some BOFH with a 12-digit PIN, with the obvious UX fix where some helpful secretary simply put a sticker with said PIN right on that xerox machine just so that the good people who have to use it are unblocked already and can do their jobs again thankyouverymuch.

tl;dr it likely is better to add a comment to the code snippet that every user is prone to copy/paste than to let the vast majority of users run into blockers right from the start by changing the default of the limit-access-to-actor setting.

@m-kuhn
Copy link
Author

m-kuhn commented Aug 15, 2022

If the first thing the Action does is to fail for the vast majority, then it does not constitute "secure by default" but "broken by default".

I'd expect gh users who make use of actions and ssh to be comfortable with ssh keys normally and would doubt the vast majority will be overwhelmed by this.

I have the vague impression that you underestimate the amount of action-tmate users who use ssh for the first time just to debug their GitHub workflow.

From my (admittedly biased) personal experience, one of the two dozen or so action-tmate users I personally know was a proficient ssh user. One. You seem to expect pretty much 100% of all users to be very familiar with ssh, or you'd like to force them to become familiar.

Possibly. I guess we differ in our expectations of the average users. But it's hard to get numbers.
I definitely would appreciate and even encourage people becoming more familiar, but I we should not describe that as "force".

It is your prerogative as an individual user to demand that action-tmate be changed to suit your preferences and tastes. The responsibility of the maintainer, however, is to counteract such an individual bias and to cater to all users, not just to a few power users.

And do not underestimate how much harm it can do to "choose the right default" without properly informing the users.

I am the maintainer of several projects and understand that one sometimes has to say no.
Still; I would very much argue this PR is doing its very best to properly inform the users.

If users need to learn about the security implications of using action-tmate, then trying to let the Action "Do The Right Thing" instead might actually lead to even worse outcomes because the most likely course of action would be for users to curse, do a web search, then curse some more at the redacted-expletive action-tmate maintainers and simply turn off limit-access-to-actor without even understanding the implications. That's pretty much the opposite of what we're trying to achieve here. It's like that office xerox machine that has been "helpfully" clamped down by some BOFH with a 12-digit PIN, with the obvious UX fix where some helpful secretary simply put a sticker with said PIN right on that xerox machine just so that the good people who have to use it are unblocked already and can do their jobs again thankyouverymuch.

tl;dr it likely is better to add a comment to the code snippet that every user is prone to copy/paste than to let the vast majority of users run into blockers right from the start by changing the default of the limit-access-to-actor setting.

That is fine, we seem to differ in what we think gives the best UX to security balance.
Since I am in no position to demand anything from the maintenance team -- of which I'm very grateful for making this action available -- I'm considering creating a fork for my own usage that suits my personal needs and spreading the word about it in the projects I'm working on.

@dscho
Copy link
Collaborator

dscho commented Aug 15, 2022

I'm considering creating a fork for my own usage that suits my personal needs and spreading the word about it in the projects I'm working on.

This is the sort of passive aggressive behavior I would like to see less of in Open Source.

I understand that there are times when you have to fork, but if you fail to convince others of your ideas regarding security (and if you fail to be convinced by the counter argument that you're just making things too hard instead of safe), have you considered reconsidering your view point?

@m-kuhn
Copy link
Author

m-kuhn commented Aug 15, 2022

This is the sort of passive aggressive behavior I would like to see less of in Open Source.

I am sorry if this appeared passive aggressive, that was not my intention.
It's more that I really need this action quite often as well as colleagues of mine and composing the right configuration each time became cumbersome. Even more convincing others to go the extra mile to enable a security layer. That's why I started this pull request in the first place and what I still need a solution for.

I understand that there are times when you have to fork, but if you fail to convince others of your ideas regarding security (and if you fail to be convinced by the counter argument that you're just making things too hard instead of safe), have you considered reconsidering your view point?

Reading comments and issues, this did convince people, but not the ones in charge.
So to me it seems this has two different target audiences which appear incompatible enough.

@tbodt
Copy link

tbodt commented Aug 15, 2022

You seem to expect pretty much 100% of all users to be very familiar with ssh, or you'd like to force them to become familiar.

I don't, I think it works out fine if the error message just tells you to disable the option.

That said, I'm somewhat surprised that so many people would have no ssh keys, given that git push from the command line pretty much needs an ssh key.

PS. I would use a fork with this option turned on, if only because it makes the necessary yaml go from 3 lines to 1.

@dscho
Copy link
Collaborator

dscho commented Aug 15, 2022

git push from the command line pretty much needs an ssh key.

Nope. Most people push via https://, typically using Git Credential Manager or GitHub Desktop. No SSH keys.

@mxschmitt
Copy link
Owner

Closing since it's stale, see #127 (comment) for more related discussions.

@mxschmitt mxschmitt closed this Aug 28, 2022
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.

Suggestion: default limit-access-to-actor to true
4 participants