Closed mholt closed 6 years ago
Update: The change is now on a branch at https://github.com/mholt/caddy/pull/2015 -- I will see what I can do to get a PR your way soon.
Hi @mholt , I am happy to help. I will make some time this week to sort this out.
@mholt ,
I have made some changes in this branch: https://github.com/pieterlouw/caddy-net/tree/issue5
The changes were all in plugin.go
Hey, that's great! You were quick. I'll close this issue now, and I'll merge my PR this week.
Hi @mholt , These changes would probably need to be merged into the master in parallel to when the Caddy PR will be merged and release. Around when will this happen? Also, although I made the changes I still need to find time to properly test it. Pieter
@pieterlouw Plan on it happening right after the Go 1.10 release. If I'm not in Spain when it's released, I will release Caddy 0.10.11 with these changes the day of or the day after.
Hi Pieter! Just wanted to let you know that an upcoming change in Caddy will affect this server type plugin. I'm opening this issue to track the progress of this plugin's alignment with the change I'll be pushing to a branch in Caddy soon. (Once Caddy's next version is released with the change, if this repo is not also updated and released after that, builds will fail on the website.)
I'll try to submit a pull request with my suggested changes, for your review, so you won't have to do too much work for it. Maybe in a week or less.
It's not a major change, but a slightly annoying one. It fixes https://github.com/mholt/caddy/issues/1994 and https://github.com/mholt/caddy/issues/1991.
High-level summary:
TLS certificates have been stored in a global certificate cache, from which they are given to clients when making TLS connections. When a certificate is loaded into memory, it is keyed in this cache by the names on it.
When a certificate is loaded with a name that overlaps another certificate, only one certificate (the first one) continues to serve that name, even though two different site definitions may load the certificates separately. In other words, there was a global name->certificate lookup even though it should be scoped per-site.
When a Caddy instance is reloaded for an updated config (think SIGUSR1), there was nothing to erase the old certificate cache, leading to inefficiency if many sites get removed or replaced by different ones.
The change I'm making will scope certificate cache to the
*caddy.Instance
instead of having one global one. Additionally, each*caddytls.Config
will have its own name->certificate map so that they won't step on the toes of maps for other configs.This requires that the
*caddy.Instance
be accessible to plugins, that the Instance have some sort of storage (hint: it's amap[interface{}]interface{}
but oh well), and that thecaddytls.Config
always be created in a way that it has a pointer to the certificate cache in the Instance's storage.What this means for you is that simply creating a caddytls.Config by
&caddytls.Config{...}
is going to be replaced by a constructor that requires an Instance,caddytls.NewConfig(instance)
, and then you'll have to set any fields after that. It also means yourNewContext()
function must now takes an argument:NewContext(*Instance)
(the interface has changed).It seems like a lot, but the changes should be minimal (just a few lines perhaps) and I think for the better, but I still welcome your feedback.
... I know this is inconvenient, sorry about that. As Caddy continues to mature, these changes will become less and less frequent.