Subject: [pgjdbc/pgjdbc] PR #3665: test: add channelBinding to SslTest

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sat, 14 Jun 2025 13:01:28 +0000

Previously, channelBinding was not tested as a part of SslTest

Now `SslTest` generates 5724 cases which take 45 seconds on my local machine.

---

From: "gitguardian[bot] (bot) (@gitguardian[bot])" <noreply+gitguardian[bot]@github.com>
Date: Sat, 14 Jun 2025 13:02:02 +0000

#### ️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find [here](https://docs.gitguardian.com/platform/remediate/remediate-incidents) more information about risks.

---

<sup><sub>🦉 [GitGuardian](https://dashboard.gitguardian.com/auth/login/?utm_medium=checkruns&amp;utm_source=github&amp;utm_campaign=cr1) detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.<br/></sup></sub>

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sat, 14 Jun 2025 15:45:02 +0000

@sehrope , I remove `ChannelBindingRequiredTest` here as `SslTest` verifies many more cases. I hope you don't mind.

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Sun, 15 Jun 2025 12:28:25 +0000

If you're going to refactor the channel binding options being passed into the ScramAuthenticator then do that in its own commit. Ideally in its own PR so that we see it run through the existing tests without changing anything. Definitely don't do it in a single commit labeled "add channelBinding to ssltest" as it's not test related.

Separately, that SSLTest is too complicated as it is and I don't like adding even more to it. It also makes it annoying to test one slice of functionality (e.g., channel_binding related stuff) as it's one giant test that must be run all of nothing.

If you think this is both providing better coverage and is more supportable then go for it. But I'd prefer simpler tests that cover the real world combinations that people actually use as it is easier to understand what is being tested and how the test is expected to operate.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 16 Jun 2025 00:08:43 +0000

> If you're going to refactor the channel binding options being passed into the ScramAuthenticator then do that in its own commit. Ideally in its own PR so that we see it run through the existing tes…

I don't see where it is changing the options.
> Separately, that SSLTest is too complicated as it is and I don't like adding even more to it. It also makes it annoying to test one slice of functionality (e.g., channel_binding related stuff) as i…
I do agree with this.
.

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Mon, 16 Jun 2025 00:10:51 +0000

The original commit in this PR had the options changes but that was done in #3667 so now only the test changes remain.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Mon, 16 Jun 2025 13:05:32 +0000

>run all of nothing.

It takes about 20-50 seconds on my machine. I find it acceptable.

What do you expect by "test one slice of functionality"?

If you mean "test all cases when `channelBinding=require`", then you could do that by adding `if (channelBinding != ChannelBinding.REQUIRE) { continue; }` into `data()` method.

An alternative option is to execute a single case.

> SSLTest is too complicated

I'm all ears on suggestions to have a comparable test coverage with "less complicated" code.
https://jqwik.net/ might help with automatic shrinking, however, we don't use jqwik for testing yet.

I do not find the code hard to understand, as all it does it it setups the connection properties, then it connects. If the connection succeeds it verifies there were not expected errors, and if the connection fails, it verifies if the failure was expected or not.

The options do affect one another, so we should better test the way they interact rather than be sorry of not testing a thing.

For instance, `ChannelBindingRequiredTest` misses a case with `client certificate` authentication. It misses a case with `sslMode=allow` (==upgrade the connection to TLS as required by `channelBinding`), and it probably misses the other important cases.

Does it make sense testing "channelBinding=require + use direct ALPN connection"? Why not. `SslTest` covers it.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 17 Jun 2025 12:33:42 +0000

I think this is mergeable: the tests pass, the missing assertions added, the bug with clientcert in PG11 fixed.

I've created https://github.com/pgjdbc/pgjdbc/pull/3681 to configure TLS in Appveyor