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

internal/autocert: re-use cert if renewing failed but cert not expired #1237

Merged
merged 2 commits into from Aug 10, 2020

Conversation

cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Aug 10, 2020

Summary

If autocert failed to renew a valid cert, we should continue using that cert instead of aborting pomerium.

Related issues

Fixes #1232

Checklist:

  • add related issues
  • updated docs
  • updated unit tests
  • updated UPGRADING.md
  • ready for review

@codeclimate
Copy link

codeclimate bot commented Aug 10, 2020

Code Climate has analyzed commit 134e6f8 and detected 0 issues on this pull request.

View more on Code Climate.

@codecov
Copy link

codecov bot commented Aug 10, 2020

Codecov Report

Merging #1237 into master will decrease coverage by 0.1%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #1237     +/-   ##
========================================
- Coverage    61.6%   61.5%   -0.2%     
========================================
  Files         102     102             
  Lines        7678    7678             
========================================
- Hits         4735    4727      -8     
- Misses       2547    2556      +9     
+ Partials      396     395      -1     
Impacted Files Coverage Δ
pkg/storage/redis/redis.go 76.0% <0.0%> (-3.3%) ⬇️

Copy link

@grandmogbarkin grandmogbarkin left a comment

Choose a reason for hiding this comment

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

For what it's worth, my bug wasn't while the service was running, pomerium kept running just fine even with the renewals failing.

The issue was once I restarted - everything would block behind cert reissuing, envoy wouldn't even start up (I brought up a local build and sh'ed into the container, ps showed only /bin/pomerium running, no envoy instance was even present).

return fmt.Errorf("autocert: failed to renew client certificate: %w", err)
}
if !expired {

Choose a reason for hiding this comment

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

Will this log even if cert successfully renews since you don't update expired after cm.RenewCert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks, fair point 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

By factor out obtain and renew certification process, return specific
error for each process if failed to contact with letsencrypt server.
@cuonglm cuonglm force-pushed the cuonglm/fix-renew-valid-cert branch from 6923502 to 134e6f8 Compare August 10, 2020 10:03
@cuonglm cuonglm merged commit 277e6b5 into master Aug 10, 2020
@cuonglm cuonglm deleted the cuonglm/fix-renew-valid-cert branch August 10, 2020 16:26
@travisgroth travisgroth added the bug Something isn't working label Nov 12, 2020
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.

Certificate renewal process blocks envoy from starting on startup
4 participants