SMB/CIFS mounts using ownCloud login, fixes #7843 #7875

RobinMcCorkell · 2014-03-25T13:59:38Z

This PR provides a new mount type, that uses the user's ownCloud login credentials to connect to the SMB/CIFS share. It also has the ability to use the username as the share name, for use with home shares. The username and password are stored as session variables, however I do not consider this a security risk as iRODS already does the same thing.

Fixes #7843

Please review @blizzz @PVince81 @icewind1991 @DeepDiver1975

icewind1991 · 2014-03-25T14:18:35Z

The admin interface shows the external storage to be invalid since it wont be able to check if the storage is valid without login in to cache the credentials, this check will need to be overwritten.

Besides that everything seems to work

RobinMcCorkell · 2014-03-25T14:43:35Z

Hmm, how should that be implemented? I could override the check to always return success, but perhaps extending the current status mechanism to include a yellow 'unknown' symbol might be the best solution?

icewind1991 · 2014-03-25T14:49:22Z

Maybe only check if the hostname provided is valid

ghost · 2014-03-25T15:10:51Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3873/

PVince81 · 2014-03-25T16:47:11Z

Another "problem" here is that the password is always stored in the session regardless whether any ext storage is used. You only need to enable the app and then the password will be stored.
I'm not sure how to do this better, it might be possible to find out whether at least one SMB_OC storage is mounted, and only then the password would be stored.

I know you copied this from iROdS, I never realized that it worked that way... (CC @LukasReschke)

RobinMcCorkell · 2014-03-25T18:03:52Z

Is that an issue? All communication between a client and the server is over HTTPS, and we assume the browser is secure, so there is little to no security risk with this surely?

LukasReschke · 2014-03-25T18:07:10Z

Storing the password in the sessions which may get stored on the local file system is a bad idea.

This basically makes ownCloud "one service to pwn them all" (especially in LDAP environments), if an attacker gains access to the sessions he has access to the clear text passwords of every user... This is not really what a normal user or sysadmin would expect.

I do understand the need for such "SSO" setups. And I think that there is not a secure solution at all. Therefore I'd like to suggest one of the following two approaches:

The password is stored in the session but encrypted with the salt in config/config.php
The password is stored encrypted inside the users mount config and updated after every login.

I think 2 may be a better approach as it allows ownCloud to access the mountpoint even if the user is not logged in (e.g. for some occ commands)

In both cases when clicking the "use the same login" checkbox a warning should get shown to the user/admin explaining that this requires ownCloud to store the password in plaintext and in case of a security vulnerability an attacker may be able to gain access to the ownCloud password.

Any other suggestions?

LukasReschke · 2014-03-25T18:08:11Z

And as Vincent said: The password should only get stored if a mountpoint requires it.

PVince81 · 2014-03-25T18:11:02Z

Question is, should this be fixed separately ? Because that's what happens for iRODS.
We could fix this for iRODS first then reuse it for this PR ?

LukasReschke · 2014-03-25T18:11:41Z

Yeah. This would be a good idea.

RobinMcCorkell · 2014-03-25T18:12:36Z

Caching the password in the mounts config could also provide a partial solution to the sharing problem.

PVince81 · 2014-03-25T18:15:27Z

One idea would be to modify the login hook and make it go through the "Config" class. That class could then check whether there is a mount for SMB_OC or iRODS and then store the password into its config (if it has the "use ownCloud login" flag set).

PVince81 · 2014-03-25T18:16:13Z

Ok maybe not iRODS yet as it might not work, we can do that later separately.

RobinMcCorkell · 2014-03-26T11:15:14Z

Well that's the checks part complete. Tested here, seems to work, please review.

An interesting bug I found: when configuring a system share with 'All Users' defined, the check performed is done with $isPersonal = false (aka a system share), which is correct. However, reloading the page, or indeed loading that page with already configured shares that use 'All Users', the check performed is done with $isPersonal = true, which knocks out my custom logic and is likely not desired behaviour.

EDIT: Ignore me, that seems to be a bug of my modified code.

ghost · 2014-03-26T12:03:51Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3895/

ghost · 2014-03-26T12:20:35Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3896/

PVince81 · 2014-03-26T12:24:51Z

@Xenopathic you might not need the option "username as share", there is an obscure hidden feature in the code where "$user" gets replaced by the current user, see owncloud-archive/documentation#307

But keeping it is good for visibility / UX

PVince81 · 2014-03-26T12:26:31Z

Going to test this:

Old SMB still works (regression test)
System-wide SMB_OC with "username as share"
System-wide SMB_OC with custom share name
Personal SMB_OC with "username as share"
Personal SMB_OC with custom share name

PVince81 · 2014-03-26T12:34:10Z

One test failed. Note that I've used a "root" value for all shares.
Might need to retest everything without "root" value

TODO:

raise separate issue/PR for the password storage

PVince81 · 2014-03-26T12:35:22Z

retest the steps above with an empty root (I believe the code path is slightly different)

RobinMcCorkell · 2014-03-26T13:02:05Z

The system wide tests without root work fine for me. I will see what is wrong with a non-empty root, and a custom share name.

RE the separate PR for password storage - should it reference the iRODS credential mechanism, or wait until this PR goes through so that it can reference both?

PVince81 · 2014-03-26T13:06:12Z

As you like.
I do hope to get your PR merged soon anyway 😉

RobinMcCorkell · 2014-03-26T13:13:06Z

Hmm, custom share name with root is working fine here.

PVince81 · 2014-03-26T13:52:44Z

Oh noes, it seems you need to rebase onto master.

I tried again from scratch and I still don't see the "/smb" mount point appearing in the file list.
These are my settings:

PVince81 · 2014-03-26T13:53:36Z

Wait... I think I see. Of course the user "root" doesn't have smb access. Trying with the correct user now.

PVince81 · 2014-03-26T13:54:22Z

Never mind, it works. It was just a misunderstanding.
I logged in with a user that has the same uid and password on the Samba server and it worked.

PVince81 · 2014-03-26T13:55:01Z

Please rebase this and then I think it's good to go 👍

@icewind1991 second review?

Selecting 'SMB/CIFS Auto' in the mounts configuration allows an SMB/CIFS mount to be configured that uses the credentials of the user logging in to authenticate to the server. Optionally, the username can be used as the share name, permitting home shares to be dynamically mounted.

getDisplayName would return the display name of the user, not great if it is a canonical string. The uid passed back from 'login' is the UUID of the user, so also not suitable. The login name from the session is the username the user used to log in to ownCloud in the first place, which is what is needed.

Shares authenticated with user credentials (aka not hard coded) cannot be accessed by other users, breaking sharing. This change completely revokes sharing for such shares

SMB_Auto is now SMB_OC, and the name has been changed from "SMB / CIFS Auto" to "SMB / CIFS using OC login"

To check for shares, the code attempts to connect anonymously to the share. In most cases this will fail with NT_STATUS_ACCESS_DENIED, so the regex array used for parsing the output of smbclient in smb4php has been overridden to treat such output as success. The 'test' method for storage classes can now take a single parameter, $isPersonal, which allows the storage to adjust the tests performed based on if they are being configured as personal shares or as system shares.

RobinMcCorkell · 2014-03-26T14:38:39Z

Rebase complete

icewind1991 · 2014-03-26T15:01:05Z

apps/files_external/lib/smb_oc.php

+
+	public function test($isPersonal = true) {
+		if ($isPersonal) {
+			if ($this->stat(''))


Please always use {}

icewind1991 · 2014-03-26T15:06:01Z

It still shows the mount as invalid in the admin panel for me

scrutinizer-notifier · 2014-03-26T15:11:52Z

A new inspection was created.

RobinMcCorkell · 2014-03-26T15:12:53Z

@icewind1991 Can you post the details of the configuration? And if you try to run:

smbclient -N //<host>/<share -c 'exit'

What error do you get? You should get an NT_STATUS_ACCESS_DENIED, which is correctly detected by the code as success.

ghost · 2014-03-26T15:47:40Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3903/

ghost · 2014-03-26T16:30:26Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3904/

PVince81 · 2014-03-26T17:46:03Z

@icewind1991 did you pull ? What combination of settings did you use ?

icewind1991 · 2014-03-27T11:18:11Z

Never mind, the problem was that I didn't do a login after checking out the branch, works for me now after a re-login.

👍

SMB/CIFS mounts using ownCloud login, fixes #7843

PVince81 · 2014-03-27T11:20:12Z

@Xenopathic do you want to take care of the "only store password when mount point is used" part as a separate PR ? (with the details I suggested here #7875 (comment))

Edit: missing word

RobinMcCorkell · 2014-03-27T14:24:36Z

@PVince81 Sure, I'll get on it.

PVince81 · 2014-03-27T16:20:00Z

Excellent. Thanks a lot for your contributions 😄

segdy · 2014-05-02T04:44:04Z

Hi, did this patch find its way in the official branch? When would it be available in the official version?

LukasReschke · 2014-05-02T06:05:35Z

It will be included in our next major release: ownCloud 7.

segdy · 2014-05-02T06:12:24Z

Thanks! Cool, can't wait for it ... will retry my experiments then.

PVince81 added Enhancement labels Mar 25, 2014

Robin McCorkell added 5 commits March 26, 2014 14:35

Revoke sharable permission on automatic SMB shares

86aa610

Shares authenticated with user credentials (aka not hard coded) cannot be accessed by other users, breaking sharing. This change completely revokes sharing for such shares

Rename SMB_Auto to SMB_OC and change visible name

cac4aaa

SMB_Auto is now SMB_OC, and the name has been changed from "SMB / CIFS Auto" to "SMB / CIFS using OC login"

Fix error in $isPersonal setting

cd5ebac

icewind1991 reviewed Mar 26, 2014
View reviewed changes

Use braces on single line if statements

a1dca82

PVince81 pushed a commit that referenced this pull request Mar 27, 2014

Merge pull request #7875 from Xenopathic/files_external_smb_auto

1f6259d

SMB/CIFS mounts using ownCloud login, fixes #7843

PVince81 merged commit 1f6259d into owncloud:master Mar 27, 2014

RobinMcCorkell deleted the files_external_smb_auto branch March 27, 2014 14:24

RobinMcCorkell mentioned this pull request Jul 16, 2014

UI improvements for external storage configuration #9663

Closed

RobinMcCorkell mentioned this pull request Aug 28, 2014

Files External "Use ownCloud login" #6646

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 16, 2019

SMB/CIFS mounts using ownCloud login, fixes #7843 #7875

SMB/CIFS mounts using ownCloud login, fixes #7843 #7875

Conversation

RobinMcCorkell commented Mar 25, 2014

icewind1991 commented Mar 25, 2014

RobinMcCorkell commented Mar 25, 2014

icewind1991 commented Mar 25, 2014

ghost commented Mar 25, 2014

PVince81 commented Mar 25, 2014

RobinMcCorkell commented Mar 25, 2014

LukasReschke commented Mar 25, 2014

LukasReschke commented Mar 25, 2014

PVince81 commented Mar 25, 2014

LukasReschke commented Mar 25, 2014

RobinMcCorkell commented Mar 25, 2014

PVince81 commented Mar 25, 2014

PVince81 commented Mar 25, 2014

RobinMcCorkell commented Mar 26, 2014

ghost commented Mar 26, 2014

ghost commented Mar 26, 2014

PVince81 commented Mar 26, 2014

PVince81 commented Mar 26, 2014

PVince81 commented Mar 26, 2014

PVince81 commented Mar 26, 2014

RobinMcCorkell commented Mar 26, 2014

PVince81 commented Mar 26, 2014

RobinMcCorkell commented Mar 26, 2014

PVince81 commented Mar 26, 2014

PVince81 commented Mar 26, 2014

PVince81 commented Mar 26, 2014

PVince81 commented Mar 26, 2014

RobinMcCorkell commented Mar 26, 2014

icewind1991 Mar 26, 2014

Choose a reason for hiding this comment

icewind1991 commented Mar 26, 2014

scrutinizer-notifier commented Mar 26, 2014

RobinMcCorkell commented Mar 26, 2014

ghost commented Mar 26, 2014

ghost commented Mar 26, 2014

PVince81 commented Mar 26, 2014

icewind1991 commented Mar 27, 2014

PVince81 commented Mar 27, 2014

RobinMcCorkell commented Mar 27, 2014

PVince81 commented Mar 27, 2014

segdy commented May 2, 2014

LukasReschke commented May 2, 2014

segdy commented May 2, 2014