Fix custominstallerbuilder/issue#16 #173

lukpueh · 2016-06-10T17:20:34Z

Adds big red warning to all web pages when DEBUG option is set to true.
Adds option to chose whether the Clearinghouse should be able to communicate with a Custom Installer Builder via XMLRPC if the Custom Installer Builder uses self-signed certificates.
Option defaults to the same value as DEBUG option.

There are more parts in the Clearinghouse code that use XMLRPC but were not changed because they either don't use SSL or aren't probably used at all:

lukpueh · 2016-06-10T17:26:24Z

website/html/views.py

@@ -22,6 +22,8 @@
 import shutil
 import subprocess
 import xmlrpclib
+import re


This import slipped in from a previous commit (wrong fork, wrong branch, had to cherry pick, resolve conflicts, ...).
Anyways, I actually think it already slipped into the original commit by accident. Because I can't see the re module being used.

So should it be removed?

vladimir-v-diaz · 2017-07-12T19:43:20Z

website/context_processor.py

@@ -9,3 +9,14 @@ def customizable_strings(request):
      "CLEARINGHOUSE_URL": settings.CLEARINGHOUSE_URL,
      "DEMOKIT": settings.DEMOKIT,
 }
+
+def options(request):
+    """Make variables defined in settings.py available to the template engine."""


Is this really making all variables defined in settings.py available to the template engine, or just settings.DEBUG?

vladimir-v-diaz · 2017-07-12T20:04:41Z

website/settings.py

+# This setting provides to chose whether to accept self-signed certificates
+# (e.g. in DEBUG mode) by using an unverified ssl context.
+# c.f. https://github.com/SeattleTestbed/custominstallerbuilder/issues/16
+UNVERIFIED_SSL_CONTEXT = DEBUG


I suppose UNVERIFIED_SSL_CONTEXT can be set to True independent of DEBUG. In that case, I can imagine someone doing so and failing to notice that self-signed certificates are used in production (because they forgot it was set to True).

Consider having some type of warning for when self-signed certificates are accepted by the server.

This is exactly what should be prevented by setting UNVERIFIED_SSL_CONTEXT to DEBUG, i.e.:

Either we are in

DEBUG mode (== accept self-signed certificates) and we aware of it (c.f. warning sign added in this PR), or we are

not in DEBUG mode (== don't accept self-signed certificates).

Also see discussion in SeattleTestbed/custominstallerbuilder#16.

I understand, but the hypothetical situation described is for a user that sets UNVERIFIED_SSL_CONTEXT to True independent of DEBUG. In other words, someone might try to set DEBUG to False, but UNVERIFIED_SSL_CONTEXT to True.

Are you saying that users should not attempt to modify UNVERIFIED_SSL_CONTEXT, and only modify DEBUG? If so, that should documented somewhere.

I think that that's not necessary, since the variable is currently tied to the debug mode and shouldn't really be set independently anyway.

Why have a separate UNVERIFIED_SSL_CONTEXT setting? Perhaps you should make it so that DEBUG results in the ability to use unverified SSL contexts.

Someone might try to modify if you don't at least explain that it shouldn't be independently modified.

I proposed the separate setting mostly for transparency. We never instruct a ch operator to set this to a different value than DEBUG and I can't see why a ch operator would want to do this, or would do this by accident.

vladimir-v-diaz · 2017-07-12T20:16:48Z

website/html/views.py

+    if settings.UNVERIFIED_SSL_CONTEXT:
+      xmlrpc_proxy = xmlrpclib.ServerProxy(
+        settings.SEATTLECLEARINGHOUSE_INSTALLER_BUILDER_XMLRPC, 
+        context=ssl._create_unverified_context())


Isn't this function only available in Python version 2.7+? What if a user is using version <2.7? Should there be checks for Python versions in the code, or some kind of disclaimer?

AFAIK we agreed that we don't care for Python <2.7 to run clearinghouse (c.f. discussion in SeattleTestbed/custominstallerbuilder#16).

As for the disclaimer, our main instruction to run the clearinghouse already suggests to use Python 2.7

Python 2.6 is plainly unsupported for running a clearinghouse as of the docs change.

…signed certificates

The modified context_processor should not make the string, but the value of `settings.DEBUG` available to the template engine.

lukpueh · 2017-08-16T18:17:26Z

Thanks for reviewing @vladimir-v-diaz and @JustinCappos! I have addressed your comments and also fixed a small but important bug in my code. Let me know if we can merge.

@vladimir-v-diaz

Following @vladimir-v-diaz's review comment this change makes the docstring and comments of the custom options context_processor clearer.

This import slipped in from a previous commit (wrong fork, wrong branch, had to cherry pick, resolve conflicts, ...) Anyway, I actually think it already slipped into the original commit by accident: z3r0w0n@74fad83

vladimir-v-diaz · 2017-08-28T16:08:20Z

Feel free to merge!

@vladimir-v-diaz

Following a review discussion with @vladimir-v-diaz [1] we add an extra warning comment to the `UNVERIFIED_SSL_CONTEXT` setting to prevent ch/cib operators from accidentely not verifying the ssl context in production. [1] SeattleTestbed#173 (comment)

lukpueh reviewed Jun 10, 2016
View reviewed changes

vladimir-v-diaz requested changes Jul 12, 2017

View reviewed changes

lukpueh and others added 3 commits August 16, 2017 12:43

Adds setting UNVERIFIED_SSL_CONTEXT to be able to call cib with self …

0a52325

…signed certificates

Adds 'you are in debug mode' warning to web page

3e82dd0

Fix return string instead of value bug

710a07d

The modified context_processor should not make the string, but the value of `settings.DEBUG` available to the template engine.

lukpueh force-pushed the fix-ssl-verify branch from b7947a0 to dce6ce9 Compare August 16, 2017 16:46

lukpueh added 2 commits August 16, 2017 14:19

Enhance docstring/comments in context_processor

0479289

Following @vladimir-v-diaz's review comment this change makes the docstring and comments of the custom options context_processor clearer.

Remove obsolete re import in html/views.py

be7516d

This import slipped in from a previous commit (wrong fork, wrong branch, had to cherry pick, resolve conflicts, ...) Anyway, I actually think it already slipped into the original commit by accident: z3r0w0n@74fad83

lukpueh force-pushed the fix-ssl-verify branch from 7b52e14 to be7516d Compare August 16, 2017 18:19

Rephrase comment for setting

9eb7b6a

Following a review discussion with @vladimir-v-diaz [1] we add an extra warning comment to the `UNVERIFIED_SSL_CONTEXT` setting to prevent ch/cib operators from accidentely not verifying the ssl context in production. [1] SeattleTestbed#173 (comment)

lukpueh merged commit 3f84cde into SeattleTestbed:master Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix custominstallerbuilder/issue#16 #173

Fix custominstallerbuilder/issue#16 #173

lukpueh commented Jun 10, 2016

lukpueh Jun 10, 2016

JustinCappos Jun 23, 2017

vladimir-v-diaz Jul 12, 2017

vladimir-v-diaz Jul 12, 2017

lukpueh Aug 16, 2017 •

edited

Loading

vladimir-v-diaz Aug 24, 2017

lukpueh Aug 28, 2017

vladimir-v-diaz Aug 28, 2017

lukpueh Aug 28, 2017

vladimir-v-diaz Jul 12, 2017

lukpueh Aug 16, 2017

aaaaalbert Aug 28, 2017

lukpueh commented Aug 16, 2017

vladimir-v-diaz commented Aug 28, 2017

Fix custominstallerbuilder/issue#16 #173

Fix custominstallerbuilder/issue#16 #173

Conversation

lukpueh commented Jun 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh commented Aug 16, 2017

vladimir-v-diaz commented Aug 28, 2017

lukpueh Aug 16, 2017 •

edited

Loading