Skip to content

[#10881] feat(authentication): Add local authentication design document#10883

Merged
jerryshao merged 42 commits intoapache:mainfrom
lasdf1234:local-idp
Apr 30, 2026
Merged

[#10881] feat(authentication): Add local authentication design document#10883
jerryshao merged 42 commits intoapache:mainfrom
lasdf1234:local-idp

Conversation

@lasdf1234
Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This PR adds a new design document, design-docs/gravitino-local-idp.md, for Local IDP support in Gravitino.

The document covers:

  • the motivation and goals for introducing a built-in local IDP mode
  • the Basic authentication model and related configuration
  • password hashing design based on Argon2id
  • the local identity data model, including local_user_meta, local_group_meta, and local_group_user_rel
  • the logical mapping between local identity tables and existing user_meta / group_meta
  • bootstrap account behavior
  • authentication flow and transport security considerations
  • local user/group/password management APIs

Why are the changes needed?

Gravitino currently relies on external identity providers for OAuth-based authentication. That works well in enterprise environments, but it creates friction for POC, offline, isolated, and emergency-fallback scenarios.

This design document captures a lightweight Local IDP proposal so the feature scope, storage model, authentication flow, and management APIs are clearly defined before implementation.

Fix: N/A

Does this PR introduce any user-facing change?

Yes.

It adds a new design document describing the proposed Local IDP feature, including:

  • a new basic authenticator mode
  • a proposed gravitino.authenticator.basic.algorithm configuration key
  • new local identity management APIs for users, groups, group membership, and password changes

How was this patch tested?

Documentation-only change.
Reviewed the document structure and aligned it with the existing design-docs style.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 28, 2026 06:30
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a new design document proposing a built-in “Local IDP” mode for Apache Gravitino, primarily aimed at POC/offline/emergency-fallback deployments where external OAuth IdPs are undesirable or unavailable.

Changes:

  • Introduces a Local IDP proposal using HTTP Basic authentication and relational-store-backed identity data.
  • Specifies password hashing/storage using Argon2id with PHC-style encoded hash strings.
  • Describes proposed global (non-metalake-scoped) local user/group tables and management APIs, plus bootstrap/admin behavior and security notes.

Comment thread design-docs/gravitino-local-idp.md Outdated
Comment thread design-docs/gravitino-local-idp.md Outdated
Comment thread design-docs/gravitino-local-idp.md Outdated
Comment thread design-docs/gravitino-local-idp.md
Comment thread design-docs/gravitino-local-idp.md Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@lasdf1234 lasdf1234 changed the title [MINOR] docs: add local IDP design document [#10881] [MINOR] docs: add local IDP design document Apr 28, 2026
@lasdf1234 lasdf1234 changed the title [#10881] [MINOR] docs: add local IDP design document [#10881] feat(core): add local IDP design document Apr 28, 2026
@lasdf1234 lasdf1234 changed the title [#10881] feat(core): add local IDP design document [#10881] feat(core): Add local IDP design document Apr 28, 2026
Comment thread design-docs/gravitino-local-idp.md Outdated
Comment thread design-docs/gravitino-local-idp.md Outdated
Comment thread design-docs/gravitino-local-idp.md Outdated
| Field | Type | Description |
|---|---|---|
| `name` | `String` | User name |
| `audit` | `AuditDTO` | Audit information |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need audit information here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is consistent with the previous similar interface, so "audit info" has been added.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we can keep it simple first, we can add it when required for the user.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would it be possible to remove the audit information from the local auth-related interfaces?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can remove them first. We can add them if required by users.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got i have fixed it.

Comment thread design-docs/gravitino-local-idp.md Outdated
| Item | Value |
|---|---|
| Method | `PUT` |
| Path | `/api/users/{user}/password` |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do u need use the password here? Is /api/users/{user} enough?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, remove "password" at the end.

Comment thread design-docs/gravitino-local-idp.md Outdated

- Only service admin can change any account password.
- The password cannot contain a colon (`:`). (RFC 7617)
- The password must be at least 12 characters long and at most 64 characters long. (OWASP)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's meaning of OWASP?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A general cryptography association can remove this. I'll submit the code.

Comment thread design-docs/gravitino-local-idp.md Outdated
- local IdP mode is recommended for POC, offline rather than as a
replacement for enterprise-grade external identity systems

---
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How did the Trino handle the Basic authentication?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will conduct further tests on this. If the use of Trino requires code modification for verification, I will open a separate issue/pr to address this problem.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should a segment to explain this point.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

trinodb/trino#29132 have found this PR for Trino that can support this scenario. This PR was merged on April 29, 2026 at 6:00 (Beijing time).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should add the segment to explain this point.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My document has been updated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

you can find it in 8.3

Comment thread design-docs/gravitino-local-idp.md Outdated
|---|---|
| Method | `GET` |
| Path | `/api/users/{user}` |
| Permission | Only service admin / owner can execute |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are these interfaces optional? Do we have a configuration option to control them?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These interfaces are currently fixed. Should we add a feature to determine whether basic authentication is enabled or not, in order to decide whether these interfaces can be accessed?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This interfaces should be optional.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got I have been modified.These interfaces can only be called when the user uses "basic" seriously.

Comment thread design-docs/gravitino-local-idp.md Outdated

| Error case | HTTP status |
|---|---|
| Password verification failed | `401` |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have concern about these error codes. Are they necessary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I removed the 428 and other less commonly used HTTP codes, and only retained codes like 401, which are common authentication failure codes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Especially for 422 Unprocessable Content, Is it common usage?

Copy link
Copy Markdown
Contributor

@roryqi roryqi Apr 28, 2026

Choose a reason for hiding this comment

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

This comment isn't resolved. You shouldn't mark this resolved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Source of the specification: Initially defined in RFC 4918 of the WebDAV extension, it has now been incorporated into the core specification of HTTP semantics and content RFC 9110, becoming an Internet standard.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Systems That Use HTTP 422 Status Code
HTTP 422 (Unprocessable Entity) originated as a WebDAV extension but is now widely adopted in modern RESTful APIs for semantic validation errors.

🌐 API Endpoints / Gateways & Cloud Platforms
Mainstream Web Frameworks (provide built-in support for returning 422):

Laravel (PHP): Returns 422 automatically for AJAX request validation failures.

Rails (Ruby) & Devise: Returns 422 when Turbo-driven form validations fail.

Drupal (PHP): Provides UnprocessableHttpEntityException class for throwing 422 exceptions.

Yii (PHP): Offers yii\web\UnprocessableEntityHttpException.

ASP.NET Core (C#): Includes UnprocessableEntity() result type.

Apache HttpClient (Java): Defines SC_UNPROCESSABLE_ENTITY constant.

Ruby: Gem::Net::HTTPUnprocessableEntity class for handling 422.

Specific Services & Components:

Red Hat Satellite: Returns 422 via its API when creating host entries with invalid parameters.

CnosDB: Uses 422 in its REST API to indicate operation execution failure.

FOLIO: Returns 422 with "EMAIL_ALREADY_EXIST" error when a user registers with an existing email.

NHS e-Referral Service: Returns 422 in its FHIR API when a request is unprocessable due to business logic (e.g., invalid relationship).

📡 Web Servers & Storage Systems
WebDAV servers: Use 422 when handling complex or malformed XML request bodies.

NetApp E-Series (SANtricity): Returns 422 when enabling/disabling AutoSupport with invalid parameters.

Dell PowerProtect Data Manager (versions <19.18): Shows a 422 error in the UI when configuring network with an invalid "search domain" value.

IBM Systems (AIX, z/OS): 422 may appear as a specific system error code.

📱 Operating Systems & Application Layer
Microsoft AD FS: Web Application Proxy (WAP) servers log Event ID 422 when idle connection timeout occurs (e.g., 100 seconds of inactivity).

Bittitan: Reports 422 Unprocessable Entity errors when interacting with Exchange WebDAV API on unprocessable mailbox folders.

OutSystems (mobile app framework): Returns 422 when data sent to an API fails validation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do they use 422 to present that old password same as the new one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

image The core reason for using 422 is: when the request format is correct but the data is logically invalid, 422 is the most appropriate status code – unlike 400 Bad Request, which only indicates a malformed syntax.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe you can use 400 Bad Request here. Because it represents illegalArguements in our system instead of format exception. I prefer using 400. You can listen to others' opinion.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got

Comment thread design-docs/gravitino-local-idp.md Outdated

## 10. Security Considerations

The local IdP is intentionally lightweight, but the following constraints remain important:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could u give me the user process, bootstrap process and verification?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have included this part of the content in sections 6.2 and 7.1.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The code has been submitted and the design document has been revised.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You don't get the point about process. You should add user process instead of implementation process.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got I have add in document : "### 6.3 Example Basic Authentication Bootstrap Flow"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can't find it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ctrl + F you can find it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Image You don't have.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got resolved

Comment thread design-docs/gravitino-local-idp.md
lasdf1234 and others added 8 commits April 28, 2026 15:05
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread design-docs/gravitino-local-idp.md Outdated

- only one bootstrap account is created by default: **service admin**
- the default password is **123456**
- after initial login, the administrator is expected to change the password immediately
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is used for UI login. REST API is hard to judge the first login.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The web filter can filter such requests. If it is the "serviceAdmin" account and there is no password for "serviceAdmin" in the database, then only the "serviceAdmin" account is allowed to call the interface for changing the password.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cc @jerryshao WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I plan to adopt a different approach by using 'interactive script' to initialize the authentication of serviceAdmin. This will generate an SQL statement that will be directly written into the database.

Comment thread design-docs/gravitino-local-idp.md Outdated

| Key | Default | Optional Values |
|---|---|---|
| `gravitino.authenticator.basic.algorithm` | `Argon2id` | `Argon2id` |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is gravitino.authenticator.basic.algorithm a good name?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

gravitino.authenticator.basic.password-hash-algorithm I have changed config name to this one.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could u borrow other system's experience? Notice, we should use camel style.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

image I think gravitino.authenticator.basic.password-hashing.algorithm this is the best, do you think so?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Prefer passwordHasher

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got I have resolved and push code.

2. Start Gravitino.

3. On first startup, no active `adminUser` record exists yet in `local_user_meta`, so Gravitino
accepts the bootstrap credential `adminUser:123456` only for the bootstrap login and immediate
Copy link
Copy Markdown
Contributor

@roryqi roryqi Apr 28, 2026

Choose a reason for hiding this comment

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

How to determine bootstrap login, this is not easy for REST.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For basic authentication, the user is stateless. Therefore, logic can be added in the filter: check the local_user_meta table. If the serviceAdmin does not have any data in this table, then the serviceAdmin must not have been registered in the built-in IDP. At this time, the filter will intercept all requests except for "change password" and prompt the serviceAdmin account to change the password.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Collect others' suggestion.

Copy link
Copy Markdown
Contributor Author

@lasdf1234 lasdf1234 Apr 29, 2026

Choose a reason for hiding this comment

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

I plan to adopt a different approach by using 'interactive script' to initialize the authentication of serviceAdmin. This will generate an SQL statement that will be directly written into the database.

These tables follow Gravitino's existing metadata table conventions:

- numeric primary keys,
- `audit_info`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is audit info required?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got i have resolved it.

lasdf1234 and others added 5 commits April 29, 2026 10:40
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

```shell
curl -X GET -H "Accept: application/vnd.gravitino.v1+json" \
http://localhost:8090/api/idp/users/alice
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the response?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got I have supplemented the responses for all the interfaces.

| Error case | HTTP status |
|---|---|
| Account doesn't exist | `404` |
| Password same with old | `422` |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have concern about this error code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got I have fixed it

You can add users to a local group by providing the group name in the path and the target user
names in the request body.

The request path for REST API is `/api/idp/groups/{group}/users/add`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems that you have a user called add.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got it. I removed "/users" from the URL and have submitted the code.

Comment thread design-docs/gravitino-local-authentication.md

The recommended module name is:

- `authenticators:authenticator-local-authentication`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not aligned with one mentioned above. Also, the name is too long.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got, 'authenticators/authenticator-basic' is better. It is shorter and more in line with the semantic meaning of the context.

Comment on lines +184 to +185
Unlike Gravitino's existing `user_meta` and `group_meta` tables, `local_user_meta` and
`local_group_meta` are intentionally designed as **global identity tables** and therefore **do not
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Make the table name more meaningful, local_xxx will easily be confused with existing table names.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

May be idp_user_meta/idp_group_meta is more meaningful.The URL of the newly added interface also includes "idp".

- only one bootstrap account is created by default: the configured **service admin** account (for
example, **adminUser**)
- the default password is **123456**
- the default password **123456** is intended only for the initial bootstrap login and the immediate
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would suggest to change the password to like "gravitino".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got, the document has been modified.

admin account (for example, **adminUser**) with password **123456** to pass Basic verification
only for the bootstrap login and immediate password reset flow.
3. Reject other management operations until the bootstrap password has been reset successfully.
4. During the first successful password reset, create the bootstrap service admin record and store
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How to reset the password, does it happen automatically or the service admin should do it manually?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The process has been changed. An 'interactive script' will be used, and the password will be changed by the service admin actively.


```properties
gravitino.authenticators=basic
gravitino.authenticator.basic.passwordHasher=Argon2id
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you need to make this a configuration? I don't think it is necessary. If you want to make it a configuration, you need to provide different choices, do you support this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got, Currently only supports this one encryption algorithm and there are no other options available. This configuration can be removed.
The design document has been modified.

-H 'Content-Type: application/json' \
https://<gravitino-host>/api/idp/users/adminUser \
-d '{
"password": "ChangeMeToAStrongPassword"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it safe to transfer the password without hashing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, transmitting passwords in plain text is secure. However, certain conditions must be met: 1. HTTPS 2. TLS 1.2 or a higher version 3. The server does not log passwords (log desensitization).
OWASP and NIST also recommend using this solution.

```shell
curl -X DELETE -H "Accept: application/vnd.gravitino.v1+json" \
http://localhost:8090/api/idp/groups/engineering
```
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we remove a group if this group still has users?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, when there are still users in the user group, the group can be removed. This interface will first remove the relationship between group and users, and then remove the group. The design of this interface is based on the design of the "remove role" interface.
Spec have also been incorporated into the design document.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This may be dangerous, you can have a query parameter like force to let the user choose. You can refer to schema / catalog deletion implementation.

```shell
curl -X PUT -H "Accept: application/vnd.gravitino.v1+json" \
-H "Content-Type: application/json" -d '{
"userNames": ["alice"]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Simplify all the userNames to users.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got, 'users' is better. I have modified the document.

lasdf1234 and others added 7 commits April 29, 2026 15:31
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines +293 to +296
- the JDBC URL
- the Gravitino database name
- the JDBC user name
- the JDBC password
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can be read from the configuration. If user they want to start Gravitino, they have to configure the JDBC information, so that can be shared here, no need to let user to input again.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Besides, I think this script is only needed for local authentication, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

1.Yes, JDBC information can be read from other configuration files.
2.Yes, this script is only for local authentication.

7. Hash the password with Argon2id.
8. Generate the `INSERT` statement required for the target JDBC backend and execute it immediately
so the service admin record is written into `idp_user_meta`.
9. Exit successfully only after the insert has been committed.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What happens if the initialization script is not running before the start?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you sync with @danhuawang and @geyanggang about how to use it in the docker and k8s environment?

Copy link
Copy Markdown
Contributor

@jerryshao jerryshao Apr 29, 2026

Choose a reason for hiding this comment

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

Frankly saying, I'm in favor of the previous solution to have a default password, and let service admin to reset it for the first time.

To avoid hardcode issue, we can leave initial password as blank, then ask service admin to reset to a valid one.

Using a separate script and interactive solution makes things complicated, and hard to use in a container deployment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. if the initialization script is not running, the filter will block all requests.(Due to service admin has no password)
  2. Got, I only discussed this matter with Rory. Now I'm going to discuss it with @danhuawang and @geyanggang

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with the idea leaving initial password as blank, then ask service admin to reset it for the first time access. Using a separate script only for password init is not convenient.

- **interactive service admin initialization**

The result is a self-contained authentication path that is easy to deploy, fast to evaluate, and
well aligned with Gravitino's lightweight quick-start experience.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add a chapter to describe you work plan and checklist, it will be helpful for reviewer and AI to check.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got I will follow the style of other design documents to complete the plan and checklist.

lasdf1234 and others added 4 commits April 29, 2026 20:43
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown

Code Coverage Report

Overall Project 65.49% 🟢
Files changed No Java source files changed -

Module Coverage
aliyun 1.73% 🔴
api 47.13% 🟢
authorization-common 85.96% 🟢
aws 1.1% 🔴
azure 2.6% 🔴
catalog-common 10.2% 🔴
catalog-fileset 80.02% 🟢
catalog-glue 82.47% 🟢
catalog-hive 81.83% 🟢
catalog-jdbc-clickhouse 79.06% 🟢
catalog-jdbc-common 43.93% 🟢
catalog-jdbc-doris 80.28% 🟢
catalog-jdbc-hologres 54.03% 🟢
catalog-jdbc-mysql 79.23% 🟢
catalog-jdbc-oceanbase 78.38% 🟢
catalog-jdbc-postgresql 82.05% 🟢
catalog-jdbc-starrocks 78.27% 🟢
catalog-kafka 77.01% 🟢
catalog-lakehouse-generic 45.07% 🟢
catalog-lakehouse-hudi 79.1% 🟢
catalog-lakehouse-iceberg 86.98% 🟢
catalog-lakehouse-paimon 77.71% 🟢
catalog-model 77.72% 🟢
cli 44.51% 🟢
client-java 77.63% 🟢
common 48.75% 🟢
core 81.5% 🟢
filesystem-hadoop3 76.97% 🟢
flink 40.55% 🟢
flink-runtime 0.0% 🔴
gcp 14.2% 🔴
hadoop-common 10.39% 🔴
hive-metastore-common 46.83% 🟢
iceberg-common 55.24% 🟢
iceberg-rest-server 66.95% 🟢
integration-test-common 0.0% 🔴
jobs 66.17% 🟢
lance-common 23.88% 🔴
lance-rest-server 57.84% 🟢
lineage 53.02% 🟢
optimizer 82.95% 🟢
optimizer-api 21.95% 🔴
server 85.6% 🟢
server-common 70.01% 🟢
spark 32.79% 🔴
spark-common 39.09% 🔴
trino-connector 34.28% 🔴

@jerryshao jerryshao merged commit 42917aa into apache:main Apr 30, 2026
26 checks passed
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.

5 participants