Repository navigation
fix(security): restrict service connection reads to bots and editors - #36219
Conversation
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
backend
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
47bda69 to
6467045
Compare
mohityadav766
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| JsonUtils.convertValue(asAdmin.getConnection().getConfig(), AmundsenConnection.class); | ||
| assertEquals("*********", adminConn.getPassword()); | ||
|
|
||
| // VIEW_BASIC user: no connection at all. |
There was a problem hiding this comment.
The new comments label the results asserted directly below them, such as “no connection at all” before assertNull(asUser.getConnection()). The repository requires comments to explain why rather than restate obvious code. Remove these labels or explain a non-obvious reason for the cases. This requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
manerow
left a comment
manerow
left a comment
There was a problem hiding this comment.
One gap in the migration, see inline. Rest looks good.
| UPDATE metadata_service_entity | ||
| SET json = JSON_REMOVE(json, '$.connection.config.elasticsSearch', '$.connection.config.securityConfig') | ||
| WHERE name = 'OpenMetadata' | ||
| AND (JSON_EXTRACT(json, '$.connection.config.elasticsSearch') IS NOT NULL | ||
| OR JSON_EXTRACT(json, '$.connection.config.securityConfig') IS NOT NULL); |
There was a problem hiding this comment.
The migration should also scrub the version history of this service. Previous versions are stored in entity_extension with the full connection, so they still carry elasticsSearch and securityConfig. Could you apply the same removal there, in both the MySQL and Postgres scripts?
-- MySQL
UPDATE entity_extension
SET json = JSON_REMOVE(json, '$.connection.config.elasticsSearch', '$.connection.config.securityConfig')
WHERE id = (SELECT id FROM metadata_service_entity WHERE name = 'OpenMetadata')
AND extension LIKE 'metadataService.version.%';
-- Postgres
UPDATE entity_extension
SET json = (json #- '{connection,config,elasticsSearch}') #- '{connection,config,securityConfig}'
WHERE id = (SELECT id FROM metadata_service_entity WHERE name = 'OpenMetadata')
AND extension LIKE 'metadataService.version.%';I checked this locally: after an update, the history row keeps both fields once the current migration has run, and the statement above clears them. For the 2.0 backport, this needs to go in a 2.0.7 folder.
6467045 to
0dd6e18
Compare
mohityadav766
changed the title
0dd6e18 to
7c2a161
Compare
Return a service connection in full only to bots, masked to callers who can edit the service (admins and explicitly delegated operators with EDIT_ALL), and not at all to anyone else. The built-in OpenMetadata service no longer stores the Elasticsearch sink or the ingestion-bot JWT (ingestion builds its own securityConfig on the fly and reindexing reads the search config from the application config); it keeps only host/port/auth provider. The server-connection JWT is masked for non-bot readers, and a migration drops those fields from existing metadata service rows and their version history. The decision is recorded in docs/decisions.
7c2a161 to
58bb2aa
Compare
🚦 Removed from the merge queue —
|
Code Review ✅ Approved 4 closed / 4 findings🔴 High risk · Changing service-connection access and deleting stored connection fields could expose secrets to unauthorized users or remove credentials needed by existing workflows. Restricts service connection visibility so bots receive full connections, editors with EDIT_ALL receive masked connections, and other callers receive none, and removes the Elasticsearch sink and ingestion-bot JWT from the built-in service with a migration for existing rows and history. Four findings were resolved, including non-admin editors now getting no connection, the visibility rule being recorded in docs/decisions, the per-service EDIT_ALL entity load, and the ADR's Data Stewards error. No issues remain open. ✅ 4 closed✅ Bug: Non-admin service editors (EditAll/owners) now get no connection
✅ Quality: New service-connection visibility rule is not recorded in docs/decisions
✅ Performance: Per-service EDIT_ALL check adds an entity load to every listed service
✅ Quality: ADR wrongly says Data Stewards hold EDIT_ALL and see masked connections
VerificationRules No rules evaluated Functional validation Not enabledAuto-approval Not enabledFor more verification details, comment OptionsDisplay: compact → Showing compact verification results. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
mohityadav766
added
the
To release
|
|
🤖 Backport to |
|
🤖 Backport to |
…36219) (#36266) * fix(security): restrict service connection reads to bots and editors Return a service connection in full only to bots, masked to callers who can edit the service (admins and explicitly delegated operators with EDIT_ALL), and not at all to anyone else. The built-in OpenMetadata service no longer stores the Elasticsearch sink or the ingestion-bot JWT (ingestion builds its own securityConfig on the fly and reindexing reads the search config from the application config); it keeps only host/port/auth provider. The server-connection JWT is masked for non-bot readers, and a migration drops those fields from existing metadata service rows and their version history. The decision is recorded in docs/decisions. * test: align messaging connection visibility with edit permissions (cherry picked from commit 4934556)
Describe your changes:
Harden how service connections are returned and stored.
service (admins and delegated operators with
EDIT_ALL), and not at all to anyone else.The masked-vs-null choice is gated on edit permission, not
isAdmin(), so delegated operators keepmanaging connection details while view-only users receive no connection.
Elasticsearch sink or the ingestion-bot JWT. Ingestion builds its own connection on the fly and
reindexing reads the search config from the application config.
(
entity_extension), in both MySQL and Postgres.The visibility rule is recorded in
docs/decisions/2026-10-26-service-connection-visibility-follows-edit-permission.md.Type of change:
Tests:
PasswordEntityMaskerTest— the server-connection JWT is masked and restored on write.MetadataServiceResourceIT— editor=masked / viewer=no connection / bot=full; the built-inservice has no Elasticsearch sink or stored JWT.
The remaining comment-style requirement must be satisfied before merging.
Findings
Summary
Service connections are returned in full to bots, masked to editors, and hidden from viewers.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Read service connection] --> B{Bot?} B -->|Yes| C[Return full connection] B -->|No| D{Can edit the service?} D -->|Yes| E[Return masked connection] D -->|No| F[Return null connection]Reviews (6) · Last reviewed commit: "test: align messaging connection visibil..." · Reviewed by Greptile