irods / irods_rule_engine_plugin_metadata_guard

BSD 3-Clause "New" or "Revised" License
0 stars 7 forks source link

Metadata guard plugin allows renaming to protected string #58

Closed AliceSLee closed 8 months ago

AliceSLee commented 1 year ago

iRODS Version, OS and Version

iRODS 4.3.0; Ubuntu 20.04.6 LTS

What did you try to do?

Using the metadata guard plugin, we protected a metadata prefix "teststring".

Expected behavior

  1. Adding new metadata to an object with the protected prefix "teststring" in attribute name should fail.
  2. Removing or editing the "teststring" prefix on existing metadata should fail.
  3. Renaming existing metadata to have the protected prefix "teststring" in its new attribute name should fail.

Observed behavior

  1. and 2. above worked as expected, but in case 3, this activity was allowed (bypassing the guard).

To reproduce:

korydraughn commented 1 year ago

The behavior you're describing is intended.

If admins aren't allowed to move AVUs into the protected space, then any time they want to introduce new AVUs, they'd have to disable the plugin.

If you don't want that behavior, you can add new policy to disallow that.

Please explain why you feel the expressed behavior should not be allowed.

trel commented 1 year ago

is the current behavior (3) affected by the user being a rodsadmin or not?

should the intended behavior be affected by that?

AliceSLee commented 1 year ago

Thanks for your fast replies! @trel, the test user was a rodsuser, not rodsadmin @korydraughn, my apologies - I had thought that regular users being able to change an existing AVU to a protected string would not be allowed (otherwise it's an easy workaround to case 1), but if this is intended we will just be cautious with it

trel commented 1 year ago

So a rodsuser is/was able to move an AVU 'into' the protected space, but not out of it.

I think that's okay/intended.

Yes, the guard's primary function is to make sure AVUs that are used as system/programmatic/state information cannot be changed. Agreed that 3) is a workaround for 1)... and we're happy to discuss if you think there is a better balance.

AliceSLee commented 1 year ago

Thanks for explaining. For context, the reason we found this to be an issue is because we use a protected AVU for tiering. While a regular user cannot change the protected attribute name or value (case 1), they can simply create a new AVU with a different value and then rename it to the protected one (case 3), thereby bypassing standard policy with respect to where their data object is stored. We don't anticipate users deliberately doing so, but in theory it's possible with this workaround.

trel commented 1 year ago

Makes sense. You should be able to craft a PEP that explicitly denies this type of edit/update.

We are happy to help think through how that may work.

If you/we can write it in a generic way, we may be able to incorporate the approach into the plugin directly.

korydraughn commented 1 year ago

I was struggling for a second on how modifying an existing AVU is possible, but now I see it.

That is a bug that is certainly fixable. This issue needs to be moved to the metadata guard repo.

Updating the plugin to only allow approved users to modify protected AVUs should be trivial.

trel commented 1 year ago

Well, they're not protected at the time of PEP firing, we'd have to check the 'new' value in the pre-PEP.

Moving this issue... and we can reopen over there if we decide to add this check.

We already have the list of 'editors', which I think satisfies the 'approved users' part.

trel commented 1 year ago

reopening - at least for discussion, so it doesn't disappear.

korydraughn commented 1 year ago

The metadata API uses the arg0 to argN interface. The plugin does not check the target of a modification.

The plugin can be configured to allow only admins or approved editors to touch protected metadata.

Should users who do not satisfy the config requirements be able to overwrite protected metadata via a rename?

I don't think so.

trel commented 1 year ago

I think the plugin already allows only admins and editors to touch already protected metadata, and prevents non admin/editors to change/delete the protected AVU.

We're talking about making sure a non-admin/editor can also not modify a not-protected AVU and put it into the protected namespace.

korydraughn commented 1 year ago

That's correct.

Do you think that should be allowed?

My concern comes from whether it is possible to overwrite a protected AVU via a rename. I think you can.

trel commented 1 year ago

Oh... I see...

A) In addition to the arguable bug (the current discussion) where a non-admin/editor user can modify an existing AVU and put it INTO the protected namespace...

B) You are concerned that a non-admin/editor user can modify an existing non-protected AVU and OVERWRITE an already existing protected AVU... hmmm, that would definitely be worse and definitely be a bug.

Sounds like we have a couple tests to write.

alanking commented 8 months ago

@FifthPotato - Please close if complete. Thanks!