Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

system check for forced SSL connection shouldn't warn behind reverse proxy #13374

Closed
Findus23 opened this issue Aug 31, 2018 · 8 comments
Closed

system check for forced SSL connection shouldn't warn behind reverse proxy #13374

Findus23 opened this issue Aug 31, 2018 · 8 comments
Assignees
@geekdenz
Labels
c: Documentation For issues related to in-app product help messages, or to the Matomo knowledge base. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Milestone
4.5.0

Comments

@Findus23
Copy link
Member

followup to #13193 and reported on the forum:
https://forum.matomo.org/t/matomo-behind-reverse-proxy-and-force-ssl-setting/29672

When assume_secure_protocol=1 is already set, Matomo shouldn't complain to setup SSL.
@cpoetter
Copy link

I am seeing the same bug. Is there a timeline for a fix?

@Findus23 Findus23 added Bug For errors / faults / flaws / inconsistencies etc. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. labels Apr 11, 2019
@mattab mattab added this to the 3.12.0 milestone Jun 18, 2019
@tsteur tsteur modified the milestones: 3.13.0, 3.14.0 Nov 5, 2019
@mattab mattab modified the milestones: 3.14.0, 4.1.0 Nov 6, 2019
@Findus23
Copy link
Member Author

Two years later I was again thinking about this and I'm not 100% sure if the current status isn't correct. If I am not mistaken force_ssl=1 tells Matomo that it is set up via HTTPS and it therefore should use only HTTPS URLs, secure cookies, etc.

assume_secure_protocol=1 alone does not force that, which means that if you use Matomo behind a reverse proxy that adds SSL, you still need to add force_ssl=1 to get secure cookies (as the system check reminds)

There is even a function in Matomo that checks this case:

matomo/core/Url.php

Lines 711 to 723 in 1155273

/**
* @return bool
*/
public static function isSecureConnectionAssumedByPiwikButNotForcedYet()
{
$isSecureConnectionLikelyNotUsed = Url::isSecureConnectionLikelyNotUsed();
$hasSessionCookieSecureFlag = ProxyHttp::isHttps();
$isSecureConnectionAssumedByPiwikButNotForcedYet = Url::isPiwikConfiguredToAssumeSecureConnection() && !SettingsPiwik::isHttpsForced();
return $isSecureConnectionLikelyNotUsed
&& $hasSessionCookieSecureFlag
&& $isSecureConnectionAssumedByPiwikButNotForcedYet;
}

Of course one could argue that assume_secure_protocol=1 should always automatically set force_ssl=1, but I am also not sure about that as that would make the meaning of force_ssl confusing.

In case I am wrong, changing the check is simply a matter of modifying this:

matomo/plugins/Diagnostics/Diagnostic/ForceSSLCheck.php

Lines 48 to 52 in b16a791

$forceSSLEnabled = (Config::getInstance()->General['force_ssl'] == 1);
if ($forceSSLEnabled) {
return array(DiagnosticResult::singleResult($label, DiagnosticResult::STATUS_OK));
}

@mattab mattab modified the milestones: 4.1.0, 4.2.0 Dec 21, 2020
@mattab mattab modified the milestones: 4.2.0, 4.3.0 Feb 22, 2021
@mattab mattab modified the milestones: 4.3.0, 4.4.0 May 26, 2021
@mattab mattab modified the milestones: 4.4.0, 4.5.0 Jul 28, 2021
@geekdenz geekdenz self-assigned this Aug 5, 2021
@tsteur
Copy link
Member

tsteur commented Aug 6, 2021

When assume_secure_protocol=1 is configured, then this system check should not complain.

geekdenz pushed a commit that referenced this issue Aug 6, 2021
relax force ssl if assuming secure protocol #13374
40e9efc
@sgiehl sgiehl linked a pull request Aug 6, 2021 that will close this issue
relax force ssl if assuming secure protocol #13374 #17861
Closed
10 tasks
geekdenz pushed a commit that referenced this issue Aug 6, 2021
avoid error ignoring on always defined config variables #13374
8ee3fba
@Findus23 Findus23 mentioned this issue Aug 6, 2021
Closed
10 tasks
@tsteur tsteur added c: Documentation For issues related to in-app product help messages, or to the Matomo knowledge base. and removed Bug For errors / faults / flaws / inconsistencies etc. labels Aug 8, 2021
@tsteur
Copy link
Member

tsteur commented Aug 8, 2021

As suggested in #17861 we keep the system check as it is and instead adjust the documentation and mention that we suggest to also set force_ssl=1 when someone configures assume secure protocol for security to make sure any HTTP request is redirected to HTTPS.

This is to be done in https://matomo.org/faq/how-to-install/faq_98/ and could also mention it in https://matomo.org/faq/how-to-install/

geekdenz pushed a commit that referenced this issue Aug 8, 2021
solidify Config::getBool() with unit test #13374
3aa6550
geekdenz pushed a commit that referenced this issue Aug 8, 2021
leave logic as was but introduce new method: Config::getBool() #13374
7de2f75
@geekdenz
Copy link
Contributor

geekdenz commented Aug 9, 2021

This is to be done in https://matomo.org/faq/how-to-install/faq_98/ and could also mention it in https://matomo.org/faq/how-to-install/

OK, added it:
https://matomo.org/faq/how-to-install/faq_98/
and it automatically got added to
https://matomo.org/faq/how-to-install/

@tsteur
Copy link
Member

tsteur commented Aug 9, 2021

Nice @geekdenz 👍 Maybe be good to mention that it will cause HTTP requests to be redirected to HTTPS. And could be good to put in a link to https://matomo.org/faq/how-to/faq_91/ to get more information if needed.

@geekdenz
Copy link
Contributor

geekdenz commented Aug 9, 2021

put in a link to https://matomo.org/faq/how-to/faq_91/ to get more information if needed.

done

@tsteur
Copy link
Member

tsteur commented Aug 9, 2021

Great 👍 💯

@tsteur tsteur closed this as completed Aug 9, 2021
@tsteur tsteur added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Aug 9, 2021
geekdenz pushed a commit that referenced this issue Aug 10, 2021
ensure all possible true settings' values are reflected and documente…
92cf2b6 
…d in tests #13374
geekdenz pushed a commit that referenced this issue Aug 11, 2021
show developers how boolean settings should be processed and what tru…
47bea79 
…e means in config.ini.php files #13374
geekdenz pushed a commit that referenced this issue Aug 15, 2021
remove edge cases and exception handling fixes #13374
d0f2db3
geekdenz pushed a commit that referenced this issue Aug 18, 2021
beautify test cases with @dataProvider #13374
0677edf
sgiehl added a commit that referenced this issue Aug 19, 2021
@sgiehl
Test Config::getBool() for clarity of configuration options (#17865)
9d26506 
* relax force ssl if assuming secure protocol #13374

* avoid error ignoring on always defined config variables #13374

* solidify Config::getBool() with unit test #13374

* leave logic as was but introduce new method: Config::getBool() #13374

* ensure all possible true settings' values are reflected and documented in tests #13374

* show developers how boolean settings should be processed and what true means in config.ini.php files #13374

* remove edge cases and exception handling fixes #13374

* Update tests/PHPUnit/Unit/Config/ConfigTest.php

Co-authored-by: Stefan Giehl <stefan@matomo.org>

* beautify test cases with @dataProvider #13374

* Apply suggestions from code review

Co-authored-by: Stefan Giehl <stefan@matomo.org>
@jorgeuos jorgeuos mentioned this issue Jan 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@geekdenz geekdenz

Labels
c: Documentation For issues related to in-app product help messages, or to the Matomo knowledge base. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.5.0
Development

Successfully merging a pull request may close this issue.

relax force ssl if assuming secure protocol #13374 matomo-org/matomo
5 participants
@tsteur @mattab @geekdenz @Findus23 @cpoetter