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

fix http3 outdated certificates after config reload #5517

Merged
merged 4 commits into from
May 10, 2023

Conversation

WeidiDeng
Copy link
Member

Caddy http3 will serve outdated certificates is config is reloaded because caddy tries to reuse existing quic listeners. However, because after config reload, old tls.Config won't update its certificates anymore. We could disable quic listener reuse, but by changing GetConfigForClient field, quic listener will use latest certificates anyway.

Fix https://github.com/caddyserver/caddy/issues/4849.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm intrigued -- thanks for the patch!! Just one question; we need to make sure this is thread-safe.

listeners.go Outdated Show resolved Hide resolved
@WeidiDeng WeidiDeng requested review from mholt and francislavoie May 6, 2023 02:42
Copy link
Member

@francislavoie francislavoie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Matt is better equipped to review the logic, but this is just some style nits

listeners.go Outdated Show resolved Hide resolved
listeners.go Outdated Show resolved Hide resolved
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking pretty good -- does it work alright @WeidiDeng ?

Thanks for working on this!

listeners.go Outdated Show resolved Hide resolved
listeners.go Outdated Show resolved Hide resolved
@WeidiDeng
Copy link
Member Author

This is looking pretty good -- does it work alright @WeidiDeng ?

Thanks for working on this!

It works fine when new config is ok. I haven't been able to test how it will react when new config load failed (i.e. when http3 is starting ok but other parts fail), but theoretically it will use the older tls.Config in this case.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. I'm willing to give this a try in the 2.7 beta 👍

@mholt mholt merged commit 808b05c into master May 10, 2023
@mholt mholt deleted the fix-http3-after-reload branch May 10, 2023 20:25
@francislavoie francislavoie added the bug 🐞 Something isn't working label May 10, 2023
@francislavoie francislavoie added this to the v2.7.0 milestone May 10, 2023
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 this pull request may close these issues.

3 participants