tls: add system and combined CA pool modules by HarshPatel5940 · Pull Request #7406 · caddyserver/caddy

HarshPatel5940 · 2025-12-25T03:53:06Z

Summary

I've implemented the system trust pool module which uses x509.SystemCertPool() and works as expected.

However, the combined module has a fundamental limitation: Go's x509.CertPool doesn't expose its certificates, making it impossible to merge multiple pools.

so should we modify the CA interface to add a method like Certificates() []*x509.Certificate so sources can expose their certificates for merging? not sure how to proceed here... Would be happy to learn.

Assistance Disclosure

I wrote the code, but Claude generated the tests.

HarshPatel5940 · 2025-12-25T03:54:52Z

+	// LIMITATION: x509.CertPool doesn't expose its certificates, making it impossible
+	// to merge multiple pools. Return an error if multiple sources are configured.
+	if len(ccp.sources) > 1 {
+		return fmt.Errorf("combined CA pool currently supports only a single source due to x509.CertPool API limitations (got %d sources); to use multiple certificate sources, consider using a single 'file' source with multiple pem_file entries, or contribute a fix to expose certificate data from the CA interface", len(ccp.sources))
+	}
+
+	// At this point we have exactly one source
+	ccp.pool = ccp.sources[0].CertPool()
+
+	return nil
+}
+
+// Syntax:
+//
+//	trust_pool combined {
+//		source <module_name> {
+//			<module_config>
+//		}
+//	}
+//
+// LIMITATION: Currently only a single source is supported due to x509.CertPool
+// API limitations. Specifying multiple sources will result in a provisioning error.
+// To combine multiple certificate files, use a single 'file' source with multiple
+// pem_file entries instead.


I have intentionally added limitation comments cause i wasnt aware how to proceed here.

HarshPatel5940 · 2025-12-25T03:58:41Z

"combined merges multiple roots sources" is not satisfied as per issue #7391 and i am erroring out as you see in above comment.

so should we modify the CA interface to add a method like Certificates() []*x509.Certificate so sources can expose their certificates for merging? not sure how to proceed here... Would be happy to learn.

This approach which i think is a breaking change so i wanted to consult with my planned implementation

FreyreCorona · 2026-01-02T23:57:35Z

Happy new year and i reviewed your code and i have two types of solutions
Ther first is like you say , changin the actual contract of CA interface ,

type CA interface {
    AppendTo(pool *x509.CertPool) error
}

This is breaking change because not all CA pools can expose certificates.

The second way is creating a secondary interface (non breaking change)

type CertificateProvider interface {
    Certificates() []*x509.Certificate
}

The combined module would then check whether each source implements this interface and merge certificates only from those that do.

HarshPatel5940 · 2026-01-20T04:16:20Z

Hello @FreyreCorona , thank you for the suggestions! will work on a non-breaking solution.

lastly, sorry for the delayed response. forgot about this pr due to work

FreyreCorona · 2026-01-20T13:03:50Z

Nice work on the implementation, let's wait for the review. No worries, we're all busy, and I've been pretty quiet around here these past few days too.

mohammed90 · 2026-01-27T01:48:34Z

When I first glanced at this, I wondered by I made it return a concrete type instead of an interface, but then remembered that tls.Config takes a struct and there's no way around it.

I don't see the point of the AppendTo(pool *x509.CertPool) error because it still doesn't resolve the issue with system trust, which is it not exporting its certificates. I prefer the other approach of the optional, secondary interface because it's non-breaking and indicates optionality for types that can't implement it.

Open to discuss this further though

HarshPatel5940 · 2026-01-27T04:52:04Z

I don't see the point of the AppendTo(pool *x509.CertPool) error because it still doesn't resolve the issue with system trust, which is it not exporting its certificates. I prefer the other approach of the optional, secondary interface because it's non-breaking and indicates optionality for types that can't implement it.

Thanks for the review!

I'm confirming that this PR effectively implements the non-breaking, optional interface approach.

To clarify:

CertificateProvider as a distinct, optional interface, leaving the main CA contract unchanged (non-breaking).
All source modules that can expose certificates (like file, inline, pki) implement this new interface.
SystemCAPool intentionally does not implement it because x509.SystemCertPool() doesn't expose underlying certs.
The CombinedCAPool module will merge certificates from compatible sources and return an error if you try to combine a source (like system) that doesn't support it.

mohammed90

On a high level. It looks good. Please add adapt tests like the ones here:

https://github.com/search?q=repo%3Acaddyserver%2Fcaddy+trust_pool+language%3AText&type=code&l=Text

HarshPatel5940 · 2026-02-03T11:06:11Z

Hmm, noted. 🤔

* doing it for first time, so not sure if its right.

mohammed90

Life was busy in the past few months, and this area of the code proved itself critical, so I had to ensure I have clear mind to go through it before merging.

Hopefully this is the last round of review. I believe I've caught everything.

HarshPatel5940 · 2026-03-27T12:04:04Z

Life was busy in the past few months, and this area of the code proved itself critical, so I had to ensure I have clear mind to go through it before merging.

Definitely understandable.

HarshPatel5940 · 2026-03-27T13:10:23Z

done the requested changes fad1950

mholt · 2026-03-31T03:08:19Z

Folks, please refrain from the pinging. It is on our list. We are very busy, especially the volunteers who have taken up this PR/issue. The more pinging that happens the more we dismiss the notification on the issue and it goes back to the end of the queue (that's just how GH notifications work, I dunno what other peoples' flows are, but that's mine).

mohammed90

Thank you for working on this and persevering through the multiple rounds of review! LGTM

HarshPatel5940 · 2026-04-06T02:07:49Z

Thank you so much for guiding me along the process!

feat: add system and combined CA pool modules

90b7656

HarshPatel5940 commented Dec 25, 2025

View reviewed changes

HarshPatel5940 marked this pull request as draft December 25, 2025 03:59

HarshPatel5940 mentioned this pull request Dec 25, 2025

cert_pool: add combined/multi and system #7391

Closed

HarshPatel5940 added 3 commits January 20, 2026 09:49

Merge branch 'caddyserver:master' into hp/system-ca-pool

5cca36e

fix: combining pools using CertificateProvider

4f386a7

fix: lint issue

91ca71a

HarshPatel5940 marked this pull request as ready for review January 20, 2026 04:50

This comment was marked as spam.

Sign in to view

mohammed90 requested changes Feb 3, 2026

View reviewed changes

chore: caddyfiletests

a4efb86

* doing it for first time, so not sure if its right.

HarshPatel5940 requested a review from mohammed90 February 6, 2026 12:47

This comment was marked as spam.

Sign in to view

mohammed90 requested changes Mar 26, 2026

View reviewed changes

Comment thread modules/caddytls/capools.go Outdated

Comment thread modules/caddytls/capools.go Outdated

Comment thread modules/caddytls/capools.go Outdated

HarshPatel5940 added 3 commits March 27, 2026 17:57

Merge branch 'caddyserver:master' into hp/system-ca-pool

2c11e31

fix: use x509 native addCert

fad1950

chore: explicit err handling

bb839ac

HarshPatel5940 requested a review from mohammed90 March 27, 2026 13:09

This comment was marked as spam.

Sign in to view

mohammed90 reviewed Apr 5, 2026

View reviewed changes

Comment thread modules/caddytls/capools.go

Apply suggestion from @mohammed90

2fcf171

mohammed90 approved these changes Apr 5, 2026

View reviewed changes

mohammed90 changed the title ~~feat: add system and combined CA pool modules~~ tls: add system and combined CA pool modules Apr 5, 2026

mohammed90 merged commit d783467 into caddyserver:master Apr 5, 2026
27 checks passed

mohammed90 linked an issue Apr 6, 2026 that may be closed by this pull request

cert_pool: add combined/multi and system #7391

Closed

Uh oh!

Conversation

HarshPatel5940 commented Dec 25, 2025

Summary

Assistance Disclosure

Uh oh!

HarshPatel5940 Dec 25, 2025

Choose a reason for hiding this comment

Uh oh!

HarshPatel5940 commented Dec 25, 2025

Uh oh!

FreyreCorona commented Jan 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HarshPatel5940 commented Jan 20, 2026

Uh oh!

FreyreCorona commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mohammed90 commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HarshPatel5940 commented Jan 27, 2026

Uh oh!

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

mohammed90 left a comment • 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!

HarshPatel5940 commented Feb 3, 2026

Uh oh!

This comment was marked as spam.

This comment was marked as spam.

mohammed90 left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

HarshPatel5940 commented Mar 27, 2026

Uh oh!

HarshPatel5940 commented Mar 27, 2026

Uh oh!

This comment was marked as spam.

mholt commented Mar 31, 2026

Uh oh!

Uh oh!

mohammed90 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HarshPatel5940 commented Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

FreyreCorona commented Jan 2, 2026 •

edited

Loading

FreyreCorona commented Jan 20, 2026 •

edited

Loading

mohammed90 commented Jan 27, 2026 •

edited

Loading

mohammed90 left a comment •

edited

Loading

mohammed90 left a comment •

edited

Loading