Fix advanced cache initialization in README

[s111]

Hi!

When initializing a Cache with NewCache it's easy to miss that a cache must be attached to the configuration in CacheOptions.GetConfigForCert and is not set by the cache itself.
If a Config with certCache == nil is used, it will cause panics at runtime (not immediately, so it's not so easy to detect the cause of the panics).

I think the proposed change to the README will address this issue and hopefully prevent other users from encountering the same problems I did, as they where not trivial to debug.

I think a good follow up discussion and possibly PR would be to look into either:

• Returning an error if certCache == nil, so that the error can be more easily traced to its origin.
• Filling in the certCache field if it's nil, so that the error is not possible to begin with.

I think this can be solved in Cache.getConfig.

It's not clear to me why the cache must be set in the Config returned by GetConfigForCert as it doesn't allow a different cache to be used in the first place (at least I think so). Since it can check whether the correct cache is used, can it simply not use the correct cache? I've not looked to deep into this, I thought it better to ask first.

[mholt]

Thanks for the PR! I will look at this as soon as I get a chance.

[mholt]

Sorry for the delay. (I had to get Caddy 2.6 released.) You still with me? :)

This is a good change, and I like your thoughts on things. This API is one of the most unfortunate parts of CertMagic, and really a thorn in my side, I'd be very happy to improve if we figure out how.

So, first: yes -- we can probably do something if certCache is nil. That's probably better since we know it'll be needed later.

I'm just now realizing that the godocs and this readme contradict each other.

Godoc:

	// REQUIRED. A function that returns a configuration
	// used for managing a certificate, or for accessing
	// that certificate's asset storage (e.g. for
	// OCSP staples, etc). The returned Config MUST
	// be associated with the same Cache as the caller.
	//
 	// The reason this is a callback function, dynamically
	// returning a Config (instead of attaching a static
	// pointer to a Config on each certificate) is because
	// the config for how to manage a domain's certificate
	// might change from maintenance to maintenance. The
	// cache is so long-lived, we cannot assume that the
	// host's situation will always be the same; e.g. the
	// certificate might switch DNS providers, so the DNS
	// challenge (if used) would need to be adjusted from
	// the last time it was run ~8 weeks ago.

but this readme code says:

// do whatever you need to do to get the right
// configuration for this certificate; keep in
// mind that this config value is used as a
// template, and will be completed with any
// defaults that are set in the Default config

... which is actually how I'd like for it to work. It'd be nice if the returned config didn't need to be fully initialized (certmagic.New()) and filled out -- just use it as a template...

I can't remember now why I changed that. 🤔 I imagine it was a bug, but maybe this was the wrong or non-optimal fix.

So, for now, this docs change LGTM. But I think we need to address this like you said.

Any chance you'd be willing to propose a more permanent fix/improvement after we merge this? I'd definitely like to make this all easier to use.

[s111]

No worries about the delay. I would be happy to propose a more permanent improvement.

To me it looks like the Cache is supposed to live until the program terminates. That is, it does not make sense to (at runtime) change the cache, nor will it work in the current implementation (see 2. below).

certmagic/cache.go

Lines 325 to 335 in 2e8dd44

	func (certCache *Cache) getConfig(cert Certificate) (*Config, error) {
	cfg, err := certCache.options.GetConfigForCert(cert)
	if err != nil {
	return nil, err
	}
	if cfg.certCache != nil && cfg.certCache != certCache {
	return nil, fmt.Errorf("config returned for certificate %v is not nil and points to different cache; got %p, expected %p (this one)",
	cert.Names, cfg.certCache, certCache)
	}
	return cfg, nil
	}

As you can see the current implementation allows two unfortunate outcomes if not configured correctly:

1. If cfg.certCache is nil no error is produced and execution continues until a nil pointer dereference happens later.
2. If one manages to return a cfg which has a different cache (cfg.certCache != certCache) the certificate won't be renewed, though a helpful error message will be logged.

I think a good improvement is to treat the Config returned from GetConfigForCert as a template only, filling in certCache in the method referenced above. If the provided Config's certCache is not nil, we could log a warning indicating that the cache field should be left untouched in the template. This shouldn't be a breaking change and if someone else is having the same problem I did, the problem would disappear and they would get a warning telling them to correct their GetConfigForCert function.

If you think that sounds reasonable I could create a new PR fixing that and updating the documentation. Of course, that would make the changes I'm making in this PR redundant, so maybe I should just update this one?

[s111]

I looked at bit more at the changes I suggested above today and have decided against it. I think it would require changing a bit more than I'm comfortable with in a project I don't know too well.

Instead, I've updated the PR as follows:

• A bit more comments in the documentation. It now follows the original text which says that the Config will be used as a template (which is true since we now use New).
• I've made it an error to return a Config with a nil Cache from GetConfigForCert. This is a lot easier to debug than a nil pointer dereference.

What do you think?

[mholt]

Hey, sorry for not replying sooner. Been a bit slow to catch up on things after the 2.6 release of Caddy.

I think the changes look good, and sounds logical. I'll do a final review as soon as I can! :) Thank you!

[mholt]

Better late than never I suppose...