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

Try to fix ACME path when renew #33668

Merged
merged 4 commits into from
Feb 23, 2025
Merged

Conversation

wxiaoguang
Copy link
Contributor

Try to fix #32191

fix
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 21, 2025
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin labels Feb 21, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@pull-request-size pull-request-size bot added size/M and removed size/S labels Feb 21, 2025
@wxiaoguang wxiaoguang added the backport/v1.23 This PR should be backported to Gitea 1.23 label Feb 21, 2025
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 21, 2025
@silverwind silverwind changed the title Fix ACEM path when renew Fix ACME path when renew Feb 21, 2025
@yp05327
Copy link
Contributor

yp05327 commented Feb 22, 2025

One thing I can not understand why they save the config here.
image

If you trace the usage of this config, you will see that the config.Storage, config.DisableARI (which seems not related, but maybe this can explain why we should set the global default variables here, as certmagic.Default.Storage -> magic.Storage(by certmagic.NewDefault()) -> ACMEIssuer.config.Storage -> maybe used during renew)

And If didn't set the Default.Issuers, in certmagic.NewDefault() it will generate a default ACME in magic.config. It will be used in here:
image

image

It is a pointer so, we changed it later and it seems no problem:
image
But when certmagic.NewDefault() executed, it calls certmagic.NewCache(), and it starts a goroutine, and in it, it will start to renew certs, which may before we change the magic.Issuers?
image

This is just a guess, there are also many locks in it, so if we define the custom ACMEIssuer in Detault.Issuers at first instead of using certmagic.NewACMEIssuer can completely solve the problem?

@wxiaoguang
Copy link
Contributor Author

Yes, I also found that certmagic.NewDefault() and certmagic.NewCache are suspicious.

IMO it's either the certmagic package's design problem, or it's caused by our abuse.

But I don't use ACME and don't have time on debugging the details, so I think it's good enough to "try" to make a quick fix and leave a comment.

@yp05327
Copy link
Contributor

yp05327 commented Feb 22, 2025

There's another func certmagic.New() which is memtioned in the official README.
Not sure whether this is the correct answer, but maybe we can provide other test versions to check which solution can fix the issue, and then choose the best/acceptable one.

so I think it's good enough to "try" to make a quick fix and leave a comment.

Yes, maybe the title can be try to fix. Then let's try it.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 22, 2025
@wxiaoguang wxiaoguang changed the title Fix ACME path when renew Try to fix ACME path when renew Feb 22, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@wxiaoguang
Copy link
Contributor Author

There's another func certmagic.New() which is memtioned in the official README.
Not sure whether this is the correct answer, but maybe we can provide other test versions to check which solution can fix the issue, and then choose the best/acceptable one.

The certmagic code was introduced by "Use caddy's certmagic library for extensible/robust ACME handling (#14177)", maybe @techknowlogick have ideas about how to make it work correctly.

@wxiaoguang wxiaoguang merged commit f991807 into go-gitea:main Feb 23, 2025
26 checks passed
@GiteaBot GiteaBot added this to the 1.24.0 milestone Feb 23, 2025
@wxiaoguang wxiaoguang deleted the fix-acme-path branch February 23, 2025 05:12
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Feb 23, 2025
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Feb 23, 2025
wxiaoguang added a commit that referenced this pull request Feb 23, 2025
Backport #33668 by wxiaoguang

Try to fix #32191

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 24, 2025
* giteaofficial/main:
  Fix git empty check and HEAD request (go-gitea#33690)
  Fix some user name usages (go-gitea#33689)
  Try to fix ACME path when renew (go-gitea#33668)
  [skip ci] Updated translations via Crowdin
  Improve Open-with URL encoding (go-gitea#33666)
  Fix for Maven Package Naming Convention Handling (go-gitea#33678)
  Improve swagger generation (go-gitea#33664)
  Deleting repository should unlink all related packages (go-gitea#33653)
wxiaoguang added a commit to wxiaoguang/gitea that referenced this pull request Mar 5, 2025
wxiaoguang added a commit to wxiaoguang/gitea that referenced this pull request Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/done All backports for this PR have been created backport/v1.23 This PR should be backported to Gitea 1.23 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ACME certificate fails to renew (incorrect directory)
4 participants