Default HTTPS for fed sharing & config option for allowing fallback #30198

tomneedham · 2018-01-19T16:14:23Z

Description

Only uses HTTPS for federated share communication - unless admins explicitly allow HTTP (useful for testing as well)

Related Issue

Potential for users to setup shares via HTTP with unsecure servers without the admin knowing.

Motivation and Context

MITM

How Has This Been Tested?

not yet

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

mmattel · 2018-01-22T12:05:19Z

lib/private/Share/Share.php

@@ -2666,6 +2666,7 @@ public static function removeProtocolFromUrl($url) {
 	 * @return array
 	 */
 	private static function tryHttpPostToShareEndpoint($remoteDomain, $urlSuffix, array $fields) {
+		$allowHttpFallback = \OC::$server->getConfig()->getSystemValue('sharing.federation.allowHttpFallback', 'no') === 'yes';


In config.sample.php above you have as values true/false but here you use yes/no.
Suggesting to use true/false homogeniously.

Thanks @mmattel for the spot. I was originally using appconfig but concluded on config.php so we can use booleans here. Updated the PR to reflect this.

codecov · 2018-01-29T09:24:14Z

Codecov Report

Merging #30198 into master will increase coverage by <.01%.
The diff coverage is 69.23%.

@@             Coverage Diff              @@
##             master   #30198      +/-   ##
============================================
+ Coverage     61.84%   61.84%   +<.01%     
- Complexity    19064    19065       +1     
============================================
  Files          1091     1091              
  Lines         61480    61486       +6     
============================================
+ Hits          38020    38026       +6     
  Misses        23460    23460

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Share/Share.php	`67.12% <69.23%> (+0.13%)`	`508 <0> (+1)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e380896...54b4c02. Read the comment docs.

PVince81 · 2018-02-08T15:48:41Z

Triple CI failure due to bad luck.

Please rebase and solve conflict and be more lucky

tomneedham · 2018-02-09T11:28:06Z

rebased, squashed and resolved conflict - let's wait for CI

PVince81 · 2018-02-13T13:41:35Z

12:49:08 1) Test\Share\ShareTest::testRemoteShareUrlCalls with data set #0 ('admin@localhost', 'localhost', false, false)
12:49:08 Exception: Sharing test.txt failed, could not find admin@localhost, check spelling and server availability.
12:49:08 
12:49:08 /var/lib/jenkins/workspace/owncloud-core_core_PR-30198-OBFZEBOE7XKCZTFDCCEZHK2P56TXDW23OPZENHUDKP2Z245D372Q/lib/private/Share/Share.php:919
12:49:08 /var/lib/jenkins/workspace/owncloud-core_core_PR-30198-OBFZEBOE7XKCZTFDCCEZHK2P56TXDW23OPZENHUDKP2Z245D372Q/lib/public/Share.php:267
12:49:08 /var/lib/jenkins/workspace/owncloud-core_core_PR-30198-OBFZEBOE7XKCZTFDCCEZHK2P56TXDW23OPZENHUDKP2Z245D372Q/tests/lib/Share/ShareTest.php:973
12:49:08 
12:49:08 2) Test\Share\ShareTest::testRemoteShareUrlCalls with data set #1 ('admin@https://localhost', 'localhost', false, false)
12:49:08 Exception: Sharing test.txt failed, could not find admin@https://localhost, check spelling and server availability.
12:49:08 
12:49:08 /var/lib/jenkins/workspace/owncloud-core_core_PR-30198-OBFZEBOE7XKCZTFDCCEZHK2P56TXDW23OPZENHUDKP2Z245D372Q/lib/private/Share/Share.php:919
12:49:08 /var/lib/jenkins/workspace/owncloud-core_core_PR-30198-OBFZEBOE7XKCZTFDCCEZHK2P56TXDW23OPZENHUDKP2Z245D372Q/lib/public/Share.php:267
12:49:08 /var/lib/jenkins/workspace/owncloud-core_core_PR-30198-OBFZEBOE7XKCZTFDCCEZHK2P56TXDW23OPZENHUDKP2Z245D372Q/tests/lib/Share/ShareTest.php:973
12:49:08 
12:49:08 3) Test\Share\ShareTest::testRemoteShareUrlCalls with data set #3 ('admin@localhost/subFolder', 'localhost/subFolder', false, false)
12:49:08 Exception: Sharing test.txt failed, could not find admin@localhost/subFolder, check spelling and server availability.
12:49:08 
12:49:08 /var/lib/jenkins/workspace/owncloud-core_core_PR-30198-OBFZEBOE7XKCZTFDCCEZHK2P56TXDW23OPZENHUDKP2Z245D372Q/lib/private/Share/Share.php:919
12:49:08 /var/lib/jenkins/workspace/owncloud-core_core_PR-30198-OBFZEBOE7XKCZTFDCCEZHK2P56TXDW23OPZENHUDKP2Z245D372Q/lib/public/Share.php:267
12:49:08 /var/lib/jenkins/workspace/owncloud-core_core_PR-30198-OBFZEBOE7XKCZTFDCCEZHK2P56TXDW23OPZENHUDKP2Z245D372Q/tests/lib/Share/ShareTest.php:973
12:49:08 
12:49:08 4) Test\Share\ShareTest::testRemoteShareUrlCalls with data set #4 ('admin@localhost/subFolder', 'localhost/subFolder', false, false)
12:49:08 Exception: Sharing test.txt failed, could not find admin@localhost/subFolder, check spelling and server availability.
12:49:08 
12:49:08 /var/lib/jenkins/workspace/owncloud-core_core_PR-30198-OBFZEBOE7XKCZTFDCCEZHK2P56TXDW23OPZENHUDKP2Z245D372Q/lib/private/Share/Share.php:919
12:49:08 /var/lib/jenkins/workspace/owncloud-core_core_PR-30198-OBFZEBOE7XKCZTFDCCEZHK2P56TXDW23OPZENHUDKP2Z245D372Q/lib/public/Share.php:267
12:49:08 /var/lib/jenkins/workspace/owncloud-core_core_PR-30198-OBFZEBOE7XKCZTFDCCEZHK2P56TXDW23OPZENHUDKP2Z245D372Q/tests/lib/Share/ShareTest.php:973

tomneedham · 2018-02-21T12:05:29Z

Fixed the failing tests - runs fine locally.

PVince81 · 2018-02-26T11:08:19Z

hmm there are still failures:

1) Test\Share\ShareTest::testRemoteShareUrlCalls with data set #0 ('admin@localhost', 'localhost', false, false)
Exception: Sharing test.txt failed, could not find admin@localhost, check spelling and server availability.

/drone/src/lib/private/Share/Share.php:919
/drone/src/lib/public/Share.php:267
/drone/src/tests/lib/Share/ShareTest.php:973

PVince81 · 2018-02-26T11:13:16Z

Running all tests locally with PHP 7.1 and sqlite does not produce this error. I even shut down my local web server in case it was trying to connect there...

Mystery...

Let's rebase this anyway as we have library updates on master

PVince81 · 2018-02-26T11:13:22Z

@ownclouders rebase

ownclouders · 2018-02-26T11:13:24Z

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

…lowing fallback to http

ownclouders · 2018-02-26T11:13:51Z

Automated rebase with GitMate.io was successful! 🎉

PVince81 · 2018-02-26T11:26:48Z

tests still pass locally...

PVince81 · 2018-02-26T13:04:20Z

@tomneedham please backport

phil-davis · 2018-03-21T03:01:15Z

Backport stable10 #30646

PVince81 · 2018-04-19T08:55:52Z

Not working, raised here #31194

lock · 2019-07-31T09:02:40Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

tomneedham added 3 - To Review feature:federated-cloud-sharing app:federation labels Jan 19, 2018

tomneedham added this to the development milestone Jan 19, 2018

tomneedham self-assigned this Jan 19, 2018

tomneedham requested a review from PVince81 January 19, 2018 16:14

tomneedham added the backport-request label Jan 19, 2018

mmattel reviewed Jan 22, 2018

View reviewed changes

tomneedham force-pushed the https-fed-sharing branch from aaa3f16 to 0f63f03 Compare January 29, 2018 09:24

tomneedham force-pushed the https-fed-sharing branch from 3171550 to cf14b4f Compare February 9, 2018 11:27

ownclouders added the status/STALE label Feb 21, 2018

tomneedham force-pushed the https-fed-sharing branch from cf14b4f to 0ee7517 Compare February 21, 2018 12:05

ownclouders removed the status/STALE label Feb 21, 2018

Default to HTTPS only for federated communication - add config for al…

54b4c02

…lowing fallback to http

ownclouders force-pushed the https-fed-sharing branch from 0ee7517 to 54b4c02 Compare February 26, 2018 11:13

PVince81 merged commit 4f0c6df into master Feb 26, 2018

PVince81 deleted the https-fed-sharing branch February 26, 2018 13:04

tomneedham mentioned this pull request Feb 28, 2018

[stable10] Flag to enable http fallback when creating fed shares #30646

Merged

phil-davis removed the backport-request label Mar 21, 2018

PVince81 mentioned this pull request Apr 19, 2018

Federated share http fallback switch not working #31194

Closed

PVince81 mentioned this pull request Apr 25, 2018

Use HTTPS for federated cloud share when another share already uses HTTPS #20216

Closed

ownclouders mentioned this pull request Jul 5, 2018

config option to enable fallback to HTTP for federated shares does not work. #31999

Closed

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default HTTPS for fed sharing & config option for allowing fallback #30198

Default HTTPS for fed sharing & config option for allowing fallback #30198

tomneedham commented Jan 19, 2018

mmattel Jan 22, 2018

tomneedham Jan 29, 2018

codecov bot commented Jan 29, 2018 •

edited

Loading

PVince81 commented Feb 8, 2018

tomneedham commented Feb 9, 2018

PVince81 commented Feb 13, 2018

tomneedham commented Feb 21, 2018

PVince81 commented Feb 26, 2018

PVince81 commented Feb 26, 2018

PVince81 commented Feb 26, 2018

ownclouders commented Feb 26, 2018

ownclouders commented Feb 26, 2018

PVince81 commented Feb 26, 2018

PVince81 commented Feb 26, 2018 •

edited

Loading

phil-davis commented Mar 21, 2018

PVince81 commented Apr 19, 2018

lock bot commented Jul 31, 2019

Default HTTPS for fed sharing & config option for allowing fallback #30198

Default HTTPS for fed sharing & config option for allowing fallback #30198

Conversation

tomneedham commented Jan 19, 2018

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

mmattel Jan 22, 2018

Choose a reason for hiding this comment

tomneedham Jan 29, 2018

Choose a reason for hiding this comment

codecov bot commented Jan 29, 2018 • edited Loading

Codecov Report

PVince81 commented Feb 8, 2018

tomneedham commented Feb 9, 2018

PVince81 commented Feb 13, 2018

tomneedham commented Feb 21, 2018

PVince81 commented Feb 26, 2018

PVince81 commented Feb 26, 2018

PVince81 commented Feb 26, 2018

ownclouders commented Feb 26, 2018

ownclouders commented Feb 26, 2018

PVince81 commented Feb 26, 2018

PVince81 commented Feb 26, 2018 • edited Loading

phil-davis commented Mar 21, 2018

PVince81 commented Apr 19, 2018

lock bot commented Jul 31, 2019

codecov bot commented Jan 29, 2018 •

edited

Loading

PVince81 commented Feb 26, 2018 •

edited

Loading