Skip to content

Conversation

@icewind1991
Copy link
Contributor

This moves the logic for managing trusted certificates from files_external to core, note that the certs are still stored in the files_external folder for backwards compatibilty, this is transparent for any app using the api.

To test:

  • try adding a remote share from a server with a self-signed certificate, this should fail
  • import the self-signed cert trough settings->personal
  • try adding the remote share again, now it should work

Next step is making a nice gui for trusting the certificate when trying to add an external share from an untrusted cert

cc @schiesbn @PVince81 @simsasaile

@icewind1991 icewind1991 added this to the 2014-sprint-01-current milestone Aug 14, 2014
@icewind1991
Copy link
Contributor Author

Fixes #9786

@karlitschek
Copy link
Contributor

looks good. 👍
@LukasReschke @schiesbn What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openssl_pkey_get_public is not a proper way to check whether a certificate is valid. I'll hijack this PR later to adjust this.

@LukasReschke
Copy link
Member

In my opinion we should also have a system wide certificate manager and not only one per user. Opinions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we move this to out of files_external and change this to core?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was planning that for a future pr, together with ajaxifying the add cert setting

@icewind1991
Copy link
Contributor Author

Added checks for filename and changed directory permissions

@icewind1991
Copy link
Contributor Author

In my opinion we should also have a system wide certificate manager and not only one per user. Opinions?

Agree, just wanted to keep this PR small

@LukasReschke
Copy link
Member

I'll add unit unit tests and correct the certificate validity check later. Please don't merge yet.

@schiessle
Copy link
Contributor

In my opinion we should also have a system wide certificate manager and not only one per user. Opinions?

Not sure. On a system-wide level the admin can add the trusted root certs to the certificate bundle of his operating system. Users who want to trust their self-signed cert can't do it, that's why we provide the UI for them.

@LukasReschke
Copy link
Member

On a system-wide level the admin can add the trusted root certs to the certificate bundle of his operating system.

We have to consider shared hosting environments etc.

@LukasReschke
Copy link
Member

Also it's easier to tell an admin "upload your certificate at the admin settings" than telling them to investigate how their OS stores certificates :-)

@icewind1991
Copy link
Contributor Author

Moved the certificate manager interface to core and improved it, it now shows the common name, expire date and issuer for each certificate and expired certificates are highlighted in red.
Making it a lot easier to see what certs are trusted

@icewind1991
Copy link
Contributor Author

@jancborchardt can you check if the interface is up to standard?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please typehint the object you want __construct(IUser $user)

@icewind1991
Copy link
Contributor Author

Forgot to commit some files...

@th3fallen
Copy link
Contributor

@icewind1991 why not use this since we already have this loaded http://phpseclib.sourceforge.net/x509/intro.html instead of hand writing all of it in certificate.php?

@icewind1991 icewind1991 force-pushed the external-share-self-signed branch from 893905f to 4bc9980 Compare August 31, 2014 08:52
@scrutinizer-notifier
Copy link

A new inspection was created.

@ghost
Copy link

ghost commented Aug 31, 2014

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

LukasReschke added a commit that referenced this pull request Aug 31, 2014
Make external shares work with imported self signed certificates
@LukasReschke LukasReschke merged commit 8009df0 into master Aug 31, 2014
@LukasReschke LukasReschke deleted the external-share-self-signed branch August 31, 2014 13:50
@PVince81
Copy link
Contributor

PVince81 commented Sep 5, 2014

I don't think we'll want to backport such a huge change (CC @karlitschek).
Is there a possible workaround for people on OC 7 ?

@karlitschek
Copy link
Contributor

This would be a helpful feature but I agree that it is too risky for backport

@craigpg craigpg added Type:Bug and removed Type:Bug labels Sep 9, 2014
@PVince81
Copy link
Contributor

@icewind1991 is there a simpler solution / workaround for OC 7 to make it work ?

@carlaschroder
Copy link

Needs documenting for oc7 that self-signed certs won't work with mounting remote shares. It's a showstopper for S2S sharing.

@godfuture
Copy link

Is it somehow possible to completely turn off certificate verification for s2s? I have imported the self signed cert of the source server, but external storage connection is not established anyway:

[CURL] Error while making request: SSL certificate problem: unable to get local issuer certificate (error code: 60) 

problem_with_ssc_s2s_oc

Source server: oc 7.0.4, iis 7.5 win 7 x64, php 5.5.15
Target server: oc 8.0.0, apache2, ubuntu 14.10, php 5.5.12

@DeepDiver1975
Copy link
Member

@LukasReschke anything we can do about this? THX

@DeepDiver1975
Copy link
Member

@godfuture Can I ask you to open a new issue? THX

@LukasReschke
Copy link
Member

Can't help without a copy of the certificate ;)

@godfuture
Copy link

@DeepDiver1975
Sure, #14747.
@LukasReschke
Have sent you an email with the cert attached. Hope this is okay!

@lock lock bot locked as resolved and limited conversation to collaborators Aug 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.