Add $uid and $password templates to 'files_external' configuration

[cedric-dufour]

Pull 'uid' and 'password' variables from PHP session 'smb-credentials'
array - if existing - and allow them to be used as $uid and $password
templates when configuring system mounts (in data/mount.json).

This allows to dynamically mount users' home directories using their
login credentials and comes handy when using the 'user_ldap' authentication
App/backend.

It has been verified to work successfully using OC 7.0.2 and SFTP.

I Hope it can help!

Best,

Cédric

[ghost]

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

[LukasReschke]

This will most likely not work anymore that way with #11747

[ghost]

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

[cedric-dufour]

Regarding #11747
Pulling the encrypted credentials shoudn't be a problem... provided they remain stored, independently from SMB_OC.
Couldn't we imagine having the login credentials stored in the PHP session "generically" (independently from SMB_OC), maybe based on a general configuration setting (to prevent/allow those credentials to be stored inconditionally in the PHP session, for those users who don't need them and have stronger security requirements) ?

[LukasReschke]

Couldn't we imagine having the login credentials stored in the PHP session "generically" (independently from SMB_OC), maybe based on a general configuration setting (to prevent/allow those credentials to be stored inconditionally in the PHP session, for those users who don't need them and have stronger security requirements) ?

Well, this would not really offer any advantages despite the fact that it would be another untested function that leads to potential bugs.

Better approach is: Store the credentials if required and if not, just don't store them.

[LukasReschke]

I propose that I will first take care of #11747 after 7.0.3 is released (could take up to two weeks) and then we can update this PR to decrypt the credentials.

[RobinMcCorkell]

I'm the original author of SMB_OC, and it has a really bad design. There is an PR floating around somewhere that replaces the smb-credentials design with the storage of the password in mount.json, but it wasn't complete and was also messy. We should definitely avoid setting session variables containing the password!

[LukasReschke]

For now the goal is: Store the password only if required and encrypt it with a secret stored in config.php.

Sure - this is not a perfect solution either but we need to move forward and this needs also to work for older versions. We can improve the SMB_OC storage at any time later :-)

[scrutinizer-notifier]

The inspection completed: 8 new issues, 3 updated code elements

[cedric-dufour]

If the "only if required" can be less constraining than SMB_OC being used, then it would great.
Thanks for your comments.
I'll keep watching #11747 and be ready to update this PR if you still deem it useful/appropriate (and send you a signed CA).

[LukasReschke]

Sounds like a plan. Thank you!

[cedric-dufour]

Hello,

I've had time to experiment with what this PR allows to do and thought it may be worth sharing what I learned.

From the (SFTP) sync' perspective:
pro: allows the sysadmin to pre-define and decide which particular sub-directory a user can use in his home; works great...
con: ... until one has the wonderful idea of symlinking his huge-junkyard-directory into that directory; it still works (thanks SFTP and the *nix filesystem)... but it may not be what the sysadmin had in mind to start with

From the OC sharing perspective:
con: it just won't work! since only user-initiated sessions (and stored credentials) will allow to successfully access the external storage, 3rd-parties will not be able to access the file (as far as I could tell and unless I missed something about how OC deals with shared files)
pro: it may come as a nifty "feature" for some sysadmins

About user credentials in PHP sessions:
con: storing plaintext user credentials in PHP session is not the best idea; encrypting those credentials would be only a slight improvement (if one vulnerability exposes the session content, another one - or the same one! - may also expose the encryption key)
con: doesn't allow files to be shared (see above)
pro: limited lifetime; expired session files are regularly deleted

About user credentials in "dynamically-maintained" mount.json:
con: doesn't improve security (if one vulnerability can expose the session content, another one - or the same one! - can expose the mount.json and the encryption key)
con/pro: the mount.json would make the user credentials available permanently (rather than limited to the session lifetime)... much the same way as user-configured external storages
pro: would allow files to be shared (as far as I can tell), the user credentials being stored in the always-available mount.json

Given all those pros and cons (and mostly because of the security consequences), I'm now wondering whether I still want to go down that way...

Hope these thoughts can help others,

Best

[cedric-dufour]

Hello,
I see you're making progress on #11747
But again, I think storing login credentials in the session shouldn't be something specific to each external storage backend (e.g. SMB_OC).
It would be better design to store (encrypted) generic "login-credentials" based on a specific (and well documented) configuration option, and then bring each storage backend (that need them) to use those "login-credentials" rather than having each one replicate the same feature/behavior (with some potentially doing so without encryption)
No?
(if you concur, I'm willing to write the corresponding code)

[LukasReschke]

But again, I think storing login credentials in the session shouldn't be something specific to each external storage backend (e.g. SMB_OC).

Sure. I never said something against that. But our development cycle is: Fixing something in master/stable7 before we think of "changing" the approach completely ;-)

We even have this planned for the next releases, see #12216 - feel free to comment there as well about your thoughts. And if you want to do a task of it: Feel free to. (but please comment there to prevent that somebody else is also working on it)

[ghost]

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

[LukasReschke]

@Xenopathic External storage fun – interested?

[RobinMcCorkell]

I disagree with the ability to substitute in $password - the only field that it is useful for is the password field, but that is hidden by the UI, so it becomes really unintuitive to use. And isn't $uid the same as $user (or at least should be)?

[ghost]

Can one of the admins verify this patch?

[PVince81]

We probably need a different approach for this.
See #16305

[blizzz]

I understand this is to be closed in favor of a modernized solution as required by #16305. The current state would not be mergable also and at least would need a rebase. Please reopen if I should be wrong.

[RobinMcCorkell]

@blizzz This is about replacement of substitution variables in configuration fields, so it's related, but not quite the same. I don't know where we are going to take this exactly though, and as I mentioned above I think the password should never be substituted, so closing is probably the best solution. I'm currently refactoring a lot of this anyway.

[cedric-dufour]

Fine for me.
After giving it much toughts and from a security policies point of view, I can not live with user passwords being stored "in-clear" on the server, whichever the method used to obfuscate it.
The only method I might consider is encrypting the password in the session data, using PHP session ID as key, and making sure to use an ad-hoc session handler (http://php.net/manual/en/session.customhandler.php) such as to make sure the session ID appears nowhere (save for the - hopefully HTTPS-secured - session cookie).
Thanks for consider this anyway.

[RobinMcCorkell]

@cedric-dufour We will implement that, but not by using $password as a substitution variable - the UI issues that creates is too much. The way I'm designing it over at my WIP PR is pluggable authentication mechanisms, so any storage that supports username and password can use a 'Use OC login' method.