-
-
Notifications
You must be signed in to change notification settings - Fork 297
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
Conversation
What happens when the actor has no SSH key uploaded to their GitHub profile? Does the action still continue to run, but without |
Fair concern. Line 85 in 00be0d9
which could be expanded to:
to make it even more helpful |
@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 |
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
|
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. |
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". |
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 |
I have another idea: how about introducing a new mode |
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 |
I think that you're right and #127 is the best direction to address that.
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.
Yes. But we do not really have to address this concern in this PR. |
Should we move it to the top and suggest ssh also for other examples like
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 was just mentioning that because it seemed yo be a concern earlier in this PR |
I guess I am starting to come around to agree that it makes sense to change the default. @mxschmitt what's your take? |
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 |
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. |
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. |
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 Besides, 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 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. |
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.
Let's leave the door unlocked in all new flats, someone might have to come to fix the dishwasher one day.
It's a line of config that's asked. It can be included and commented by default.
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 ;-)
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.
:-/ |
I have the vague impression that you underestimate the amount of From my (admittedly biased) personal experience, one of the two dozen or so It is your prerogative as an individual user to demand that 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 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 |
Possibly. I guess we differ in our expectations of the average users. But it's hard to get numbers.
I am the maintainer of several projects and understand that one sometimes has to say no.
That is fine, we seem to differ in what we think gives the best UX to security balance. |
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? |
I am sorry if this appeared passive aggressive, that was not my intention.
Reading comments and issues, this did convince people, but not the ones in charge. |
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. |
Nope. Most people push via |
Closing since it's stale, see #127 (comment) for more related discussions. |
Better safe than sorry
Resolves #124