Skip to content

adapter: authorize retained ALTER CONNECTION dependencies - #38486

Merged
aljoscha merged 1 commit into
MaterializeInc:mainfrom
aljoscha:sql-654-connection-secret-authorization
Aug 28, 2026
Merged

adapter: authorize retained ALTER CONNECTION dependencies#38486
aljoscha merged 1 commit into
MaterializeInc:mainfrom
aljoscha:sql-654-connection-secret-authorization

Conversation

@aljoscha

@aljoscha aljoscha commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Motivation

ALTER CONNECTION initially authorizes only dependencies named in the submitted statement. Replanning the complete stored definition with a system session can retain secret dependencies that the caller is not allowed to use, then read those secrets during content checks or connection validation.

Description

Connection owners can no longer change connection options while retaining secrets or connections they lack USAGE privileges on. Authorize the fully reconstructed connection's final dependency set against the invoking session before any secret reads or validation. The check applies even when validation is disabled, and it uses the final set so callers can remove dependencies they are no longer authorized to use.

The SQL privilege documentation describes the additional requirement. The adapter guide records the authorization boundary for DDL that replans stored definitions with a system session.

Verification

Added sqllogictest coverage that transfers a secret-bearing connection to a role without secret access, verifies alterations are denied until USAGE is granted, and verifies the role can remove an inaccessible secret dependency.

Added testdrive coverage that denies validated option changes before external validation while preserving delegated VALIDATE CONNECTION access to an unchanged connection.

Closes: SQL-654

@aljoscha
aljoscha marked this pull request as ready for review August 26, 2026 11:36
@aljoscha
aljoscha requested review from a team as code owners August 26, 2026 11:36
@aljoscha
aljoscha requested review from SangJunBak and ggevay August 26, 2026 16:05
@bosconi bosconi added the release-blocker Critical issue that should block *any* release if not fixed label Aug 26, 2026

@ggevay ggevay left a comment

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.

LGTM, minor comments, plus one comment about a potential followup.

Comment thread doc/developer/guide-adapter.md Outdated
guaranteed system-wide. Always ask: "does this still work if another node is
running the same code against the same backing store?"

### System-session replanning does not grant authority

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.

(Just double-checking if it was intentional to add this here, because we sometimes have over-eager AI additions in this file.)

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 AI did at it but I left it in because it maybe felt useful, but could also remove it again, we should always be fine to cut things here, btw, even after we merge

Comment thread doc/developer/guide-adapter.md Outdated
during replanning. Before reading secrets, performing external I/O, or
persisting the result, authorize the final dependency set against the invoking
session. Check the final set rather than the union of old and new dependencies,
so a caller can remove a dependency they are no longer authorized to use.

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.

Not for this PR, but worth a follow-up: the final-set rule does not cover tunnel-type connections. The owner of an SSH TUNNEL / AWS PRIVATELINK connection needs only ownership to change its HOST / SERVICE NAME, its own dependency set is empty, and handle_alter_connection re-renders every dependent through the new tunnel. The dependents' secrets then go to whatever the attacker's bastion connects them to, on any transport short of full hostname verification against a CA the attacker cannot influence: PG require/verify-ca and MySQL required/verify_ca never verify the hostname and fall back to system roots without a custom CA, Kafka SASL_PLAINTEXT has no TLS at all, and PrivateLink-tunneled Kafka defaults endpoint identification to none even on SASL_SSL. Same ownership premise as H01, and the tunnel creator can also be the attacker with no transfer involved. Maybe add a sentence here that a definition's peer can also change through a referenced tunnel connection.

Comment thread doc/developer/guide-adapter.md Outdated
guaranteed system-wide. Always ask: "does this still work if another node is
running the same code against the same backing store?"

### System-session replanning does not grant authority

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.

nit, if the section stays: it lands between "No local-only assumptions" and the timestamp checklist, which belongs to the timestamp material above. Consider moving it below the checklist so that block stays contiguous.

ALTER CONNECTION initially authorizes only dependencies named in the
submitted statement. Replanning the complete stored definition with a
system session can retain secrets that the caller is not allowed to use.

Authorize the final dependency set against the invoking session before
reading secrets or validating the connection. This also permits callers
to remove dependencies they can no longer use.

Closes: SQL-654
@aljoscha
aljoscha force-pushed the sql-654-connection-secret-authorization branch from 38c474b to 392dfe0 Compare August 28, 2026 11:50
@aljoscha
aljoscha merged commit 5998b50 into MaterializeInc:main Aug 28, 2026
86 checks passed
@aljoscha
aljoscha deleted the sql-654-connection-secret-authorization branch August 28, 2026 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-blocker Critical issue that should block *any* release if not fixed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants