Merge ~tshinn/snap-store-proxy-charm:fix_https_cert_config into snap-store-proxy-charm:main

[Tong Shinn]

snap-store-proxy CLI has the import-certificate option. Update the charm to use it instead of setting proxy.tls.cert and proxy.tls.key directly, which doesn't fully work.

After deploying the charm, the following commands can be used to configure snap-store-proxy to support HTTPS:

juju config snap-store-proxy certificate="$(cat my.cert)"
juju config snap-store-proxy private_key="$(cat my.key)"

where my.cert and my.key are PEM formatted key/cert pair.

[Odysseus Kaziolas (odysseus-k)]

Haven't used snap-store-proxy but changes LGTM plus tests pass. Maybe @suligap should take a look here as well as he's more experienced with snap store proxy.

Do we also need to have an entry in our docs with the commands above?

review: Approve

[Przemysław Suliga (suligap)]

This looks good to me.

But looking at snap-store-proxy code it also looks like a possible issue in snap-store-proxy itself that cert validation will fail during `snap-proxy config ...` when the passed in proxy.tls.cert is actually a chain eg. intermediate and leaf cert.

This validation is done properly in case of `import-certificate` command. But I don't see a reason why `snap-proxy config` wouldn't do the same.

But also:

1. From your description I'm not sure - is that the issue you're trying to solve?
2. It probably won't fully solve this issue because the charm only expects one cert to be present as `certificate` and does validation against that and that might fail first in those cases?

[Tong Shinn (tshinn)]

1. No, I treated snap-store-proxy itself as a black box while getting to this point. My focus was solely on fixing the charm. The charm didn't work as is because instead of using `import-certificate` it configured `proxy.tls.cert` and `proxy.tls.key` configurations directly, which didn't result in nginx getting configured properly (was unable to get to the bottom of exactly why). When I tried `import-certificate` it started working, hence the MP.

2. Yes I think you are right that the charm itself only supports a single certificate in the PEM file. When the snap-store-proxy supports it, the charm should be updated also! :)

[Wouter van Bommel (woutervb)]

Left one question, probably doesn't hurt that it's there, just not sure it's needed.

review: Approve

[Przemysław Suliga (suligap)]

> Yes I think you are right that the charm itself only
> supports a single certificate in the PEM file. When the
> snap-store-proxy supports it, the charm
> should be updated also! :)

`snap-store-proxy import-certificate` already supports that correctly. `snap-store-proxy config proxy.tls.cert=...` does not I think. So if this change is merged, then it's just the charm's cert validation logic that would need to be updated. Or perhaps it should be removed since the proxy does the validation anyway? But there's probably a reason why it's in the charm too.

[Tong Shinn (tshinn)]

Will merge this MP as is and work on supporting chained certs in a seperate MP.

diff --git a/src/charm.py b/src/charm.py
index ac50311..f71f20b 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -22,6 +22,7 @@ from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus, ModelError
 from exceptions import ConfigException
 from helpers import (
     all_config_options,
+    configure_cert,
     configure_proxy,
     configure_self_signed_cert,
     default_values,
@@ -338,13 +339,7 @@ class SnapstoreProxyCharm(CharmBase):
 
         # Okay, the certificate and private key do make sense and match this host,
         # so let's install them
-        configure_proxy(
-            ["proxy.tls.cert", "proxy.tls.key"],
-            [
-                getattr(self._stored, "certificate"),
-                getattr(self._stored, "private_key"),
-            ],
-        )
+        configure_cert(raw_key, raw_certificate)
         logger.info("Successfully installed the tls certificate on the proxy")
 
     def handle_pgsql_dsn_change(self):
diff --git a/src/helpers.py b/src/helpers.py
index 8b92f42..3088c2b 100644
--- a/src/helpers.py
+++ b/src/helpers.py
@@ -35,6 +35,12 @@ def configure_proxy(option, value, force=False):
     run(command)
 
 
+def configure_cert(key, cert):
+    logger.info("Importing and configuring certificate for nginx")
+    run(f'{PROXY_CLI} config proxy.tls.cert="" proxy.tls.key=""'.split())
+    run(f"{PROXY_CLI} import-certificate".split(), input=f"{key}\n{cert}", text=True)
+
+
 def configure_self_signed_cert():
     logger.info("Generating and configuring self signed certificate for nginx")
     run(f'{PROXY_CLI} config proxy.tls.cert="" proxy.tls.key=""'.split())
diff --git a/tests/test_charm.py b/tests/test_charm.py
index e36b9a3..02f03c6 100644
--- a/tests/test_charm.py
+++ b/tests/test_charm.py
@@ -631,15 +631,13 @@ class TestTLSCertificate(CharmTest):
         cert, key = self.create_x509_certificate("test.domain.just.invented", san)
 
         # Since valid values will be pushed on to the proxy, we need to stub that here
-        with patch("charm.configure_proxy") as m_configure, patch(
+        with patch("charm.configure_cert") as m_configure, patch(
             "charm.open_port"
         ) as m_open_port:
             # Ensure that the default values are set, as expected
             harness.update_config({"certificate": cert, "private_key": key})
 
-        m_configure.assert_called_once_with(
-            ["proxy.tls.cert", "proxy.tls.key"], [cert, key]
-        )
+        m_configure.assert_called_once_with(key, cert)
         m_open_port.assert_called_once_with(ssl=True)
 
         assert len(harness.charm.errors) == 0
diff --git a/tests/test_helpers.py b/tests/test_helpers.py
index 15fac97..2747726 100644
--- a/tests/test_helpers.py
+++ b/tests/test_helpers.py
@@ -47,6 +47,25 @@ def test_configure_proxy_list_forced():
     )
 
 
+def test_configure_cert():
+    key = "private_key_in_pem_format"
+    cert = "certificate_in_pem_format"
+    with mock.patch("helpers.run") as mocked_run:
+        helpers.configure_cert(key, cert)
+
+    mocked_run.assert_has_calls(
+        [
+            mock.call(f'{PROXY_CLI} config proxy.tls.cert="" proxy.tls.key=""'.split()),
+            mock.call(
+                f"{PROXY_CLI} import-certificate".split(),
+                input=f"{key}\n{cert}",
+                text=True,
+            ),
+        ],
+        any_order=False,
+    )
+
+
 def test_configure_self_signed_cert():
     with mock.patch("helpers.run") as mocked_run:
         helpers.configure_self_signed_cert()