-
Notifications
You must be signed in to change notification settings - Fork 392
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
[#4467] docs: Refactor the security document and add access control page #4496
Conversation
docs/security/access-control.md
Outdated
|
||
Catalogs are under the metalake. Catalogs represent different kinds of data sources. | ||
|
||
Gravitino supports Hive, Iceberg, MySQL, PostgreSQL, Hadoop, and Kafka catalogs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you don't have to mention the supported catalogs here, we will add more catalogs, so it needs to be updated continuously. Removing this sentence, so we don't have to update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I remove this.
docs/security/access-control.md
Outdated
Schemas are under the catalog. | ||
|
||
There are tables, topics, or filesets under the schema. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest you write an image about the hierarchical structure of metadata objects here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
docs/security/access-control.md
Outdated
`deny` condition is prior to `allow` condition. If a role has the `allow` condition and `deny` condition at the same time. | ||
|
||
The user won't be able to use the privilege. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't separate into two paragraphs, the sentence is broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will the blank line.
docs/security/access-control.md
Outdated
-H "Content-Type: application/json" -d '{ | ||
roleNames: ["role1"] | ||
}'http://localhost:8090/api/metalakes/test/permissions/users/user1/revoke | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this empty line in here and other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
Can you please add a chapter about security in the |
I updated the index. |
docs/security/access-control.md
Outdated
For example, if you give a use that `SELECT_TABLE` privilege on a catalog, then that the user | ||
|
||
will be able to select(read) all tables in that catalog. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one-line sentence will break into two lines according to your current way of writing markdown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I checked other places, too. Fix them by the way.
docs/security/access-control.md
Outdated
|
||
If parent securable object has the same privilege name with different condition, the parent securable privilege will still take effect. | ||
|
||
For example, securable metalake object allows to use the catalog, but securable catalog denys to use the catalog, the user isn't able to use the catalog. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
denies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix.
docs/security/how-to-authenticate.md
Outdated
|
||
Simple mode is the default authentication option of the server. | ||
|
||
For the client side, if it doesn't set the authentication explicitly, it will use anonymous to access the server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused about this, if user doesn't set authentication, it will be simple mode, the user is anonymous
or from GRAVITINO_USER
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anonymous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if users do not set the authentication explicitly, they will use the simple mode to access the Gravitino server and the corresponding user name is anonymous
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the logic is too odd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many systems have similar logic. If user doesn't enable authentication, the user uses anonymous
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but you said the default authentication is simple
in which the user is not anonymous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If request don't have credential, simple mode think it's anonymous.
docs/security/access-control.md
Outdated
|
||
:::info | ||
|
||
Gravitino only supports authorization and doesn't support metadata authentication. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain this more clearly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know to how to make it clearer. Do you have some suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"doesn't support metadata authentication"
Why do we mention authentication
in the access control chapter?
Furthermore, I believe you need to add more words to make the sentence more natural. for example
Gravitino only supports authorization for secureable objects, when it comes to authentication. Gravitino doesn't support metadata authentication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is metadata authentication
? and As I know Gravitino doesn't support authorization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
authorization
means that write privileges to underlying system.
medata authentication
means that check privileges about metadata, if has privileges to allow the operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should add this to the document to clarify it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
docs/security/access-control.md
Outdated
|
||
:::info | ||
|
||
Gravitino only supports authorization and doesn't support metadata authentication. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"doesn't support metadata authentication"
Why do we mention authentication
in the access control chapter?
Furthermore, I believe you need to add more words to make the sentence more natural. for example
Gravitino only supports authorization for secureable objects, when it comes to authentication. Gravitino doesn't support metadata authentication.
docs/security/access-control.md
Outdated
`COLUMN`, `FILESET`, `TOPIC`, `COLUMN`, `ROLE`, `METALAKE`. A metadata object is combined by a `type` and a | ||
comma-separated `name`. For example, a `CATAGLOG` object has a name "catalog1" with type | ||
"CATALOG", a `SCHEMA` object has a name "catalog1.schema1" with type "SCHEMA", a `TABLE` | ||
object has a name "catalog1.schema1.table1" with type "TABLE". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide an example of metalake metadata objects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docs/security/access-control.md
Outdated
Every securable object resides within a logical container in a hierarchy of containers. | ||
|
||
The top container is the metalake. You can understand that metalake a customer organization. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's your suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put them into paragraphs, currently each sentence is a paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel every sentence is a paragraph will be more clear. It they are a paragraph. The sentence is too long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I merged them.
docs/security/access-control.md
Outdated
|
||
The relationship of the concepts is as below. | ||
|
||
![user_group_relationshi_image](../assets/user-group.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's GroupMappingService
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides, please refine the picture like
Role Table1 reviewer -> Role: Table1 reviewer
Role Fileset 3 scientist -> Role: Fileset3 scientist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you may need to add the following contents: Securable objects consist of metadata object and a set of privileges for the securable objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's
GroupMappingService
?
I will changed it to external user system.
docs/security/access-control.md
Outdated
|
||
### Add a user | ||
|
||
The external systems like LDAP, Scim and etc manage the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is Scim
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a popular user system.
docs/security/how-to-authenticate.md
Outdated
|
||
## Authentication | ||
|
||
Apache Gravitino supports three kinds of authentication mechanisms: simple,OAuth and Kerberos. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space before OAuth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docs/security/how-to-authenticate.md
Outdated
|
||
Simple mode is the default authentication option of the server. | ||
|
||
For the client side, if it doesn't set the authentication explicitly, it will use anonymous to access the server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if users do not set the authentication explicitly, they will use the simple mode to access the Gravitino server and the corresponding user name is anonymous
.
|
||
For the client side, if it doesn't set the authentication explicitly, it will use anonymous to access the server. | ||
|
||
If the client sets the simple mode, it will use the environment variable `GRAVITINO_USER` as the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GRAVITINO_USER
in the client-server....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we support setting a custom user name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, Hadoop has the similar behaviour.
docs/security/how-to-authenticate.md
Outdated
|
||
If the client sets the simple mode, it will use the environment variable `GRAVITINO_USER` as the user. | ||
|
||
If the environment variable `GRAVITINO_USER` isn't set, the client uses the user of the machine that sends requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the meaning of user of the machine
? Is the user currently logged into the client machine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected.
docs/security/access-control.md
Outdated
|
||
:::info | ||
|
||
Gravitino only supports authorization for secureable objects, when it comes to authentication. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the meaning of "when it comes to authentication", this sentence is hard to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @yuqi1129
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I want to express: the second sentence Gravitino doesn't support metadata authentication.
is about authentication because the first sentence is just about authorization
. The sentences in this passage are unnaturally connected.
docs/security/access-control.md
Outdated
|
||
A metadata object to which access can be granted. Unless allowed by a grant, access is denied. | ||
Every securable object resides within a logical container in a hierarchy of containers. | ||
The top container is the metalake. You can understand that metalake a customer organization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the meaning of "You can understand"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
docs/security/access-control.md
Outdated
|
||
The relationship of the concepts is as below. | ||
|
||
![user_group_relationshi_image](../assets/user-group.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"relationship"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
docs/security/access-control.md
Outdated
|-------------|---------------------------|---------------------| | ||
| ManageUsers | Metalake | Add or remove users | | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove one blank line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
docs/security/access-control.md
Outdated
|
||
::: | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this one blank line here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
</Tabs> | ||
|
||
## Group Operation | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this one blank line here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
docs/security/how-to-authenticate.md
Outdated
6. You can refer to the [Configurations](gravitino-server-config.md) and append the configurations to the conf/gravitino.conf. | ||
|
||
```text | ||
gravitino.authenticator = oauth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember this key has been replaced with gravitino.authenticators
, Am I correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't merge this document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't get your word We don't merge this document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#4489 This pull request.
docs/security/how-to-authenticate.md
Outdated
|
||
Gravitino supports Kerberos mode. | ||
|
||
For the server side, users should set `gravitino.authenticator` as `kerberos` and give |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
docs/security/how-to-authenticate.md
Outdated
|
||
First, users need to guarantee that the external correctly configured OAuth 2.0 server supports Bearer JWT. | ||
|
||
Then, on the server side, users should set `gravitino.authenticator` as `oauth` and give |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
|
||
For the client side, if it doesn't set the authentication explicitly, it will use anonymous to access the server. | ||
|
||
If the client sets the simple mode, it will use the environment variable `GRAVITINO_USER` as the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we support setting a custom user name?
|
||
If parent securable object has the same privilege name with different condition, the securable object won't override the parent object privilege. | ||
For example, securable metalake object allows to use the catalog, but securable catalog denies to use the catalog, the user isn't able to use the catalog. | ||
If securable metalake object denies to use the catalog, but securable catalog allows to use the catalog, the user isn't able to use the catalog, too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
catalog, too -> catalog too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/security/access-control.md
Outdated
|
||
A metadata object to which access can be granted. Unless allowed by a grant, access is denied. | ||
Every securable object resides within a logical container in a hierarchy of containers. | ||
The top container is the metalake. You can understand that metalake a customer organization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can view metalake as a xxxxx.
The word understand
is not elegant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove this sentence.
docs/security/access-control.md
Outdated
|
||
### User | ||
|
||
A user identity recognized by Gravitino. External user system instead of Gravitino manages users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excessive empty space before External
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docs/security/access-control.md
Outdated
`COLUMN`, `FILESET`, `TOPIC`, `COLUMN`, `ROLE`, `METALAKE`. A metadata object is combined by a `type` and a | ||
comma-separated `name`. For example, a `CATAGLOG` object has a name "catalog1" with type | ||
"CATALOG", a `SCHEMA` object has a name "catalog1.schema1" with type "SCHEMA", a `TABLE` | ||
object has a name "catalog1.schema1.table1" with type "TABLE". `METALAKE` object has a name "metalake1". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a METALAKE
object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docs/security/access-control.md
Outdated
|
||
Every metadata object has an owner. The owner could be a user or group. | ||
The owner have all the privileges of the metadata object. | ||
The owner could be transferred to another user or group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please optimize these three sentences that start with The owner
. It's too tedious to have the same starting points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…trol page (apache#4496) ### What changes were proposed in this pull request? Refactor the security document and add access control page. ### Why are the changes needed? Fix: apache#4467 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Just docs.
What changes were proposed in this pull request?
Refactor the security document and add access control page.
Why are the changes needed?
Fix: #4467
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Just docs.