Skip to content

feature: letsencrypt http.Handler - #3535

Merged
szuecs merged 9 commits into
masterfrom
feature/letsencrypt
Sep 14, 2026
Merged

feature: letsencrypt http.Handler#3535
szuecs merged 9 commits into
masterfrom
feature/letsencrypt

Conversation

@szuecs

@szuecs szuecs commented Jun 22, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

feature: letsencrypt http.Handler integration via autocert with different storage/cache systems to be used by different deployments

Test:
I am running this now since a while on my personal websites and completely replaced certbot+apache+cron to automate certs via skipper+curl+cron.

ref: closes #1786

@szuecs szuecs added enhancement feature definition minor no risk changes, for example new filters do-not-merge labels Jun 22, 2025
@szuecs
szuecs force-pushed the feature/letsencrypt branch from efbe6d7 to e6b4ba5 Compare August 26, 2025 19:09
@szuecs
szuecs force-pushed the feature/letsencrypt branch from e6b4ba5 to 9aa7e75 Compare November 13, 2025 22:05
@zalando-robot

zalando-robot commented Nov 13, 2025

Copy link
Copy Markdown

Deployment Checklist

This change falls under the deployment policy.

💁 Since Nov 10th, we are in the RED deployment zone. This means all changes released to production must adhere to the following requirements:

  • Detailed release notes are provided in this PR’s description.
  • Thorough load-testing has been performed, and is documented in the description/comment.
  • You can enable/disable the change via feature toggles, and have confirmed these toggles work as expected.
  • Technical review: A Principal Engineer, Engineering Manager or Head of Engineering have green-lit your changes, and the reviewer is named in the description/comments.
  • Application Owner (Director+) approval is given about the PR, and the approver is named in the description/comments.

👉 Regardless of which boxes you click in this comment, merge/deployment will not be blocked.
Reports about deployment policy adherence will be circulated daily.

@szuecs
szuecs marked this pull request as draft November 13, 2025 22:05
@szuecs
szuecs force-pushed the feature/letsencrypt branch from 9aa7e75 to d3dd25d Compare May 12, 2026 19:27
@szuecs
szuecs force-pushed the feature/letsencrypt branch from 1348d8f to c1e6a3d Compare May 24, 2026 21:22
@szuecs
szuecs force-pushed the feature/letsencrypt branch from 1d69abe to be8f7a6 Compare June 21, 2026 18:09
@szuecs
szuecs force-pushed the feature/letsencrypt branch from be8f7a6 to 630543f Compare August 19, 2026 21:11
@szuecs
szuecs marked this pull request as ready for review August 19, 2026 21:50
@szuecs
szuecs force-pushed the feature/letsencrypt branch from 8cb4f34 to 6240641 Compare August 23, 2026 18:58
@szuecs
szuecs force-pushed the feature/letsencrypt branch from 6240641 to 6d39cce Compare September 6, 2026 19:37
@szuecs
szuecs force-pushed the feature/letsencrypt branch 4 times, most recently from 81c484d to bbc32dd Compare September 9, 2026 20:48
szuecs
szuecs commented Sep 9, 2026
View reviewed changes
Comment thread skipper.go Outdated

// TODO(sszuecs): does it make sense or do we want to chain TLSConfigs?
if o.Letsencrypt != nil {
return o.Letsencrypt.TLSConfig(), nil

@szuecs szuecs Sep 9, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@MustafaSaber @a4180p wdyt?
chain it or return here?

@szuecs szuecs Sep 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed

@szuecs szuecs Sep 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Please check this function. I wrote a one letsencrypt test for it and did not change the tests that existed so we do not break anyone.

@szuecs
feature: letsencrypt http.Handler integration via autocert with diffe…
4bc8ad0 
…rent storage/cache systems to be used by different deployments

Signed-off-by: Sandor Szuecs <sandor.szuecs@zalando.de>
@szuecs
szuecs force-pushed the feature/letsencrypt branch from bbc32dd to 4bc8ad0 Compare September 9, 2026 21:19
@szuecs
fix: tlsconfig setup
e0b7273 
Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
@szuecs
fix: skipper TLSConfig
886ff46 
test: letsencrypt TLSConfig

Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
@szuecs
fix: another edge case
64501cb 
Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
@szuecs
refactor: rename back to config to reduce changes
6a4c954 
Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
@szuecs
refactor: revert rename of exported function that could be used by sk…
2d79b2c 
…ipper proxies

Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
@szuecs
refactor: optimize for readability
ca54444 
Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
a4180p
a4180p reviewed Sep 11, 2026
View reviewed changes

@a4180p a4180p left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should also add docs for the feature, but it could be done later.

Comment thread skipper.go Outdated
Comment thread net/letsencrypt.go
func (d *DirCache) Get(ctx context.Context, key string) ([]byte, error) {
val, err := d.cache.Get(ctx, key)
if err != nil {
logrus.Errorf("Get %q -> %v", key, err)

@a4180p a4180p Sep 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we must return autocert.ErrMissCache if cerificate is not found in cache or certificate will not be created in case of cache miss
https://cs.opensource.google/go/x/crypto/+/refs/tags/v0.57.0:acme/autocert/autocert.go;l=304-314

@szuecs szuecs Sep 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Interesting, but I think we do because we wrap here the autocert.DirCache and it will return this kind of error. So the error returned will be this.

https://cs.opensource.google/go/x/crypto/+/refs/tags/v0.57.0:acme/autocert/cache.go;l=59

@szuecs szuecs Sep 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed in the other implementations: RemoteCache and InmemoryCache

@szuecs
fix: self implemented cert caches should return autocert.ErrCacheMiss…
3d7d57b 
… on cache miss to do the handshake to get a new cert

Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
@szuecs
fix: commented wrong code
554ab80 
Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
@szuecs

szuecs commented Sep 11, 2026

Copy link
Copy Markdown
Member Author

👍

@szuecs
szuecs merged commit 6074bcb into master Sep 14, 2026
18 of 21 checks passed
@szuecs
szuecs deleted the feature/letsencrypt branch September 14, 2026 11:53
This was referenced Sep 15, 2026
Merged
Merged
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@a4180p a4180p a4180p approved these changes

Assignees

No one assigned

Labels

enhancement feature definition minor no risk changes, for example new filters

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ACME support

3 participants

@szuecs @zalando-robot @a4180p