Skip to content

Update ConfigCommand to check site and system permissions#3012

Closed
cshannon wants to merge 1 commit intoapache:mainfrom
cshannon:accumulo-3010-config-command
Closed

Update ConfigCommand to check site and system permissions#3012
cshannon wants to merge 1 commit intoapache:mainfrom
cshannon:accumulo-3010-config-command

Conversation

@cshannon
Copy link
Copy Markdown
Contributor

@cshannon cshannon commented Oct 8, 2022

The ConfigCommand has been updated to handle security exceptions for users that don't have access to site/system configs and will instead just print the default config and the override of either namespace or table property.

This is part 2 of #3010

The ConfigCommand has been updated to handle security exceptions for
users that don't have access to site/system configs and will instead
just print the default config and the override of either namespace or
table property.

This is part 2 of apache#3010
@cshannon
Copy link
Copy Markdown
Contributor Author

cshannon commented Oct 8, 2022

This is a draft/prototype of just using default configs for determining the level of overrides for properties for users that don't have access to site/system configs after the changes in #2994 .

It probably still needs a bit of work (hence draft status) but I wanted to get some feedback on what people think and if this is what we want to do to solve the problem or if we want to do something else. I probably won't have a lot of time to work on this again until Friday and next weekend so if someone wants to take this over and polish it up feel free as it's a blocker for 2.1.0, assuming this is the direction we want to go in.

If this is not the strategy we want then as I mentioned previously in #3010, the other 2 possibilities would be:

  1. Move the logic for determining the overrides for the config to the server, which would require a lot more changes including to the API and RPC layer to return the information for the properties and what level they were applied. I think this is a bit late for version 2.1.0 to make those changes but if it's something we want to do and there is time I could work on that instead.
  2. Revert back the permissions checks entirely and then make the changes for 3.0 (either client side like this or handle it server side)

Copy link
Copy Markdown
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changed a lot of code, and I don't have time to review all of it, so I'm commenting mostly on what appears to be the intended design.

It seems like the intent is to provide feedback on which configs were able to be read, based on the permissions checks.

The main problem with this is that if you can't read all of them, it becomes impossible for the config command to be able to tell which level a configuration value is being set. (default, site, system, namespace, or table). So, for example, with these changes, the user might be able to see the resolved table configuration, but it won't know that the table property is actually set at the system level, and not in the table's configuration.... everything would look like it was overriding the defaults at the table level.

This PR makes me wonder what the point is of adding the permissions checks on the various configuration getters in our API. What is the end goal? Is it to protect against the user being able to view configuration? Ultimately, I'm wondering if that even matters. Previously, any authenticated user could view the configuration for any table, namespace, etc. I think that might still be okay... the configuration should not be used to hold secrets, and if we lock them down, we run into all sorts of complications, like those this PR is revealing. I think we need to ask what we're trying to protect, and from whom.

Comment on lines +173 to +175
Shell.log.debug(
"User does not have permission to read system and/or site configuration: {}",
e.getMessage());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put this as a warning rather than debug. This could also be separate messages, with information about what shell command to use to grant the appropriate permission.

@cshannon
Copy link
Copy Markdown
Contributor Author

Right, it definitely opened up a can of worms with the permissions stuff when locked down so that's why one of the options was to just revert this entirely. @dlmarion has a PR with #3015 which is a lot less changes that could be another option

@cshannon cshannon self-assigned this Oct 12, 2022
@cshannon
Copy link
Copy Markdown
Contributor Author

Closing this as it looks like we will be going with #3015

@cshannon cshannon closed this Oct 12, 2022
@cshannon cshannon deleted the accumulo-3010-config-command branch December 9, 2022 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants