Support standard certificate chains by IdelsTak · Pull Request #1561 · qzind/tray

IdelsTak · 2026-10-29T13:14:45Z

@tresf, this pr is meant to close #803

accept standard concatenated PEM chains
support multiple intermediates
keep the legacy separator and configured trust behavior

IdelsTak · 2026-10-29T13:14:54Z

Compatibility and security notes:

first certificate stays the signing identity
later certificates are path candidates only
configured roots stay the only trust anchors
legacy bundles and saved allow/block rows still load

Tests' coverage includes standard and legacy chains, two intermediates, persistence, malformed input, expiry, CRL, and embedded-root isolation

tresf · 2026-10-29T18:33:33Z

@IdelsTak thanks. The improvements look great. As for the implementation, I have a concern about single-chain certs / OutOfBoundsException. I've left a code comment about that.

The unit tests are very comprehensive, thank you. I will give you time to reply to my code comments before making any changes myself.

Some cosmetic changes...

legacy-chain -> chain-legacy, standard-chain -> chain-standard, etc
legacy-leaf -> leaf-legacy, direct-leaf -> leaf-direct
- BTW - Can you either rename these or add some code comments to what these are meant to represent?
root-ca -> ca-root
- BTW - Can you either rename this or add a code comments? For example if it's expected for a leafs to be part of a chain, the resource names should probably hint towards that. Maybe they do but it's a bit hard at a glance. One example may be ca-acme and then leaf-acme or something. Anything that makes it easier to read from a distance. Thoughts welcome here.
.pem -> .txt or .crt please. Our internal documentation uses digital-certificate.txt, override.crt and private-key.pem so we should match in our tests for consistency.

IdelsTak · 2026-10-30T06:25:45Z

@tresf, review updates are in 394beab

I also renamed the fixtures by role and kept the root-list concurrency change out of this PR

Could you take another look when convenient?

tresf · 2026-10-30T15:19:56Z

+                "ca-root.crt", true);
+    }
+
+    private void assertNames(String resource, String... expected) throws Exception {


Suggested change

private void assertNames(String resource, String... expected) throws Exception {

private void assertNames(String resource, String... expectedCns) throws Exception {

tresf · 2026-10-30T15:21:07Z

+        List<X509Certificate> certificates = Certificate.parseCertificates(read(resource));
+
+        Assert.assertEquals(certificates.size(), expected.length);
+        for(int i = 0; i < expected.length; i++) {


This will still pass under the following 'failure' scenarios:

when only one expected value is sent

when the certificates contain more values than the expected values

tresf · 2026-10-30T15:22:18Z

+    }
+
+    private void assertTrust(String certificateData, String rootResource,
+                             boolean trusted) throws Exception {


Suggested change

boolean trusted) throws Exception {

boolean trustExpected) throws Exception {

tresf · 2026-10-30T15:26:47Z

+
+public class CertificateSerializationTests {
+    @Test
+    public void testStandard() throws Exception {


This is called testStandard() however the standard and most common workflow is for an existing trusted certificate (such as our demo cert) to be tested against the rootCA that's built into Certificate.java. Do we have coverage for this? If so, do you think we should have that as the standard test?

tresf · 2026-10-30T15:32:41Z

+
+    @Test
+    public void testSelfSigned() throws Exception {
+        assertTrust(read("leaf-self-signed.crt"), "ca-root.crt", false);


Can you explain why this is a negative test? Our most common use-case for self-signed is more like this:

Suggested change

assertTrust(read("leaf-self-signed.crt"), "ca-root.crt", false);

assertTrust(read("leaf-self-signed.crt"), "leaf-self-signed.crt", true);

tresf · 2026-10-30T15:48:47Z

+    }
+
+    @Test
+    public void testUnrelated() throws Exception {


Hmm.... these may make more sense to just make an overload that uses the default caCert with explicit clear() and setTrustBuiltIn(). We should explicitly add setTrustBuiltIn() coverage anyway per https://github.com/qzind/tray/pull/1561/changes#r4425314285.

tresf · 2026-10-30T15:49:37Z

Thanks for the improvements. Code is perfect but unit tests still need some work. Left more feedback.

tresf · 2026-10-30T15:51:36Z

+    }
+
+    @Test
+    public void testDuplicate() throws Exception {


Can you please explain what this unit-test does?

tresf · 2026-10-30T15:53:25Z

+    }
+
+    @Test
+    public void testReversed() throws Exception {


Hmm... what is this testing exactly? Seems cursed and the fact that it's expected to pass is even stranger. I understand detecting and failing this scenario is a bit more nuanced, but let's at least leave a code comment that this is cursed behavior so that if we ever guard against this in the future it's clear from the unit tests comments that it's not expected to pass, just sort of does accidentally.

IdelsTak added 5 commits October 29, 2026 13:55

Support standard certificate chains

48f71e9

Test certificate persistence round trips

27f9eee

Test certificate trust boundaries

9d421b0

Test certificate chain edge cases

e3bbeac

Test certificate validity controls

15419d5

tresf requested changes Oct 29, 2026

View reviewed changes

Address certificate chain review

394beab

tresf mentioned this pull request Oct 30, 2026

Certificate.java: Calls to .add() | remove() | clear() need to be made safe for concurrency #1562

Open

tresf requested changes Oct 30, 2026

View reviewed changes

tresf reviewed Oct 30, 2026

View reviewed changes

	private void assertNames(String resource, String... expected) throws Exception {
	private void assertNames(String resource, String... expectedCns) throws Exception {

	boolean trusted) throws Exception {
	boolean trustExpected) throws Exception {

	assertTrust(read("leaf-self-signed.crt"), "ca-root.crt", false);
	assertTrust(read("leaf-self-signed.crt"), "leaf-self-signed.crt", true);

Conversation

IdelsTak commented Oct 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IdelsTak commented Oct 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tresf commented Oct 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IdelsTak commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tresf Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

tresf Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tresf Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

tresf Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

tresf Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

tresf Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tresf commented Oct 30, 2026

Uh oh!

tresf Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

tresf Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

IdelsTak commented Oct 29, 2026 •

edited

Loading

IdelsTak commented Oct 29, 2026 •

edited

Loading

tresf commented Oct 29, 2026 •

edited

Loading

IdelsTak commented Oct 30, 2026 •

edited

Loading

tresf Oct 30, 2026 •

edited

Loading

tresf Oct 30, 2026 •

edited

Loading

tresf Oct 30, 2026 •

edited

Loading