Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"ocsp_stapling off" seems to have no effect #4588

Closed
tkaehn opened this issue Feb 19, 2022 · 5 comments · Fixed by #4589
Closed

"ocsp_stapling off" seems to have no effect #4588

tkaehn opened this issue Feb 19, 2022 · 5 comments · Fixed by #4589
Labels
bug 🐞 Something isn't working

Comments

@tkaehn
Copy link

tkaehn commented Feb 19, 2022

"ocsp_stapling off" seems to have no effect. Please see the following output:

# ./caddy version
v2.4.6 h1:HGkGICFGvyrodcqOOclHKfvJC0qTU7vny/7FhYp9hNw=
# cat Caddyfile 
{
	auto_https off
	ocsp_stapling off
}

:444

tls /etc/dehydrated/certs/.../fullchain.pem /etc/dehydrated/certs/.../privkey.pem
respond "Hello World"
# ./caddy run
2022/02/19 17:40:07.464	INFO	using adjacent Caddyfile
2022/02/19 17:40:07.469	INFO	admin	admin endpoint started	{"address": "tcp/localhost:2019", "enforce_origin": false, "origins": ["localhost:2019", "[::1]:2019", "127.0.0.1:2019"]}
2022/02/19 17:40:07.470	INFO	tls.cache.maintenance	started background certificate maintenance	{"cache": "0xc00053be30"}
2022/02/19 17:40:37.474	WARN	tls	stapling OCSP	{"error": "no OCSP stapling for [...]: making OCSP request: Post \"http://r3.o.lencr.org\": dial tcp 23.32.238.80:80: i/o timeout"}
2022/02/19 17:40:37.475	INFO	autosaved config (load with --resume flag)	{"file": "/root/.config/caddy/autosave.json"}
2022/02/19 17:40:37.475	INFO	serving initial configuration
2022/02/19 17:40:37.475	INFO	tls	cleaning storage unit	{"description": "FileStorage:/root/.local/share/caddy"}
2022/02/19 17:40:37.475	INFO	tls	finished cleaning storage units
@francislavoie
Copy link
Member

Hmm, that's strange.

All four places in certmagic which write the log message stapling OCSP all call the stapleOCSP() function, which has a check first thing to check if it's disabled.

All the places that create certmagic configs in Caddy do seem to wire up the option correctly.

And the Caddyfile adapter does seem to properly set the option in JSON.

So I'm not sure where the problem is 🤔

@tkaehn
Copy link
Author

tkaehn commented Feb 19, 2022

Using caddy adapt I can confirm that disable_ocsp_stapling is set to true.
However loading the config from JSON doesn't change the result.
The binary was downloaded from GitHub.
Is there anything else I could try?

@francislavoie
Copy link
Member

Ah okay, I see the issue. The global option only applies to managed certificates (which Caddy maintains and renews), but not unmanaged certificates (i.e. from files with the tls directive).

Right now there's no wiring to configure this in the Caddyfile, but you can set the disable_ocsp_stapling boolean on the tls app in the JSON config for now https://caddyserver.com/docs/json/apps/tls/disable_ocsp_stapling/

I'll add the option in the Caddyfile adapter now, for the next release.

@tkaehn
Copy link
Author

tkaehn commented Feb 19, 2022

The option is present twice in the source code. "caddy adapt" sets the first one to true but not the second one.

If setting the second in the JSON file it works as expected. However I see no possibility to configure the same behaviour using the Caddyfile.

$ ag disable_ocsp_stapling
modules/caddytls/automation.go
132:	DisableOCSPStapling bool `json:"disable_ocsp_stapling,omitempty"`

modules/caddytls/tls.go
67:	DisableOCSPStapling bool `json:"disable_ocsp_stapling,omitempty"`

Removing the option from tls.go and setting it to t.Automation.[...]DisableOCSPStapling instead of the following could probably solve the issue:

               OCSP: certmagic.OCSPConfig{
                        DisableStapling: t.DisableOCSPStapling,
                },

@tkaehn
Copy link
Author

tkaehn commented Feb 19, 2022

I just figured it out as well. Thanks for the response and fixing the issue in the next release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants