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

ssl-key and ssl-crt removal broke compatibility with a specific TLS use case #380

Closed
DnPlas opened this issue Feb 16, 2024 · 8 comments · Fixed by #394
Closed

ssl-key and ssl-crt removal broke compatibility with a specific TLS use case #380

DnPlas opened this issue Feb 16, 2024 · 8 comments · Fixed by #394
Labels
bug Something isn't working

Comments

@DnPlas
Copy link
Contributor

DnPlas commented Feb 16, 2024

Bug Description

Changes introduced by #338 removed the support for enabling TLS for users that only have the ssl-key and ssl-crt files generated by an external Certificate Authority (CA). This change affects istio-operators v1.17 and latest/edge.

That PR removed configuration options that allowed users to pass these values with the assumption that the integration with tls-certificates-interface would be a replacement for it. This assumption considered all cases where users wanted to have a charm to provide certificates (a certificates provider) and handle all the required automation, and even considered the case where users would have an in-house CA so they could use charms like the manual-tls-certificates-operator. The only missing scenario is the one described in the first paragraph.

This scenario is still relevant for some users because they can only get the ssl-key and ssl-crt from a CA.

We should consider options to support this, ideally in a way that can guarantee a good level of security and follows the best recommendations from the maintainers of the tls-certificates interface.

@DnPlas DnPlas added the bug Something isn't working label Feb 16, 2024
Copy link

Thank you for reporting us your feedback!

The internal ticket has been created: https://warthogs.atlassian.net/browse/KF-5346.

This message was autogenerated

@DnPlas
Copy link
Contributor Author

DnPlas commented Mar 8, 2024

After some investigation, the team has agreed to provide support for the removed feature, but instead of bringing back the charm configuration, the proposal is to leverage juju secrets.

Considerations for supporting this feature

  • Charmed Kubeflow deployments have to have juju 3.x
  • This feature will only be supported for versions 1.8 and 1.9 of Charmed Kubeflow. A deprecation notice and enough documentation will be provided.
  • Because this feature is an undesirable mode of operation, as passing around keys and certs is insecure, users should seek for a better and more secure way using the TLS certificates operators.
  • The secret is owned by the model, meaning the model admin can manage it.

TLS using juju secrets

From juju 3.0, we have secrets available to be used by a charm that needs to know sensitive information, like a CA cert and key.
Users should pass these values using juju add-secret ssl-key=<some value> and juju add-secret ssl-crt=<some value>, after that, admins must grant access to istio-pilot to that secret. Using the operator framework, the charm code will have access to the secret and use it.

Changes in istio-pilot

  • The charm now has to observe self.on.<secret><event> and act
  • The charm must prioritise the certs provided by a TLS certificates operator over the ones provided via the secrets
  • The reconcile function must work with values coming from either source considering the previous rule

@DnPlas
Copy link
Contributor Author

DnPlas commented Mar 14, 2024

After investigating a bit more on how juju secrets are used by other charms, it looks like setting a value of a secret is rather done trough charm actions that have charm code in the backend to allow saving the secret than directly creating the secret from the juju CLI and expecting the charm to access it. Take for instance postgressql, which allow users to set a password by providing an action which at the same time sets a secret for saving its value. That seems to also be the pattern in the juju docs.

The juju docs mention that secret events, which we'll use to observe and react to changes in secrets, are only triggered by charm code (see here):

Secret events can’t be directly triggered by Juju admin operations. Most other events occur because someone did something on the Juju CLI (created a relation, scaled something down, and so on); secret events are, however, exclusively triggered either by charm code or an internal Juju timeout (similar to update-status).

Thus, the proposal for solving this issue may change to implement something similar to postgresql charm:

  1. Implement an action for istio-pilot that allows users to set ssl-cert and ssl-key
  2. Create some sort of interface for saving secret data in a peer relation, perhaps there is another approach, need to investigate
  3. The istio-pilot charm code will then acquire the secret values and use them for rendering the ingress gateway

DnPlas added a commit that referenced this issue Mar 20, 2024
This commits introduces actions that allow users to configure the TLS
ingress gateway for a single host directly passing the SSL cert and key
to the charm.
- save-tls-secret: allows users to pass the ssl-key and ssl-crt values,
  which the charm saves in a juju secret (owned by the charm) and uses
  them to reconcile the ingress Gateway with such information.
- remove-tls-secret: a handy action that allows users to remove the
  TLS secret, which in turn removes the TLS configuration from the
  ingress Gateway.

This commit also adds unit and integration tests to increase the
coverage due to the recent changes.

WARNING: please note this feature is only supported in 1.17 and 1.18,
and it will be removed after releasing 1.18 in favour of the TLS
provider method.

Fixes #380
DnPlas added a commit that referenced this issue Mar 22, 2024
#394)

* feat: add action to handle SSL values as secrets for TLS configuration

This commits introduces actions that allow users to configure the TLS
ingress gateway for a single host directly passing the SSL cert and key
to the charm.
- save-tls-secret: allows users to pass the ssl-key and ssl-crt values,
  which the charm saves in a juju secret (owned by the charm) and uses
  them to reconcile the ingress Gateway with such information.
- remove-tls-secret: a handy action that allows users to remove the
  TLS secret, which in turn removes the TLS configuration from the
  ingress Gateway.

This commit also adds unit and integration tests to increase the
coverage due to the recent changes.

WARNING: please note this feature is only supported in 1.17 and 1.18,
and it will be removed after releasing 1.18 in favour of the TLS
provider method.

Fixes #380
DnPlas added a commit that referenced this issue Mar 26, 2024
#394)

* feat: add action to handle SSL values as secrets for TLS configuration

This commits introduces actions that allow users to configure the TLS
ingress gateway for a single host directly passing the SSL cert and key
to the charm.
- save-tls-secret: allows users to pass the ssl-key and ssl-crt values,
  which the charm saves in a juju secret (owned by the charm) and uses
  them to reconcile the ingress Gateway with such information.
- remove-tls-secret: a handy action that allows users to remove the
  TLS secret, which in turn removes the TLS configuration from the
  ingress Gateway.

This commit also adds unit and integration tests to increase the
coverage due to the recent changes.

WARNING: please note this feature is only supported in 1.17 and 1.18,
and it will be removed after releasing 1.18 in favour of the TLS
provider method.

Fixes #380
@jameinel
Copy link
Member

There are charms that use the "create a secret for myself via an action, and put it into the peer relation". This is because in Juju 3.3 when we introduced secrets, we only had application generated secrets such that they could be shared over relation data.

In juju 3.4 we added user secrets, such that you can add a secret as the end user, populate the content of that secret, and then share that secret with an application. However, the handle for those secrets, rather than being a relation data bag, is a config entry.

I updated one of my charms to prove that it is working, and I did find a couple of rough edges in the ecosystem. But the rough diff for my ubuntu-lite charm was:

diff --git a/config.yaml b/config.yaml
index a985d46..b8fdc18 100644
--- a/config.yaml
+++ b/config.yaml
@@ -1 +1,4 @@
-options: {}
+options:
+  mysec:
+    type: secret
+    description: "tell me your secrets"
diff --git a/requirements.txt b/requirements.txt
index 890434e..d3c07b5 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,2 +1,2 @@
-ops < 2.0
+ops @ git+https://github.com/jameinel/operator@2.12-user-secrets

diff --git a/src/charm.py b/src/charm.py
index 6c9c791..a1f6937 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -3,6 +3,7 @@
 # Licensed under the AGPLv3, see LICENCE file for details.

 import os
+import logging
 import subprocess

 from ops import (
@@ -12,6 +13,9 @@ from ops import (
 )


+logger = logging.getLogger("ubuntu-lite")
+
+
 def set_application_version(version):
     # TODO: application-version-set should be modeled in the framework
     subprocess.check_call(['application-version-set', version])
@@ -39,6 +43,9 @@ class Ubuntu(charm.CharmBase):
         self.framework.observe(self.on.update_status, self._on_update_status)
         self.framework.observe(self.on.load_action, self._on_load_action)

+        self.framework.observe(self.on.config_changed, self._on_config_changed)
+        self.framework.observe(self.on.secret_changed, self._on_secret_changed)
+
     def _on_start(self, event):
         self.model.unit.status = model.ActiveStatus()
         set_application_version(_get_ubuntu_series())
@@ -56,6 +63,23 @@ class Ubuntu(charm.CharmBase):
             "15min": load15min,
         })

+    def _on_config_changed(self, event):
+        logger.info("config: %r", dict(self.config))
+        mysec = self.config.get('mysec')
+        if mysec:
+            sec = self.model.get_secret(id=mysec, label="mysec")
+            content = sec.get_content()
+            peek = sec.peek_content()
+            logger.info("secret, I shouldn't be telling you this: %r", content)
+            logger.info("secret, peeking ahead: %r", peek)
+
+    def _on_secret_changed(self, event):
+        logger.info("secret event: %r", event)
+        content = event.secret.get_content()
+        peek = event.secret.peek_content()
+        logger.info("secret, I shouldn't be telling you this: %r", content)
+        logger.info("secret, peeking ahead: %r", peek)
+

So you have to add a config field, with the type of 'secret'. And then you need to handle the fact that the value of the secret id will be supplied in config, and that updates to that secret will be triggered via secret-changed.
Note that the event object in secret changed will include the secret-id and the label that you supplied when you read the secret.
You'll also need to be aware of peek_content/get_content/get_content(refresh=True), but that is the same semantics as we have for relation secrets.

From the Juju CLI, you then do:

$ SECRET_ID=`juju add-secret my-named-secret foo=bar`
$ juju grant-secret my-named-secret application
$ juju config application mysec=$SECRET_ID

And then in the future you can test:

$ juju update-secret my-named-secret foo=bing

And that will trigger the _on_secret_changed hook to fire.

@jameinel
Copy link
Member

(Note that the reason I have a custom branch of ops is because of: canonical/operator#1166 which looks to only impact the test suite, not runtime operation.)

@jameinel
Copy link
Member

btw, I'm slightly wrong about 3.4. I tested against 3.3 and it also supports the same workflow. I just did:

 2124  sudo snap refresh juju --channel 3.3
 2125  type -a juju
 2126  juju bootstrap lxd lxd_33
 2127  juju status
 2128  juju add-model secret-test
 2131  juju deploy ./ubuntu-lite_ubuntu-16.04-amd64_ubuntu-18.04-amd64_ubuntu-20.04-amd64_ubuntu-22.04-amd64.charm
 2136  juju config ubuntu-lite mysec=foobar
 # failed as it sure because 'foobar' isn't a secret id
 2137  juju add-secret named-secret foo=bar
 2141  juju grant-secret named-secret ubuntu-lite
 2142  juju secrets
 2143  juju config ubuntu-lite mysec=secret:co1fdb3joi73h05jsvsg

And in the debug-log I can see it all working:

unit-ubuntu-lite-0: 12:25:09 INFO unit.ubuntu-lite/0.juju-log config: {'mysec': 'secret:co1fdb3joi73h05jsvsg'}
unit-ubuntu-lite-0: 12:25:09 INFO unit.ubuntu-lite/0.juju-log secret, I shouldn't be telling you this: {'foo': 'bar'}
unit-ubuntu-lite-0: 12:25:09 INFO unit.ubuntu-lite/0.juju-log secret, peeking ahead: {'foo': 'bar'}
unit-ubuntu-lite-0: 12:25:09 INFO juju.worker.uniter.operation ran "config-changed" hook (via hook dispatching script: dispatch)
unit-ubuntu-lite-0: 12:25:36 INFO unit.ubuntu-lite/0.juju-log secret event: <SecretChangedEvent via Ubuntu/on/secret_changed[26]>
unit-ubuntu-lite-0: 12:25:36 INFO unit.ubuntu-lite/0.juju-log secret, I shouldn't be telling you this: {'foo': 'bar'}
unit-ubuntu-lite-0: 12:25:36 INFO unit.ubuntu-lite/0.juju-log secret, peeking ahead: {'foo': 'bing'}
unit-ubuntu-lite-0: 12:25:36 INFO juju.worker.uniter.operation ran "secret-changed" hook (via hook dispatching script: dispatch)

@jameinel
Copy link
Member

I also confirmed that with juju 3.1 you are able to deploy the charm, but commands like juju add-secret don't exist. Interestingly, we do still validate the secret:

$ juju --version
3.1.7-genericlinux-amd64
jameinel@jammy:~/dev/charms/ubuntu-lite
$ juju config ubuntu-lite mysec=1234
{}
ERROR updating charm config settings: option "mysec" expected secret, got "1234"

So you can deploy a charm, that optionally has a config that has a secret, and still allow that to deploy with 3.1 (you just won't have a way to supply that as a user-secret). But you can do the compatibility level in the charm. Or you can set an Assumes of Juju>=3.3 which would mean you don't want to be deployed with 3.1

@ca-scribner
Copy link
Contributor

Thanks @jameinel! These examples and context are really helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants