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

Adds documentation for REST admin API permissions feature #4257

Merged

Conversation

DarshitChanpura
Copy link
Member

Issues Resolved

Resolves #4256

Checklist

  • By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and subject to the Developers Certificate of Origin.
    For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@@ -36,6 +36,37 @@ plugins.security.restapi.endpoints_disabled.<role>.<endpoint>: ["<method>", ...]
```
{% include copy.html %}

You can also control access to specific rest APIs via roles. Here is an example role which allows access to all endpoints supported by this feature currently:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
You can also control access to specific rest APIs via roles. Here is an example role which allows access to all endpoints supported by this feature currently:
You can also control access to specific rest APIs using roles. Here is an example `cluster_permissions` lists which allows access to all endpoints supported by this feature:

@DarshitChanpura
Copy link
Member Author

@cwillum @Naarcha-AWS Would you please re-review this?

Copy link
Contributor

@cwillum cwillum left a comment

Choose a reason for hiding this comment

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

@DarshitChanpura A huge thanks for taking charge on this and getting the documentation going. See my comments and let me know if you want to talk about adding a new doc PR to include a table in Permissions that describes the new REST permissions.

@@ -36,6 +36,37 @@ plugins.security.restapi.endpoints_disabled.<role>.<endpoint>: ["<method>", ...]
```
{% include copy.html %}

You can also control access to specific rest APIs using roles. Here is an example list `cluster_permissions` that you can attach to a role which will allow access to all endpoints supported by this feature:
Copy link
Contributor

@cwillum cwillum Jun 20, 2023

Choose a reason for hiding this comment

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

Suggested change
You can also control access to specific rest APIs using roles. Here is an example list `cluster_permissions` that you can attach to a role which will allow access to all endpoints supported by this feature:
Roles also allow you to control access to specific REST APIs. You can add individual or multiple cluster permissions to a role and grant users access to associated APIs when they are mapped to the role. The following list of cluster permissions includes the endpoints that correspond to the Security REST APIs:

It would also be nice to have these in table format with descriptions for each so users know exactly which APIs can be accessed when one is added to the role. Something like the table in Security Analytics permissions.
What do you think about cutting a new PR off this issue? Otherwise, we could create a new doc issue.

@@ -36,6 +36,37 @@ plugins.security.restapi.endpoints_disabled.<role>.<endpoint>: ["<method>", ...]
```
{% include copy.html %}

You can also control access to specific rest APIs using roles. Here is an example list `cluster_permissions` that you can attach to a role which will allow access to all endpoints supported by this feature:
```yml
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this section is all about APIs, I think we should simply list these in bullets, not provide a YAML config example.

  • restapi:admin/actiongroups
  • restapi:admin/allowlist
  • restapi:admin/internalusers
  • restapi:admin/nodesdn
  • restapi:admin/roles
  • restapi:admin/rolesmapping
  • restapi:admin/ssl/certs/info
  • restapi:admin/ssl/certs/reload
  • restapi:admin/tenants

- 'restapi:admin/ssl/certs/reload'
- 'restapi:admin/tenants'
```
You can add/remove these as required to your role definition. It currently supports access control to these 9 endpoints:
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, it might be more helpful to describe the corresponding APIs rather than list the endpoints, which are obvious in the cluster permissions. Rather than list them here, maybe we can have them in a table in the "Cluster permissions" documentation, like I mentioned above?

Permission Description
restapi:admin/actiongroups Permission to get, delete, create, and patch actions groups
restapi:admin/allowlist Permission to add any endpoints and HTTP requests to a list of allowed endpoints and requests.
restapi:admin/nodesdn Permission to add, retrieve, update, or delete any distinguished names from an allow list and enable communication between clusters and/or nodes.

I know we have an allowlist.yml config file. But I don't know how you would use the REST API to configure that. I don't see that API in this section.

Copy link
Member Author

Choose a reason for hiding this comment

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

its to dynamically update allowlist via APIs

@@ -393,3 +393,19 @@ These permissions apply to an index or index pattern. You might want a user to h
- indices:monitor/shard_stores
- indices:monitor/stats
- indices:monitor/upgrade


## Rest Permissions
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## Rest Permissions
## REST Permissions

Are these Security specific?

If yes, maybe "Security REST permissions".

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, since these APIs are security specific, so are these permissions. However, style check fails if I say REST instead of Rest

Copy link
Contributor

Choose a reason for hiding this comment

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

If this stands for "Representational State Transfer" API, you can disregard the style check and capitalize. But if this refers to the rest layer (versus transit layer), it's my fault and it should stay Security rest permissions. Thanks.

@@ -1336,6 +1369,29 @@ PUT _plugins/_security/api/nodesdn/<cluster-name>
}
```

### Update all distinguished names
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this differ from "Update distinguished names" using a PUT call? Just a brief blurb describing the API.

@@ -1336,6 +1369,29 @@ PUT _plugins/_security/api/nodesdn/<cluster-name>
}
```

### Update all distinguished names
#### Request
Copy link
Contributor

@cwillum cwillum Jun 21, 2023

Choose a reason for hiding this comment

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

Can we add a table before the Request for request fields? They might be obvious to most people (I'm not in that group). But if we can add some more description ...

Request fields

You can specify the following fields when updating all DNs.

Field Type Description
op String Operation to be performed. Options include remove, ... Required.
path String Path to the target distinguished name to be updated. Required.

Are these fields defined some way to update all? Does the 0 in the path update all?

And then "#### Request" would follow after this heading.

Update: just wanted to add the resource that this comment is based on. We created an API style guide end of last year to help us format API documentation and keep it consistent. API Style Guide

}
```

### Reload http certificates
Copy link
Contributor

Choose a reason for hiding this comment

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

Reload HTTP certificates

Copy link
Member Author

Choose a reason for hiding this comment

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

upon replacing with capitals, style check failed.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it refers to "HyperText Transfer Protocol", capitalization is good. When http points to a setting or something else in the code, we leave it without capitalization to show how it's represented as a component of the code.
We're still working out the kinks with the style check. Thanks.

## Rest Permissions

These permissions apply to rest APIs to control access to the endpoints. Granting access to any of these will allow access to change the crucial operational components of Security plugin.
NOTE: Allowing access to these endpoints can trigger operational changes in the cluster. Proceed with caution.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
NOTE: Allowing access to these endpoints can trigger operational changes in the cluster. Proceed with caution.
Allowing access to these endpoints has the potential to trigger operational changes in the cluster. Proceed with caution.
{: .warning }

You can add the Liquid tag below and it will format this as a warning. Since you can really mess things up, I think warning is suitable.


These permissions apply to rest APIs to control access to the endpoints. Granting access to any of these will allow access to change the crucial operational components of Security plugin.
NOTE: Allowing access to these endpoints can trigger operational changes in the cluster. Proceed with caution.

Copy link
Contributor

Choose a reason for hiding this comment

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

If these are cluster permissions, let's format them like the others in the Permissions documentation and add them in the section above index permissions, Cluster permissions. So, the whole enchilada:

Security REST API permissions

Allowing access to these endpoints has the potential to trigger operational changes in the cluster. Proceed with caution.
{: .warning }

See API for more information about Security REST APIs.

  • cluster:restapi:admin/actiongroups
  • cluster:restapi:admin/allowlist
  • cluster:restapi:admin/internalusers
  • cluster:restapi:admin/nodesdn
  • cluster:restapi:admin/roles
  • cluster:restapi:admin/rolesmapping
  • cluster:restapi:admin/ssl/certs/info
  • cluster:restapi:admin/ssl/certs/reload
  • cluster:restapi:admin/tenants

Copy link
Member Author

Choose a reason for hiding this comment

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

these permissions are not prefixed with cluster:, so these should be fine.

@cwillum
Copy link
Contributor

cwillum commented Jun 24, 2023

@DarshitChanpura Was thinking about this. It will take a little time to format this according to the doc team's style guide, which isn't your job. I'll look into how we do a hand off (or something similar) at the start of next week. Thanks again for getting this into documentation.

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Copy link
Contributor

@cwillum cwillum left a comment

Choose a reason for hiding this comment

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

Great! a few comments. But looks good. Thank you.


| **Permission** | **APIs Granted** | **Description** |
|:-------------------------------|:---------------------------------|:--------------------------------------------------------------------------------------------------------------------------------------------------|
| restapi:admin/actiongroups | `/actiongroup` & `/actiongroups` | Permission to get, delete, create, and patch actions groups (including bulk updates) |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| restapi:admin/actiongroups | `/actiongroup` & `/actiongroups` | Permission to get, delete, create, and patch actions groups (including bulk updates) |
| restapi:admin/actiongroups | `/actiongroup` & `/actiongroups` | Permission to get, delete, create, and patch actions groups (including bulk updates). |

Roles also allow you to control access to specific REST APIs. You can add individual or multiple cluster permissions to a role and grant users access to associated APIs when they are mapped to the role. The following list of cluster permissions includes the endpoints that correspond to the Security REST APIs:

| **Permission** | **APIs Granted** | **Description** |
|:-------------------------------|:---------------------------------|:--------------------------------------------------------------------------------------------------------------------------------------------------|
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to add extra spaces or "filler" dashes to make tables render properly in the markdown. And the headings should automatically be boldface, too, without having to add the double asterisks. This should do it:
| Permission | APIs Granted | Description |
| :--- | :--- | :--- |
| Permission 1 | allowlist | Does this and that |

Permission APIs Granted Description
Permission 1 allowlist Does this and that

Copy link
Member Author

Choose a reason for hiding this comment

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

These were auto-generated by my IDE. I guess for better readability

|:-------------------------------|:---------------------------------|:--------------------------------------------------------------------------------------------------------------------------------------------------|
| restapi:admin/actiongroups | `/actiongroup` & `/actiongroups` | Permission to get, delete, create, and patch actions groups (including bulk updates) |
| restapi:admin/allowlist | `/allowlist` | Permission to add any endpoints and HTTP requests to a list of allowed endpoints and requests. |
| restapi:admin/internalusers | `/internaluser` & `/user` | Permission to add, retrieve, modify and delete any user in the cluster |
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a hassle. But we follow the "Oxford comma" rule: a comma between all elements of a list. As below.

Suggested change
| restapi:admin/internalusers | `/internaluser` & `/user` | Permission to add, retrieve, modify and delete any user in the cluster |
| restapi:admin/internalusers | `/internaluser` & `/user` | Permission to add, retrieve, modify, and delete any user in the cluster. |

Global comment.

| restapi:admin/allowlist | `/allowlist` | Permission to add any endpoints and HTTP requests to a list of allowed endpoints and requests. |
| restapi:admin/internalusers | `/internaluser` & `/user` | Permission to add, retrieve, modify and delete any user in the cluster |
| restapi:admin/nodesdn | `/nodesdn` | Permission to add, retrieve, update, or delete any distinguished names from an allow list and enable communication between clusters and/or nodes. |
| restapi:admin/roles | `/roles` | Permission to add, retrieve, modify and delete any roles in the cluster |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| restapi:admin/roles | `/roles` | Permission to add, retrieve, modify and delete any roles in the cluster |
| restapi:admin/roles | `/roles` | Permission to add, retrieve, modify, and delete any roles in the cluster. |

| restapi:admin/internalusers | `/internaluser` & `/user` | Permission to add, retrieve, modify and delete any user in the cluster |
| restapi:admin/nodesdn | `/nodesdn` | Permission to add, retrieve, update, or delete any distinguished names from an allow list and enable communication between clusters and/or nodes. |
| restapi:admin/roles | `/roles` | Permission to add, retrieve, modify and delete any roles in the cluster |
| restapi:admin/rolesmapping | `/rolesmapping` | Permission to add, retrieve, modify and delete any roles-mapping |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| restapi:admin/rolesmapping | `/rolesmapping` | Permission to add, retrieve, modify and delete any roles-mapping |
| restapi:admin/rolesmapping | `/rolesmapping` | Permission to add, retrieve, modify, and delete any roles-mapping. |

{
"op":"remove",
"path":"/cluster1/nodes_dn/0"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to add a mock value to the example:

"value": "CN=Karen Berge,CN=admin,DC=corp,DC=Fabrikam,DC=COM", "CN=George Wall,CN=admin,DC=corp,DC=Fabrikam,DC=COM",

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, but since this is a remove operation we don't need it. If we want to add a sample value, I will change the example

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. Thank you.


| Field | Data Type | Description |
|:--------|:----------|:----------------------------------------------------------------------------------|
| status | string | Indicates the status of the operation. Possible values: "OK" or an error message. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| status | string | Indicates the status of the operation. Possible values: "OK" or an error message. |
| status | String | Indicates the status of the operation. Possible values: "OK" or an error message. |

| Field | Data Type | Description |
|:--------|:----------|:----------------------------------------------------------------------------------|
| status | string | Indicates the status of the operation. Possible values: "OK" or an error message. |
| message | string | Additional information about the operation. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| message | string | Additional information about the operation. |
| message | String | Additional information about the operation. |

}
```

#### Response fields
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to leave this table. But the example response is probably self explanatory for this. It can stay or go.

Copy link
Member Author

Choose a reason for hiding this comment

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

I followed the API Style Guide. Let's keep it?


## Security REST permissions

These permissions apply to rest APIs to control access to the endpoints. Granting access to any of these will allow access to change the crucial operational components of Security plugin.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
These permissions apply to rest APIs to control access to the endpoints. Granting access to any of these will allow access to change the crucial operational components of Security plugin.
These permissions apply to REST APIs to control access to the endpoints. Granting access to any of these will allow a user permission to change fundamental operational components of the Security plugin.

OK?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
{
"op":"remove",
"path":"/cluster1/nodes_dn/0"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. Thank you.

@cwillum
Copy link
Contributor

cwillum commented Jun 28, 2023

@DarshitChanpura Merging now. Thanks!

@cwillum cwillum merged commit 31c8e1e into opensearch-project:main Jun 28, 2023
4 checks passed
@cwillum cwillum added security release-notes PR: Include this PR in the automated release notes labels Jun 28, 2023
@kolchfa-aws kolchfa-aws changed the title Adds documentation for rest admin api permissions feature Adds documentation for REST admin API permissions feature Jul 24, 2023
harshavamsi pushed a commit to harshavamsi/documentation-website that referenced this pull request Oct 31, 2023
…-project#4257)

* Adds documentation for rest admin api permissions feature

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

* Address PR feedback

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

* Address CI check failure

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

* Addresses PR feedback

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

* Fixes grammar and styles

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

---------

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
vagimeli pushed a commit that referenced this pull request Dec 21, 2023
* Adds documentation for rest admin api permissions feature

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

* Address PR feedback

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

* Address CI check failure

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

* Addresses PR feedback

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

* Fixes grammar and styles

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

---------

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes PR: Include this PR in the automated release notes security v2.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DOC] Add doc about new set of permissions introduced in security plugin
3 participants