Skip to content

feat: support channel binding configuration in PgConnectOptions#1671

Open
jorsol wants to merge 1 commit into
eclipse-vertx:masterfrom
jorsol:update-scram-3.3
Open

feat: support channel binding configuration in PgConnectOptions#1671
jorsol wants to merge 1 commit into
eclipse-vertx:masterfrom
jorsol:update-scram-3.3

Conversation

@jorsol

@jorsol jorsol commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Motivation:

Introduce support for configuring channel binding mechanisms during the
SCRAM-SHA-256-PLUS authentication handshake. This allows users to require,
prefer, or disable channel binding securely.

  • Add channelBinding property and getters/setters to PgConnectOptions
  • Update PgConnectionUriParser to parse 'channel_binding' from connection URIs
  • Propagate the configuration to ScramAuthentication to negotiate -PLUS variants
  • Fail the connection handshake if a mismatch or unsupported negotiation occurs

Conformance:

You should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md
Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines

@tsegismont tsegismont self-assigned this Jun 5, 2026
@tsegismont tsegismont added this to the 5.2.0 milestone Jun 5, 2026
@tsegismont

Copy link
Copy Markdown
Member

Can you please sign the Eclipse ECA? Would you mind adding the option field to connect options while you're at it?

@tsegismont

Copy link
Copy Markdown
Member

Ideally we'd also have a test that verifies it is not possible to connect if the user sets the REQUIRE policy for the client and the server does not explicitly advertise a channel-bound mechanism.

@jorsol jorsol marked this pull request as draft June 10, 2026 09:20
@jorsol jorsol force-pushed the update-scram-3.3 branch from 26e9f4d to 343a5ba Compare June 14, 2026 10:44
@jorsol jorsol changed the title fix(deps): bump scram-client to 3.3 feat: support channel binding configuration in PgConnectOptions Jun 14, 2026
@jorsol

jorsol commented Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

@tsegismont Could you please review again? I'm not fully familiar with the code base, so maybe you can point me to improvements.

@jorsol jorsol force-pushed the update-scram-3.3 branch from 343a5ba to 7a47315 Compare June 14, 2026 15:08
@jorsol jorsol marked this pull request as ready for review June 14, 2026 15:26
@jorsol jorsol force-pushed the update-scram-3.3 branch 8 times, most recently from 312e6a4 to 86caa70 Compare June 14, 2026 17:04
Comment on lines +290 to +291
// ctx.assertEquals("Channel bindins is required", err.getMessage());
ctx.assertTrue(err instanceof ClosedConnectionException); // TODO: handle ChannelBindingException

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As in #1672 (this could be due to changes in the way we handle exceptions since 5.1)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll take a look after merging

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.

Thank you, I'm really lost on how to handle the exceptions here.

@tsegismont tsegismont left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you @jorsol ! Please find a few comments

Comment on lines +290 to +291
// ctx.assertEquals("Channel bindins is required", err.getMessage());
ctx.assertTrue(err instanceof ClosedConnectionException); // TODO: handle ChannelBindingException

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll take a look after merging

Comment on lines +86 to +88
socket.channelHandlerContext().channel()
.attr(AttributeKey.valueOf("channel_binding"))
.set(connectOptions.getChannelBinding());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can avoid indirect communication through a channel attribute.

You could add a method in this class like:

  public ChannelBinding channelBinding() {
    return connectOptions.getChannelBinding();
  }

In io.vertx.pgclient.impl.codec.InitPgCommandMessage#handleAuthenticationSasl

    PgSocketConnection pgSocketConn = (PgSocketConnection) cmd.connection().unwrap();
    ScramClientInitialMessage initialSaslMessage = scramSession.createInitialSaslMessage(in, encoder.channelHandlerContext(), pgSocketConn.channelBinding());
    encoder.writeScramClientInitialMessage(initialSaslMessage);

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.

Hi @tsegismont, thanks for pointing to this, those changes are done now.

@jorsol jorsol force-pushed the update-scram-3.3 branch from 86caa70 to 2816bac Compare June 15, 2026 12:49
Introduce support for configuring channel binding mechanisms during the
SCRAM-SHA-256-PLUS authentication handshake. This allows users to enforce,
prefer, or disable channel binding securely.

- Add channelBinding property and getters/setters to PgConnectOptions
- Update PgConnectionUriParser to parse 'channel_binding' from connection URIs
- Store channel binding preference in Netty Channel attributes during initialization
- Propagate the configuration to ScramAuthentication to negotiate -PLUS variants
- Fail the connection handshake if a mismatch or unsupported negotiation occurs

Signed-off-by: Jorge Solorzano <jorsol@gmail.com>
@jorsol jorsol force-pushed the update-scram-3.3 branch from 2816bac to d0540c8 Compare June 15, 2026 13:08
@jorsol jorsol requested a review from tsegismont June 15, 2026 16:02
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.

2 participants