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

[FEATURE] Create a way to add action oriented APIs to update configuration values #2559

Open
peternied opened this issue Mar 16, 2023 · 10 comments
Labels
enhancement New feature or request triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. untriaged Require the attention of the repository maintainers and may need to be prioritized

Comments

@peternied
Copy link
Member

Is your feature request related to a problem?
The security config of OpenSearch covers a lot of surface area. Some of these settings are very sensitive, others operators might want to allow for more control over. There is no clean way to provide granular access to these settings.

Lets take into consideration a recent configuration setting that was added jwt_clock_skew_tolerance_seconds. The legwork required to expose GET / POST / DELETE (restore to default value) is large and difficult.

What solution would you like?
There should be a way to declare that there should be an configuration based action handler to support exposing this setting such as the following:

new ConfigActionDescription().Builder()
   .valueType(ConfigActionType.Integer)
   .route("_plugins/_security/config/jwt/{auth_domain}/skew_tolerance_seconds")
   .permission("securityconfig:admin/config/jwt/skew_tolerance_seconds")
   .configPath("config.dynamic.authc.{auth_domain}.http_authenticator.config.jwt_clock_skew_tolerance_seconds")
   .onDeleteDefaultValue(DEFAULT_CLOCK_SKEW_TOLERANCE_SECONDS)
   .build();
  • Note; type safety constraints might make this more slightly more complex

After registering the ConfigActionDescription object a handler would be built up that would support integer values following a straight forward convention of retrieve, update and set to default. When attempting to access the endpoint, the permission would would be required to be prefixed with 'securityconfig:' and read from the users roles.

Example REST APIs

Retrieve

Request

GET "_plugins/_security/config/jwt/jwt_auth_domain/skew_tolerance_seconds"

Response

200 OK 
{
   "value": 30
}

Update

Request

PUT "_plugins/_security/config/jwt/jwt_auth_domain/skew_tolerance_seconds"
{
   "value":77
}

Response

200 OK
{
   "value": 77
}

Set to default

Request

DELETE "_plugins/_security/config/jwt/jwt_auth_domain/skew_tolerance_seconds"

Response

200 OK
{
   "value": 77
}

Do you have any additional context?
I think this would be a good way to work through problems around duplicate data and filtered access for dynamically modifying tenant config. I think using a 'action' oriented framework would help create abstraction layers from being exposed to downstream users, e.g. GET _plugins/_security/config/tenancy/multitenancy_enabled

@abhivka7
Copy link
Contributor

How is this approach better than having different documents for different configurations/properties, specially in case of properties such as tenancy which in general shouldn't have been included in security config in the first place?

@davidlago
Copy link

It minimizes the surface of the changes needed, de-risking the proposed feature from breaking changes that need to be kept in sync until the fields are deprecated, and it postpones a decision of reorganizing configuration fields in different files.

@prabhat-chaturvedi
Copy link

prabhat-chaturvedi commented Mar 17, 2023

@peternied one gotcha here is that the parent SecurityConfig's contract can break the tenancyConfig(assuming part of security config).

Assume, there are following Admins in the order of the High to Low permissions:
SuperAdmin
TenancyAdmin
SecurityAdmin
DomainAdmin

securityConfigAPI (SecurityAdmin)
	>tenancyConfig also can be updated
	
tenancyAPI (the admin should have securityConfig governed permission at field level)
	Only tenancyConfig fields can be updated

The permissions for Security Config should always be more or equal to permission of TenancyAPIs
How do we enforce such rules, otherwise its open to implementation and may lead to breaching the intent of bring action oriented APIs part of same config.

@peternied
Copy link
Member Author

Want to provide an update as the POC isn't coming online as I had hoped, but wanted to response to some of the comments on this issue.

The permissions for Security Config should always be more or equal to permission of TenancyAPIs

@prabhat-chaturvedi I'm not sure I understand the scenario associated with this, could you provide more context? I'll write out what I think are a couple of relevant scenarios around access.

Lets assuming there is an permission called securityconfig:admin/config/tenancy/multitenancy_enabled that governs actions to get/update on the config value dynamic.multitenancy_enabled

  1. Admin changes the value via the config
    All config values is are updated, the new state goes into effect
  2. Admin change the value via the action:update
    Only the specific value dynamic.multitenancy_enabled is changed, but the whole config is reloaded the new state goes into effect
  3. Tenacy admin (who has only access to the action:update) updates the value via action:update
    Only the specific value dynamic.multitenancy_enabled is changed, but the whole config is reloaded the new state goes into effect
  4. Tenacy admin (who has only access to the action:update) attempts to update the value via the config
    The request is blocked, a 403 is returned
  5. Generic User attempts to update the value via action:update
    The request is blocked, a 403 is returned

Are there other scenarios that need to be take into consideration or access controls that are missing?

More detail on what was not working for those interested

I was planning on building off AbstractApiAction as it handles accepting updates to the configuration system, verifying them and then refreshing the security configuration on all nodes in the cluster. This has an existing security model that is based on role membership, and doesn't support 'action' level granularity.

I built up a fresh Rest API action and then had to pull in PrivilegesEvaluator, which meant that that certificate authentication for super admin scenarios wasn't supported... definitely a little tail chasing. Coming from the other direction of implementing the config builder and then associating transport layer actions which could be permissioned directly I ran into injection related issues.

I've pushed the work in progress to my fork main...peternied:security:config-access-protos

@prabhat-chaturvedi
Copy link

I meant more of configuration issue but that should be handled well in the code and restrictions put in place so that Customer's don't shoot on their foot.

In your above example, lets assume IT security team has the securityAdmin privileges. But Tenancy is decided only the leadership level. Which means only they should be allowed to change the tenancy and not the IT security team. In that use-case how do we control IT security team not updating tenancy.
Or we assuming that securityConfig permissions are always greater than tenancy permissions.

@peternied
Copy link
Member Author

securityConfig permissions are always greater than tenancy permissions

@prabhat-chaturvedi Yes - this is a limitation of the current security model.

I've finished a draft pull request for how to model this behavior along with tests.

@stephen-crawford
Copy link
Contributor

[Triage] This seems like a desirable change. Thank you for filing the issue and taking the time to create a prototype.

@stephen-crawford stephen-crawford added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Mar 20, 2023
@davidlago
Copy link

@peternied where do we stand on this one after #2607 merged?

@peternied
Copy link
Member Author

I don't know that we have a good use case for this scenario broadly, while it would make exposing many more settings easier, we haven't really discussed intermediate management of configuration values or permissioning parts of the security configuration since this was filed. I'll close and we can revisit in the future.

@cwperks cwperks reopened this Feb 5, 2025
@github-actions github-actions bot added the untriaged Require the attention of the repository maintainers and may need to be prioritized label Feb 5, 2025
@cwperks
Copy link
Member

cwperks commented Feb 5, 2025

Re-opening this issue. There's a use-case that this could be good for like updating the value of config.dynamic.authc.saml_auth_domain.http_authenticator.config.kibana_url.

There could be other dynamic values that would also benefit from a generic solution.

I think we can build a generic solution, but make user's configure an allowlist of securityconfig values that are undateable through this API w/o allowing broad access to the securityconfig PATCH or update APIs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. untriaged Require the attention of the repository maintainers and may need to be prioritized
Projects
None yet
Development

No branches or pull requests

6 participants