search

canonical / operator

Pure Python framework for writing Juju charms
Apache License 2.0
245 stars 119 forks source link

Harness not modelling permission denied for user secrets #1229

Closed DnPlas closed 2 months ago

DnPlas commented 3 months ago

1176 introduced support for user secrets in Harness, but it looks like it is not modelling the actual Juju behaviour when a user secret hasn't been granted to a charm and it tries to get a secret with self.model.get_secret(id=<id-from-config>).

On an actual model, if the charm config gets updated with a secret ID, but the charm hasn't been granted access to the secret, ops.model.ModelError: ERROR permission denied will be raised, but it seems that when testing, harness makes the charm to raise a different exception ops.model.SecretNotFoundError: Secret 'secret:16320250-1b76-4552-94d9-10ff484f88cb' not granted access to 'istio-pilot when calling the exact same piece of code.

Steps to reproduce

  1. Deploy any charm that can use user secrets
  2. Create a user secret
  3. Pass the secret ID through charm configuration juju config <charm-name> my-secret=secret:some-id
  4. Observe the charm state and logs. You should get something like this:
  File "/var/lib/juju/agents/unit-istio-pilot-0/charm/venv/ops/model.py", line 3397, in secret_get
    result = self._run('secret-get', *args, return_output=True, use_json=True)
  File "/var/lib/juju/agents/unit-istio-pilot-0/charm/venv/ops/model.py", line 3051, in _run
    raise ModelError(e.stderr) from e
ops.model.ModelError: ERROR permission denied
  1. Add a unit test for testing what happens when the secret hasn't been granted to the charm. For example:
def test_permission_denied(...):
  secret_content = {"tls-crt": "a-tls-crt", "tls-key": "a-tls-key"}
  secret_id = harness.add_user_secret(secret_content)
  harness.update_config({"tls-secret-id": secret_id})
  harness.begin()
  with pytest.raises(ModelError) as err:
    harness.charm.function_call_to_get_secret() # <--- this is a function that calls self.model.get_secret(id=<id from config>)
  1. The above will not raise because it is expecting a ModelError but a SecretNotFoundError exception is raised instead:
 ops.model.SecretNotFoundError: Secret 'secret:16320250-1b76-4552-94d9-10ff484f88cb' not granted access to 'istio-pilot
DnPlas commented 3 months ago

Extra information:

According to get_secret docs, it only raises SecretNotFoundError if the secret does not exist, but _ModelBackend.secret_get, which is called by the model.get_secret() method, will raise a ModelError under other conditions, which can include the permission denied scenario that I am describing.

benhoyt commented 3 months ago

@IronCore864 @tonyandrewmeyer It makes sense to me to make the testing exception the same as the real one. So I think we just change the SecretNotFoundError here to ModelError. Sound reasonable?

tonyandrewmeyer commented 3 months ago

@IronCore864 @tonyandrewmeyer It makes sense to me to make the testing exception the same as the real one.

I do agree with this in general.

So I think we just change the SecretNotFoundError here to ModelError. Sound reasonable?

However, I don't think this is the right fix here. I think this is a Juju bug. All of the other permission errors (like trying to do a get_info() when only having view permission, or trying to get a charm secret that you haven't been granted permission to) raise SecretNotFoundError). Apart from the inconsistency, behaving differently when there is a secret with no access and when there is no secret feels leaky to me.

So, in my opinion, we should open a Juju bug instead.

tonyandrewmeyer commented 3 months ago

So, in my opinion, we should open a Juju bug instead.

Checked with the Juju team and they agree that it should be consistent in Juju, so I opened a Juju bug.

tonyandrewmeyer commented 3 months ago

According to get_secret docs, it only raises SecretNotFoundError if the secret does not exist

Opened #1231 to make this explicit in the docs.

tonyandrewmeyer commented 3 months ago

Actually, I got this wrong. Charm secrets do return "permission denied" as well as user ones, so there must be an ops issue here.

tonyandrewmeyer commented 3 months ago

Juju 3.4.2, Microk8s, I get this behaviour:

hook tool issue user secret charm secret
secret-get no permission "permission denied" "permission denied"
secret-info-get no permission "not found" "not found"
secret-set no permission delayed permission denied delayed permission denied
secret-remove no permission delayed permission denied delayed permission denied
secret-grant no permission "not found" "not found"
secret-revoke no permission delayed permission denied delayed permission denied
secret-get doesn't exist "not found" "not found"
secret-info-get doesn't exist "not found" "not found"
secret-set doesn't exist delayed not found delayed not found
secret-remove doesn't exist delayed not found delayed not found
secret-grant doesn't exist "not found" "not found"
secret-revoke doesn't exist delayed not found delayed not found

For the "delayed" cases, the hook tool returns successfully, and then when the hook finishes (after ops is done) the charm goes into error state with something like this in the debug-log:

unit-dummycharm-1: 12:56:16 ERROR juju.worker.uniter.context cannot apply changes: removing secrets: permission denied
unit-dummycharm-1: 12:56:16 ERROR juju.worker.uniter.operation hook "config-changed" (via hook dispatching script: dispatch) failed: removing secrets: permission denied

Also discussing this with @barrettj12 who is working on the Juju ticket.