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

Remove recommendation to change permissions #431

Merged
merged 1 commit into from Apr 26, 2017

Conversation

@LukasReschke
Copy link
Member

@LukasReschke LukasReschke commented Apr 26, 2017

  1. The wording here is really off. We don't really recommend this highly.
  2. This is even counterproductive as it disallows updating via the app store. Also makes the updater unusable.
  3. Once we have automatic updates this will effectively decrease security from my PoV
  4. The script that is there had even some bugs and once destroyed the complete root file system…

Also, an attacker can execute code also using other ways once they have write access to Nextcloud or the data folder. So this doesn't really help...

As we get a ton of bug reports about issues with that, let's kill it. I never liked it anyways…

1. The wording here is really off. We don't really recommend this highly.
2. This is even contraproductive as it disallows updating via the app store
3. Once we have automatic updates this will effectively decrease security from my PoV

Also, an attacker can execute code also using other ways once they have write access to Nextcloud or the data folder.

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
@LukasReschke LukasReschke added this to the Nextcloud 12.0 milestone Apr 26, 2017
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Makes sense 👍

@MorrisJobke MorrisJobke merged commit 4da075f into master Apr 26, 2017
2 checks passed
2 checks passed
continuous-integration/drone/pr the build was successful
Details
continuous-integration/drone/push the build was successful
Details
@MorrisJobke MorrisJobke deleted the remove-recommendation-to-change-permissions branch Apr 26, 2017
@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Apr 26, 2017

Backport?

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Apr 26, 2017

stable11 8edb15a

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Apr 26, 2017

stable10 c65b89c

@nachoparker
Copy link
Member

@nachoparker nachoparker commented Apr 26, 2017

yeah, that is actually a good move

@nachoparker
Copy link
Member

@nachoparker nachoparker commented Apr 26, 2017

btw, do you have any conversations already about automatic updates? any link?

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Apr 27, 2017

btw, do you have any conversations already about automatic updates? any link?

It's still only in our head ... but we want to use our current updater and enhance it a bit.

@nachoparker
Copy link
Member

@nachoparker nachoparker commented Apr 27, 2017

are these discussions open? IRC? can I help?

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Apr 27, 2017

For now they were only temporary in IRCsome time back

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Apr 27, 2017

Maybe you open a ticket in der updater woo and write down your thoughts and how you think it should work

@homelessgamer
Copy link

@homelessgamer homelessgamer commented Apr 30, 2017

you know that apache users will talk shit about nextcloud's security, cause they dont like to see www-data owned files in their /var/www ?!

@LukasReschke
Copy link
Member Author

@LukasReschke LukasReschke commented Apr 30, 2017

Then they should be aware that an attacker with write permissions can already do nasty things by writing to cache data in the data folder which must be writable.

So in essence, I believe that's just offering a wrong sense of security. Furthermore, I do believe that decreasing the barrier to perform updates is essential and until now a lot of people copied some instructions they didn't understand and then never updated. For me that's a way bigger concern.

If someone wants to do that, sure go ahead. But we won't advertise it. Especially not with the mid-term goal of having auto-updates.

@enoch85
Copy link
Member

@enoch85 enoch85 commented Jun 1, 2017

Ehm, it's better to use it than to not use it right?

Feels like most bug reports comes from that users can't use the updater, and that that is the biggest reason to remove it. Correct me if I'm wrong. The VM uses it, but we also have out own updater written in bash, never had any issues or reports about it.

@pmario

This comment has been minimized.

Hi I'm new to nc ... but I think there should be some info about the access rights needed and / or some info about "best practice" also see: PR "fix bash chown problem" #561

DecaTec referenced this pull request in DecaTec/Nextcloud-Backup-Restore Sep 23, 2017
nickvergessen added a commit that referenced this pull request Sep 19, 2018
Signed-off-by: Joas Schilling <coding@schilljs.com>
MorrisJobke added a commit that referenced this pull request Sep 28, 2018
…-dont-exist-anymore

Remove reference to strong permissions, see #431 for reasons
MorrisJobke added a commit that referenced this pull request Sep 28, 2018
Signed-off-by: Joas Schilling <coding@schilljs.com>
MorrisJobke added a commit that referenced this pull request Sep 28, 2018
Signed-off-by: Joas Schilling <coding@schilljs.com>
MorrisJobke added a commit that referenced this pull request Sep 28, 2018
Signed-off-by: Joas Schilling <coding@schilljs.com>
@cmonty14
Copy link

@cmonty14 cmonty14 commented Oct 12, 2018

Hello!
Does this mean you will not provide any recommendation regarding permissions anymore?

If I don't install NC from source but a package (available for Debian, Alpine Linux, etc.) I depend on the correct permission settings defined by the package provider.

I would appreciate some (official) recommendations from NC to verify at least if the installation from a package provider is correct.

xshadow added a commit to systemli/ansible-role-nextcloud that referenced this pull request Dec 10, 2019
* Removes permission enforcement since it was deleted from
  upstream in Nextcloud 12 see:
  nextcloud/documentation#431
* Fixes molecule calls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants