pgjdbc/pgjdbc GitHub issues and pull requests (mirror)help / color / mirror / Atom feed
[pgjdbc/pgjdbc] PR #3665: test: add channelBinding to SslTest 8+ messages / 4 participants [nested] [flat]
* [pgjdbc/pgjdbc] PR #3665: test: add channelBinding to SslTest @ 2025-06-14 13:01 "vlsi (@vlsi)" <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: vlsi (@vlsi) @ 2025-06-14 13:01 UTC (permalink / raw) To: pgjdbc/pgjdbc <[email protected]> Previously, channelBinding was not tested as a part of SslTest Now `SslTest` generates 5724 cases which take 45 seconds on my local machine. ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3665: test: add channelBinding to SslTest @ 2025-06-14 13:02 ` "gitguardian[bot] (bot) (@gitguardian[bot])" <noreply+gitguardian[bot]@github.com> 6 siblings, 0 replies; 8+ messages in thread From: gitguardian[bot] (bot) (@gitguardian[bot]) @ 2025-06-14 13:02 UTC (permalink / raw) To: pgjdbc/pgjdbc <[email protected]> #### ️✅ 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&utm_source=github&...) 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> ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3665: test: add channelBinding to SslTest @ 2025-06-14 15:45 ` "vlsi (@vlsi)" <[email protected]> 6 siblings, 0 replies; 8+ messages in thread From: vlsi (@vlsi) @ 2025-06-14 15:45 UTC (permalink / raw) To: pgjdbc/pgjdbc <[email protected]> @sehrope , I remove `ChannelBindingRequiredTest` here as `SslTest` verifies many more cases. I hope you don't mind. ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3665: test: add channelBinding to SslTest @ 2025-06-15 12:28 ` "sehrope (@sehrope)" <[email protected]> 6 siblings, 0 replies; 8+ messages in thread From: sehrope (@sehrope) @ 2025-06-15 12:28 UTC (permalink / raw) To: pgjdbc/pgjdbc <[email protected]> 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. ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3665: test: add channelBinding to SslTest @ 2025-06-16 00:08 ` "davecramer (@davecramer)" <[email protected]> 6 siblings, 0 replies; 8+ messages in thread From: davecramer (@davecramer) @ 2025-06-16 00:08 UTC (permalink / raw) To: pgjdbc/pgjdbc <[email protected]> > 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. 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 it's one giant test that must be run all of nothing. I do agree with this. . ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3665: test: add channelBinding to SslTest @ 2025-06-16 00:10 ` "sehrope (@sehrope)" <[email protected]> 6 siblings, 0 replies; 8+ messages in thread From: sehrope (@sehrope) @ 2025-06-16 00:10 UTC (permalink / raw) To: pgjdbc/pgjdbc <[email protected]> The original commit in this PR had the options changes but that was done in #3667 so now only the test changes remain. ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3665: test: add channelBinding to SslTest @ 2025-06-16 13:05 ` "vlsi (@vlsi)" <[email protected]> 6 siblings, 0 replies; 8+ messages in thread From: vlsi (@vlsi) @ 2025-06-16 13:05 UTC (permalink / raw) To: pgjdbc/pgjdbc <[email protected]> >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. ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3665: test: add channelBinding to SslTest @ 2025-06-17 12:33 ` "vlsi (@vlsi)" <[email protected]> 6 siblings, 0 replies; 8+ messages in thread From: vlsi (@vlsi) @ 2025-06-17 12:33 UTC (permalink / raw) To: pgjdbc/pgjdbc <[email protected]> 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 ^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2025-06-17 12:33 UTC | newest] Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2025-06-14 13:01 [pgjdbc/pgjdbc] PR #3665: test: add channelBinding to SslTest "vlsi (@vlsi)" <[email protected]> 2025-06-14 13:02 ` "gitguardian[bot] (bot) (@gitguardian[bot])" <noreply+gitguardian[bot]@github.com> 2025-06-14 15:45 ` "vlsi (@vlsi)" <[email protected]> 2025-06-15 12:28 ` "sehrope (@sehrope)" <[email protected]> 2025-06-16 00:08 ` "davecramer (@davecramer)" <[email protected]> 2025-06-16 00:10 ` "sehrope (@sehrope)" <[email protected]> 2025-06-16 13:05 ` "vlsi (@vlsi)" <[email protected]> 2025-06-17 12:33 ` "vlsi (@vlsi)" <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox