Enable secure cookies #6868

rzig · 2017-06-20T17:12:30Z

This will enable secure cookies automatically over HTTPS and has a line in config.default.php that can be uncommented to enable secure session cookies.

Would you like me to add any documentation on the line in config.default.php?

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

fixes: #6866

Enables secure cookies for authentication when HTTPS is used.

CLAassistant · 2017-06-20T17:12:37Z

All committers have signed the CLA.

LibreNMS-CI · 2017-06-20T17:15:28Z

Auto-Deploy finished, Test PR at http://6868.ci.librenms.org or https://6868.ci.librenms.org

murrant · 2017-06-20T17:18:46Z

config.php.default

@@ -49,3 +49,6 @@ $config['enable_billing'] = 1;

 # Enable the in-built services support (Nagios plugins)
 $config['show_services'] = 1;
+
+# Uncomment the following line if you're using HTTPS and want secure session cookies
+# ini_set('session.cookie_secure', 1);


Don't put code in this file, only config definitions.
You can move this to init.php

Also, any reason not to do this by default? Does it break setups that don't have http?

Commented out because having this enabled will prevent the server from sending cookies over HTTPS.

LibreNMS-CI · 2017-06-20T17:32:49Z

Auto-Deploy finished, Test PR at http://6868.ci.librenms.org or https://6868.ci.librenms.org

LibreNMS-CI · 2017-06-20T17:35:46Z

Auto-Deploy finished, Test PR at http://6868.ci.librenms.org or https://6868.ci.librenms.org

rzig · 2017-06-20T17:57:24Z

@murrant, the Travis CI build failed, but I don't think it is as a result of my code as the lines I added were commented out and the error related to a syslog test that appears to be a result of the previous merge.

laf · 2017-06-20T18:29:47Z

html/includes/authentication/functions.php

-    setcookie('sess_id', $sess_id, $expiration, '/', null, false, true);
-    setcookie('token', $token_id, $expiration, '/', null, false, true);
-    setcookie('auth', $auth, $expiration, '/', null, false, true);
+    // Params: name, value, expire, path, domain, secure, httponly


you can get rid of all of these comments.

laf · 2017-06-20T18:31:14Z

html/includes/authentication/functions.php

+    // Params: name, value, expire, path, domain, secure, httponly
+    // We set the secure param to $_SERVER["HTTPS"], which will return true
+    // and make it a secure cookie if HTTPS is being used
+    setcookie('sess_id', $sess_id, $expiration, '/', null, $_SERVER["HTTPS"], true);


I'm not really sure we should just be basing it on if the user is connecting over https or not as surely that's what we have now.

Would it not be better to have the user set in config.php if the connection should be secure and use that variable here?

I'll set an option in config.php.

laf · 2017-06-20T18:52:18Z

includes/init.php

@@ -90,6 +90,9 @@
 require $install_dir . '/includes/definitions.inc.php';
 include $install_dir . '/config.php';

+# Uncomment the following line if you're using HTTPS and want secure session cookies


Please use // instead of # just to be consistent.

We should use the same config variable we would use in setcookie() to also run this ini_set() which would be better placed in includes/process_config.inc.php

LibreNMS-CI · 2017-06-20T19:23:38Z

Auto-Deploy finished, Test PR at http://6868.ci.librenms.org or https://6868.ci.librenms.org

LibreNMS-CI · 2017-06-20T19:27:30Z

Auto-Deploy finished, Test PR at http://6868.ci.librenms.org or https://6868.ci.librenms.org

LibreNMS-CI · 2017-06-20T19:34:14Z

Auto-Deploy finished, Test PR at http://6868.ci.librenms.org or https://6868.ci.librenms.org

LibreNMS-CI · 2017-06-20T19:37:46Z

Auto-Deploy finished, Test PR at http://6868.ci.librenms.org or https://6868.ci.librenms.org

LibreNMS-CI · 2017-06-20T19:43:11Z

Auto-Deploy finished, Test PR at http://6868.ci.librenms.org or https://6868.ci.librenms.org

LibreNMS-CI · 2017-06-20T19:49:41Z

Auto-Deploy finished, Test PR at http://6868.ci.librenms.org or https://6868.ci.librenms.org

LibreNMS-CI · 2017-06-20T19:57:44Z

Auto-Deploy finished, Test PR at http://6868.ci.librenms.org or https://6868.ci.librenms.org

murrant · 2017-06-21T01:38:09Z

I've pushed a fix for the travis failures, you'll have to merge it to fix your build failure.

rzig · 2017-06-22T17:38:55Z

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Have you followed our code guidelines?

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

Forgot to include this earlier, sorry.

murrant · 2017-06-23T04:02:02Z

config.php.default

@@ -49,3 +49,6 @@ $config['enable_billing'] = 1;

 # Enable the in-built services support (Nagios plugins)
 $config['show_services'] = 1;
+
+# Whether to use secure cookies. Should be true if you're using HTTPS
+$config['secure_cookies'] = false;


Lets move this to includes/defaults.inc.php and then this looks good to go.

Thanks for discussing in depth :)

…curecookies

rzig · 2017-06-23T15:23:56Z

There's a bit of an issue. When I try and apply the patch it says error: patch failed: includes/process_config.inc.php:36. I'm thinking it has something to do with whitespace, but I don't see anything out of the ordinary. Any ideas?

rzig · 2017-06-23T16:57:50Z

The git issue is fixed - it still complains about whitespace, but it applies the patches.

scrutinizer-notifier · 2017-06-23T20:15:00Z

The inspection completed: No new issues

laf · 2017-06-23T21:14:43Z

LGTM

lock · 2018-05-17T22:03:47Z

This thread has been automatically locked since there has not been any recent activity after it was closed.

rzig added 3 commits June 20, 2017 12:57

Enable secure cookies for authentication

01f76d4

Enables secure cookies for authentication when HTTPS is used.

Add line with example secure session cookies

48432aa

Fix comment line that's too long

73a38ed

murrant reviewed Jun 20, 2017

View reviewed changes

rzig added 2 commits June 20, 2017 13:29

Remove session secure cookie config

ab647f9

Added secure cookie config to init.php

f4a2577

Commented out because having this enabled will prevent the server from sending cookies over HTTPS.

laf requested changes Jun 20, 2017

View reviewed changes

rzig added 2 commits June 20, 2017 15:20

Enable secure cookies if config value provided

b3b23ca

Make code formatting consistent

f75a118

Remove secure cookie config

066386a

rzig added 2 commits June 20, 2017 15:28

Use $config['secure_cookies'] to set cookies

54147b3

Add default option to turn secure cookies on

8634e7b

rzig added 2 commits June 20, 2017 15:41

Fix formatting issues

a006f5c

Set secure cookies to default to false

7a7f085

rzig and others added 2 commits June 21, 2017 10:34

Fast forward

0f2075d

Add info about secure cookies

d2dd740

rzig mentioned this pull request Jun 22, 2017

Secure cookies not enabled #6866

Closed

rzig added 2 commits June 22, 2017 13:42

Fix formatting

f0b30ff

Remove trailing whitespace

2050ab3

murrant requested changes Jun 23, 2017

View reviewed changes

rzig added 2 commits June 23, 2017 11:11

move secure cookie default config to defaults.inc.php

6a712c7

Merge branch 'master' of https://github.com/librenms/librenms into se…

0c77e8d

…curecookies

rzig added 3 commits June 23, 2017 11:41

Remove trailing whitespace

4777dfb

Fix formatting

df46091

Remove empty line by rrd config

6c501a6

Remove whitespace

460db84

laf approved these changes Jun 23, 2017

View reviewed changes

murrant approved these changes Jun 25, 2017

View reviewed changes

laf merged commit 1ba6381 into librenms:master Jun 26, 2017

lock bot locked as resolved and limited conversation to collaborators May 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable secure cookies #6868

Enable secure cookies #6868

rzig commented Jun 20, 2017 •

edited by murrant

Loading

CLAassistant commented Jun 20, 2017 •

edited

Loading

LibreNMS-CI commented Jun 20, 2017

murrant Jun 20, 2017 •

edited

Loading

LibreNMS-CI commented Jun 20, 2017

LibreNMS-CI commented Jun 20, 2017

rzig commented Jun 20, 2017 •

edited

Loading

laf Jun 20, 2017

laf Jun 20, 2017

rzig Jun 20, 2017 •

edited

Loading

laf Jun 20, 2017

LibreNMS-CI commented Jun 20, 2017

LibreNMS-CI commented Jun 20, 2017

LibreNMS-CI commented Jun 20, 2017

LibreNMS-CI commented Jun 20, 2017

LibreNMS-CI commented Jun 20, 2017

LibreNMS-CI commented Jun 20, 2017

LibreNMS-CI commented Jun 20, 2017

murrant commented Jun 21, 2017

rzig commented Jun 22, 2017

murrant Jun 23, 2017

rzig commented Jun 23, 2017

rzig commented Jun 23, 2017

scrutinizer-notifier commented Jun 23, 2017

laf commented Jun 23, 2017

lock bot commented May 17, 2018

Enable secure cookies #6868

Enable secure cookies #6868

Conversation

rzig commented Jun 20, 2017 • edited by murrant Loading

Testers

CLAassistant commented Jun 20, 2017 • edited Loading

LibreNMS-CI commented Jun 20, 2017

murrant Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

LibreNMS-CI commented Jun 20, 2017

LibreNMS-CI commented Jun 20, 2017

rzig commented Jun 20, 2017 • edited Loading

laf Jun 20, 2017

Choose a reason for hiding this comment

laf Jun 20, 2017

Choose a reason for hiding this comment

rzig Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

laf Jun 20, 2017

Choose a reason for hiding this comment

LibreNMS-CI commented Jun 20, 2017

LibreNMS-CI commented Jun 20, 2017

LibreNMS-CI commented Jun 20, 2017

LibreNMS-CI commented Jun 20, 2017

LibreNMS-CI commented Jun 20, 2017

LibreNMS-CI commented Jun 20, 2017

LibreNMS-CI commented Jun 20, 2017

murrant commented Jun 21, 2017

rzig commented Jun 22, 2017

Please note

Testers

murrant Jun 23, 2017

Choose a reason for hiding this comment

rzig commented Jun 23, 2017

rzig commented Jun 23, 2017

scrutinizer-notifier commented Jun 23, 2017

laf commented Jun 23, 2017

lock bot commented May 17, 2018

rzig commented Jun 20, 2017 •

edited by murrant

Loading

CLAassistant commented Jun 20, 2017 •

edited

Loading

murrant Jun 20, 2017 •

edited

Loading

rzig commented Jun 20, 2017 •

edited

Loading

rzig Jun 20, 2017 •

edited

Loading