Add SFTP public key authentication support #13190

rnicoll · 2015-01-09T11:07:21Z

Adds a new SFTP external file plugin which uses public key based authentication instead of passwords.

A number of private fields/functions in the existing SFTP class have been made protected in order to make extension of that class feasible, as part of this work.

When adding a new external file store, the first field is now automatically selected. This is a minor UI improvement, but also caught by the SFTP key UI in order to trigger key pair generation.

ghost · 2015-01-09T11:08:03Z

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/

rnicoll · 2015-01-09T11:09:08Z

Signed contributor agreement went to Frank Karlitschek on Wednesday 7th Jan 2015, and I believe is being progressed.

LukasReschke · 2015-01-09T11:09:53Z

apps/files_external/ajax/sftp_key.php

Can I ask you to use the AppFramework for this new AJAX file?
http://doc.owncloud.org/server/7.0/developer_manual/app/index.html

What would be needed here is:

Create an application.php for the files_external app

Migrate this to a controller

Add a route

Add some unit tests

LukasReschke · 2015-01-09T11:21:33Z

@icewind1991 @bantu THX - your expertise is requested here.

LukasReschke · 2015-01-09T11:22:11Z

apps/files_external/appinfo/app.php

More or less a direct cut & paste from the SFTP plugin definition above. Will test and find out what type is coming back.

Please change:
'root' => '&'.$l->t('Root')) --> 'root' => '&'.$l->t('Remote subfolder'))
as we have harmonized the terms used recently

Please do not forget to document this in:
https://github.com/owncloud/documentation/blob/master/admin_manual/configuration/external_storage_configuration.rst
https://github.com/owncloud/documentation/blob/master/admin_manual/configuration/external_storage_configuration_gui.rst

The (string) thing was introduced by @wakeup - perhaps he could shed some light on it?

LukasReschke · 2015-01-09T11:29:31Z

@owncloud-bot This is okay to test.

LukasReschke · 2015-01-09T11:31:01Z

@rnicoll THX for your contribution. We're currently in code-freeze for 8.0 this month so this code can get merged earliest next month.

Let me know if you need any help to address the comments that I made. - Using the AppFramework here would allow us to test this code instead of an untestable file.

LukasReschke · 2015-01-09T11:31:49Z

Please also take a look at the things @scrutinizer-notifier reported.

rnicoll · 2015-01-09T17:47:37Z

Still not done with the cleanup, just pushing updates so you can see progress. Should be much closer now though.

vgezer · 2015-01-09T20:41:07Z

apps/files_external/appinfo/app.php

FYI: @rnicoll @Xenopathic casting to string was necessary to make the strings visible. This is the correct way though. If there are other chars like * or & no casting is necessary.

rnicoll · 2015-01-12T15:04:27Z

Squashed the whole lot down, that should be everything but the tests done now.

rnicoll · 2015-01-13T17:05:44Z

Barring two tests (testHashInFileName and testRenameOverWriteDirectory) which also fail with the existing SFTP plugin (and I think are therefore more about how my server works, than anything else), all unit tests pass. Any other changes required?

RobinMcCorkell · 2015-01-13T22:11:44Z

Looks good! I'll be testing this at some point, then I can give my review mark 😄

LukasReschke · 2015-01-13T22:16:03Z

@karlitschek Please verify #13190 (comment). - THX.

karlitschek · 2015-01-13T22:47:34Z

@LukasReschke Confirmed :-)

RobinMcCorkell · 2015-01-14T09:00:36Z

Neat! I like the auto-generation of SSH keys, it makes it really simple to use. Only issue is a minor problem with the position of the 'Generate keys' button: it drops underneath the divider line and looks a bit odd:

It seems that the button is an <a class="button"> element, while it should be an <input type="button">, as it then gets the CSS necessary to put it inline with the other elements.

Other than that though, it's looking great!

Add support for external files accessed via SFTP using public key exchange authentication. Keys are generated automatically when the configuration is added, or can be regenerated on demand if a key is compromised. Creation of a new configuration row now triggers focus on that row. This is used to trigger auto-configuration for SFTP keys. Generated public keys are saved in user's data directory for easy retrieval by an external application. Add controller for SFTP key generation AJAX SFTP class initialisation no longer produces a warning if the password field is missing. Add unit tests for SFTP with key authentication backend

rnicoll · 2015-01-14T17:02:06Z

Done. I'll update Google and Dropbox plugins to match in a moment.

RobinMcCorkell · 2015-01-14T18:58:38Z

Nice! 👍 Now we wait for feature merges to be accepted again

scrutinizer-notifier · 2015-01-14T19:39:06Z

The inspection completed: 26 new issues, 30 updated code elements

PVince81 · 2015-02-09T12:13:49Z

@owncloud-bot ok to test

PVince81 · 2015-02-09T12:21:22Z

You might want to rebase on top of master in case this PR needs the styling fixes from #13364 which was merged on master

ghost · 2015-02-09T12:52:02Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9031/
Test PASSed.

PVince81 · 2015-02-10T12:25:20Z

Works 👍

But I wonder about the private key which is stored in plain text inside mount.json.

Is it worth encrypting the private key like we did for passwords there ? CC @LukasReschke
This could be done simply by renaming the field "private_key" to "password" or adding the field name here: https://github.com/owncloud/core/blob/master/apps/files_external/lib/config.php#L773 and here https://github.com/owncloud/core/blob/master/apps/files_external/lib/config.php#L790

PVince81 · 2015-02-10T15:44:25Z

I'm merging this now as we need the AjaxController for other stuff.

The "private_key" encryption can be submit as a separate PR, if wanted.

Thanks a lot @rnicoll !

Add SFTP public key authentication support

rnicoll · 2015-02-11T20:39:45Z

Private key encryption occurs at https://github.com/owncloud/core/pull/13190/files#diff-fc861a6589e68fe5eaca28c5e2f03296R23 based on the system's secret value. Please do let me know if there's any problems there.

PVince81 · 2015-02-12T08:34:45Z

@rnicoll ah ok. I wasn't sure because when I looked at "mount.json" I saw the block "--- PRIVATE KEY BEGIN ---" or something. It looked like it was just the plain key.
If it's encrypted then it's fine.

LukasReschke reviewed Jan 9, 2015
View reviewed changes

LukasReschke added this to the 8.1-next milestone Jan 9, 2015

LukasReschke reviewed Jan 9, 2015
View reviewed changes

vgezer reviewed Jan 9, 2015
View reviewed changes

rnicoll force-pushed the master-sftp-key branch 2 times, most recently from 4fb08e0 to f7d79e3 Compare January 12, 2015 15:03

rnicoll force-pushed the master-sftp-key branch from 06cf347 to 64f4f8f Compare January 14, 2015 17:01

rnicoll mentioned this pull request Jan 14, 2015

Clean up files_external #13364

Merged

RobinMcCorkell mentioned this pull request Jan 20, 2015

Async external storage status checking #13515

Closed

PVince81 pushed a commit that referenced this pull request Feb 10, 2015

Merge pull request #13190 from is-apps/master-sftp-key

bd01ff1

Add SFTP public key authentication support

PVince81 merged commit bd01ff1 into owncloud:master Feb 10, 2015

This was referenced Feb 10, 2015

SFTP: private-key-auth instead of password #9182

Closed

enabled sftp private key support (instead of password) #9198

Closed

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

Add SFTP public key authentication support #13190

Add SFTP public key authentication support #13190

Uh oh!

Conversation

rnicoll commented Jan 9, 2015

Uh oh!

ghost commented Jan 9, 2015

Uh oh!

rnicoll commented Jan 9, 2015

Uh oh!

LukasReschke Jan 9, 2015

Choose a reason for hiding this comment

Uh oh!

LukasReschke Jan 9, 2015

Choose a reason for hiding this comment

Uh oh!

LukasReschke commented Jan 9, 2015

Uh oh!

LukasReschke Jan 9, 2015

Choose a reason for hiding this comment

Uh oh!

rnicoll Jan 9, 2015

Choose a reason for hiding this comment

Uh oh!

mmattel Jan 9, 2015

Choose a reason for hiding this comment

Uh oh!

RobinMcCorkell Jan 9, 2015

Choose a reason for hiding this comment

Uh oh!

LukasReschke commented Jan 9, 2015

Uh oh!

LukasReschke commented Jan 9, 2015

Uh oh!

LukasReschke commented Jan 9, 2015

Uh oh!

rnicoll commented Jan 9, 2015

Uh oh!

vgezer Jan 9, 2015

Choose a reason for hiding this comment

Uh oh!

rnicoll commented Jan 12, 2015

Uh oh!

rnicoll commented Jan 13, 2015

Uh oh!

RobinMcCorkell commented Jan 13, 2015

Uh oh!

LukasReschke commented Jan 13, 2015

Uh oh!

karlitschek commented Jan 13, 2015

Uh oh!

RobinMcCorkell commented Jan 14, 2015

Uh oh!

rnicoll commented Jan 14, 2015

Uh oh!

RobinMcCorkell commented Jan 14, 2015

Uh oh!

scrutinizer-notifier commented Jan 14, 2015

Uh oh!

PVince81 commented Feb 9, 2015

Uh oh!

PVince81 commented Feb 9, 2015

Uh oh!

ghost commented Feb 9, 2015

Uh oh!

PVince81 commented Feb 10, 2015

Uh oh!

PVince81 commented Feb 10, 2015

Uh oh!

rnicoll commented Feb 11, 2015

Uh oh!

PVince81 commented Feb 12, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!