Merge ~cjwatson/squid-reverseproxy-charm:auth-helper into squid-reverseproxy-charm:master

[Colin Watson]

Add authentication helper support

This allows relating a subordinate authentication helper charm to set up custom user authentication. We need this for Launchpad's builder proxy, which maintains a database of valid tokens that have been authorized by Launchpad's buildd-manager.

This commit is based on modifications to the squid-forwardproxy charm by Kit Randel in 2015. I've reworked them for squid-reverseproxy, moved most of the helper configuration into the subordinate charm's relation data, and added tests.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Colin Watson (cjwatson)]

The implementation I've been working on for the other end of this can be found in https://code.launchpad.net/~cjwatson/rutabaga/+git/rutabaga/+merge/408430.

[Tom Haddon (mthaddon)]

LGTM, thx

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 1b2ba3fe0f47a49721f445db9d31f9154e2b8144

diff --git a/README.md b/README.md
index c4b3b56..71e7744 100644
--- a/README.md
+++ b/README.md
@@ -81,6 +81,36 @@ A current implementation limitation is that it doesn't support multiple https vh
 This charm provides relations that support monitoring via Nagios using 
 nrpe_external_master as a subordinate charm.
 
+## Authentication Helpers
+
+To set up user authentication to the proxy, you need an authentication
+helper, which you may need to supply yourself if none of the [built-in
+helpers](https://wiki.squid-cache.org/Features/Authentication) are suitable.
+You can do this by relating a subordinate charm providing the
+`squid-auth-helper` interface to this charm.  Such a charm may publish
+relation data like this:
+
+    'auth-params': yaml.dump([
+        {
+            'scheme': 'basic',
+            'program': '/path/to/auth/helper',
+            'credentialsttl': '10 seconds',
+            'casesensitive': 'on',
+        }
+    ])
+
+This charm will turn that relation data into corresponding
+[`auth_param`](http://www.squid-cache.org/Doc/config/auth_param/)
+directives.  You may need to use something like this as one of your
+`auth_list` items to cause Squid to require authentication:
+
+    {"!proxy_auth": [REQUIRED], http_access: deny}
+
+If you do this, you should also set `wait_for_auth_helper: true` to cause
+this charm to wait for the `auth-helper` relation before starting Squid, as
+Squid will fail to start if it has a `proxy_auth` ACL without an
+authentication scheme being configured.
+
 ## Caveats
 The example above is just for reference. In order to make it usable, you 
 will have to supply a proper virtual host configuration for apache2.
diff --git a/config.yaml b/config.yaml
index e2232df..99267bb 100644
--- a/config.yaml
+++ b/config.yaml
@@ -195,6 +195,13 @@ options:
       stdin:
 
         python3 -c 'import sys, yaml; print(yaml.dump(yaml.safe_load(sys.stdin.read())))'
+  wait_for_auth_helper:
+    type: boolean
+    default: false
+    description: >
+      If true, do not start squid until an auth-helper relation is joined.
+      This is useful if auth_list configuration (e.g. "proxy_auth REQUIRED")
+      will cause squid to fail to start until an auth helper is available.
   metrics_target:
     default: ""
     type: string
diff --git a/hooks/auth-helper-relation-broken b/hooks/auth-helper-relation-broken
new file mode 120000
index 0000000..9416ca6
--- /dev/null
+++ b/hooks/auth-helper-relation-broken
@@ -0,0 +1 @@
+hooks.py
\ No newline at end of file
diff --git a/hooks/auth-helper-relation-changed b/hooks/auth-helper-relation-changed
new file mode 120000
index 0000000..9416ca6
--- /dev/null
+++ b/hooks/auth-helper-relation-changed
@@ -0,0 +1 @@
+hooks.py
\ No newline at end of file
diff --git a/hooks/auth-helper-relation-departed b/hooks/auth-helper-relation-departed
new file mode 120000
index 0000000..9416ca6
--- /dev/null
+++ b/hooks/auth-helper-relation-departed
@@ -0,0 +1 @@
+hooks.py
\ No newline at end of file
diff --git a/hooks/auth-helper-relation-joined b/hooks/auth-helper-relation-joined
new file mode 120000
index 0000000..9416ca6
--- /dev/null
+++ b/hooks/auth-helper-relation-joined
@@ -0,0 +1 @@
+hooks.py
\ No newline at end of file
diff --git a/hooks/hooks.py b/hooks/hooks.py
index da59000..73672a0 100755
--- a/hooks/hooks.py
+++ b/hooks/hooks.py
@@ -148,6 +148,17 @@ def get_reverse_sites():
     return all_sites
 
 
+def get_auth_params():
+    reldata = []
+    for relid in get_relation_ids('auth-helper'):
+        units = related_units(relid)
+        for unit in units:
+            data = relation_get(rid=relid, unit=unit)
+            if 'auth-params' in data:
+                reldata.extend(yaml.safe_load(data['auth-params']))
+    return reldata
+
+
 def ensure_package_status(packages, status):
     if status in ['install', 'hold']:
         selections = ''.join(['{} {}\n'.format(package, status)
@@ -342,6 +353,7 @@ def construct_squid3_config():
         'cache_dir': cache_dir,
         'dhparams_file': dhparams_file,
         'snmp_allowed_ips': snmp_allowed_ips,
+        'auth_params': get_auth_params(),
     }
     template = render_template('main_config.template', templ_vars)
     write_squid3_config('\n'.join(
@@ -548,6 +560,21 @@ def install_hook():
     return True
 
 
+def service_can_start():
+    # If wait_for_auth_helper is set, wait until squid has started and the
+    # squid-auth-helper relation has been joined.
+    config_data = config_get()
+    if not config_data["wait_for_auth_helper"]:
+        return True
+    if is_relation_made("auth-helper", keys=["auth-params"]):
+        log("Squid auth helper available, squid may start...")
+        return True
+    else:
+        log("Squid not ready, waiting for auth helper...")
+        service_squid3("stop")
+        return False
+
+
 def config_changed():
     old_service_ports = get_service_ports()
     old_sitenames = get_sitenames()
@@ -560,10 +587,16 @@ def config_changed():
                           metrics_config_path,
                           metrics_cronjob_path)
 
+    if not service_can_start():
+        return
+
     if service_squid3("check"):
         updated_service_ports = get_service_ports()
         update_service_ports(old_service_ports, updated_service_ports)
-        service_squid3("reload")
+        if service_squid3("status"):
+            service_squid3("reload")
+        else:
+            service_squid3("start")
         if not (old_sitenames == get_sitenames()):
             notify_cached_website()
     else:
@@ -575,6 +608,12 @@ def config_changed():
 
 
 def start_hook():
+    if service_can_start():
+        if not service_squid3("check"):
+            sys.exit(1)
+    else:
+        return
+
     if service_squid3("status"):
         return service_squid3("restart")
     else:
@@ -603,6 +642,13 @@ def cached_website_interface(hook_name=None):
         config_changed()
 
 
+def auth_helper_interface(hook_name=None):
+    if hook_name is None:
+        return
+    if hook_name in ("joined", "changed", "broken", "departed"):
+        config_changed()
+
+
 def get_hostname(host=None):
     my_host = socket.gethostname()
     if host is None or host == "0.0.0.0":
@@ -668,6 +714,15 @@ def main(hook_name):
     elif hook_name == "cached-website-relation-departed":
         cached_website_interface("departed")
 
+    elif hook_name == "auth-helper-relation-joined":
+        auth_helper_interface("joined")
+    elif hook_name == "auth-helper-relation-changed":
+        auth_helper_interface("changed")
+    elif hook_name == "auth-helper-relation-broken":
+        auth_helper_interface("broken")
+    elif hook_name == "auth-helper-relation-departed":
+        auth_helper_interface("departed")
+
     elif hook_name in ("nrpe-external-master-relation-joined",
                        "local-monitors-relation-joined"):
         update_nrpe_checks()
diff --git a/hooks/tests/test_config_changed_hooks.py b/hooks/tests/test_config_changed_hooks.py
index 4b7e66f..34ba268 100644
--- a/hooks/tests/test_config_changed_hooks.py
+++ b/hooks/tests/test_config_changed_hooks.py
@@ -1,4 +1,7 @@
-from unittest.mock import patch
+from unittest.mock import (
+    call,
+    patch,
+)
 
 from testtools import TestCase
 
@@ -15,6 +18,7 @@ class ConfigChangedTest(TestCase):
             "construct_squid3_config")
         self.update_nrpe_checks = self.patch_hook(
             "update_nrpe_checks")
+        self.is_relation_made = self.patch_hook("is_relation_made")
         self.update_service_ports = self.patch_hook(
             "update_service_ports")
         self.service_squid3 = self.patch_hook(
@@ -61,3 +65,45 @@ class ConfigChangedTest(TestCase):
         hooks.config_changed()
 
         self.assertFalse(self.notify_cached_website.called)
+
+    def test_config_changed_no_wait_for_auth_helper(self):
+        self.config_get.return_value = {"wait_for_auth_helper": False}
+        self.service_squid3.side_effect = (
+            True,   # check
+            False,  # status
+            True,   # start
+        )
+
+        hooks.config_changed()
+
+        self.is_relation_made.assert_not_called()
+        self.assertEqual(
+            [call("check"), call("status"), call("start")],
+            self.service_squid3.call_args_list)
+
+    def test_config_changed_wait_for_auth_helper_no_relation(self):
+        self.config_get.return_value = {"wait_for_auth_helper": True}
+        self.is_relation_made.return_value = False
+
+        hooks.config_changed()
+
+        self.is_relation_made.assert_called_once_with(
+            "auth-helper", keys=["auth-params"])
+        self.service_squid3.assert_called_once_with("stop")
+
+    def test_config_changed_wait_for_auth_helper_relation(self):
+        self.config_get.return_value = {"wait_for_auth_helper": True}
+        self.is_relation_made.return_value = True
+        self.service_squid3.side_effect = (
+            True,   # check
+            False,  # status
+            True,   # start
+        )
+
+        hooks.config_changed()
+
+        self.is_relation_made.assert_called_once_with(
+            "auth-helper", keys=["auth-params"])
+        self.assertEqual(
+            [call("check"), call("status"), call("start")],
+            self.service_squid3.call_args_list)
diff --git a/hooks/tests/test_helpers.py b/hooks/tests/test_helpers.py
index f52117f..0f91738 100644
--- a/hooks/tests/test_helpers.py
+++ b/hooks/tests/test_helpers.py
@@ -45,6 +45,7 @@ class SquidConfigTest(TestCase):
         super(SquidConfigTest, self).setUp()
         self.get_reverse_sites = self._apply_patch('hooks.get_reverse_sites')
         self.get_forward_sites = self._apply_patch('hooks.get_forward_sites')
+        self.get_auth_params = self._apply_patch('hooks.get_auth_params')
         self.config_get = self._apply_patch('hooks.config_get')
         self.mkdir = self._apply_patch('os.mkdir')
         self.chmod = self._apply_patch('os.chmod')
@@ -569,3 +570,27 @@ class SquidConfigTest(TestCase):
             """,
             )
         hooks.construct_squid3_config()
+
+    def test_auth_params(self):
+        self.config_get.return_value = Serializable({
+            "refresh_patterns": "",
+            "cache_size_mb": 1024,
+            "cache_mem_mb": 256,
+            "target_objs_per_dir": 1024,
+            "avg_obj_size_kb": 1024,
+        })
+        self.get_auth_params.return_value = [{
+            "scheme": "basic",
+            "program": "/path/to/helper argument",
+            "credentialsttl": "1 second",
+            "casesensitive": "on",
+        }]
+        self.get_reverse_sites.return_value = None
+        self.write_squid3_config.side_effect = self._assert_contents(
+            """
+            auth_param basic program /path/to/helper argument
+            auth_param basic credentialsttl 1 second
+            auth_param basic casesensitive on
+            """,
+            )
+        hooks.construct_squid3_config()
diff --git a/metadata.yaml b/metadata.yaml
index dd673af..09d34cf 100644
--- a/metadata.yaml
+++ b/metadata.yaml
@@ -31,6 +31,10 @@ provides:
   local-monitors:
     interface: local-monitors
     scope: container
+  auth-helper:
+    interface: squid-auth-helper
+    scope: container
+    optional: true
 requires:
   website:
     interface: http
diff --git a/templates/main_config.template b/templates/main_config.template
index cbe1ebf..b331488 100644
--- a/templates/main_config.template
+++ b/templates/main_config.template
@@ -79,6 +79,14 @@ refresh_pattern {{ rp }} {{ refresh_patterns[rp]['min'] }} {{ refresh_patterns[r
 {% endfor -%}
 refresh_pattern . {{default_refresh_pattern.min}} {{default_refresh_pattern.percent}}% {{default_refresh_pattern.max}} {{ ' '.join(default_refresh_pattern.options) }}
 
+{% for auth in auth_params %}
+  {% for key, val in auth.items() -%}
+    {% if key != 'scheme' -%}
+auth_param {{ auth['scheme'] }} {{ key }} {{ val }}
+    {% endif -%}
+  {% endfor -%}
+{% endfor %}
+
 # These are needed by the "squidpeers" Nagios check.  We place them before
 # custom ACLs since it's otherwise difficult to write custom ACLs that do
 # things like "deny all unauthenticated requests" without breaking that