Add option to disable DNS ACME provider#290
6543
momar Labels
No labels
breaking
bug
build_pr_image
documentation
d̶u̶p̶l̶i̶c̶a̶t̶e̶
feature
good first issue
improvement
i̶n̶v̶a̶l̶i̶d̶
open questions
performance
refactor
research required
No milestone
No project
No assignees
4 participants 
Notifications
Due date No due date set.
Dependencies
No dependencies set.
Reference
Codeberg/pages-server!290
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "histausse/pages-server:main"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR add the
$NO_DNS_01option (disabled by default) that removes the DNS ACME provider, and replaces the wildcard certificate by individual certificates obtained using the TLS ACME provider.This option allows an instance to work without having to manage access tokens for the DNS provider. On the flip side, this means that a certificate can be requested for each subdomains. To limit the risk of DOS, the existence of the user/org corresponding to a subdomain is checked before requesting a cert, however, this limitation is not enough for an forge with a high number of users/orgs.
I cannot see the comment on the interface. I guessed the issue from the content of the notification mail but that's quite inconvenient.
Is the comment problem just with me?
That's a really funny behavior: I started a review and then got interrupted by real world stuff and couldn't finish it, sorry.
I think that's a bug, I'll talk to the forgejo people and open a ticket if they also think that's a bug.
LGTM!
I don't know much about the acme code so I requested a review from @6543, because he has much more knowledge than me.
@histausse sorry for the long delay
Any news?
ping @6543
@ -33,6 +33,8 @@ func TLSConfig(mainDomainSuffix string,firstDefaultBranch string,keyCache, challengeCache, dnsLookupCache, canonicalDomainCache cache.ICache,certDB database.CertDB,noDNS01 bool,well I dont like to see this func get more args ... but well whatever
@ -67,3 +69,2 @@if strings.HasSuffix(domain, mainDomainSuffix) || strings.EqualFold(domain, mainDomainSuffix[1:]) {// deliver default certificate for the main domain (*.codeberg.page)domain = mainDomainSuffixinstead of use
domainand let it be not set tomainDomainSuffixI would rather create a dedicated bool value that we do check against ...the current code works like that ... but I would say it's a technical debt and should be cleaned up anyway
What information would the bool value contain? Whether or not to use the wildcard cert? I can try to do that but it will probably lead to modifying some part of the code I'm not sure to understand.
I think we can leave it like it is. Nobody understands the reasoning of every last bit of this code, so we shouldn't introduce large changes.
ok in that case let it merge and i try to send a smal patch to make it bit better
@ -223,0 +241,4 @@if name == mainDomainSuffix {mock_domain = "*" + mainDomainSuffix}return mockCert(mock_domain, "DNS ACME client is not defined", mainDomainSuffix, keyDatabase)This if can be remove if we remove
if useDnsProvider && domains[0] != "" && domains[0][0] == '*' {domains = domains[1:]}@6543 do you know what the test at L202 is for? I'm guessing it was for the dummy provider in the CI that does not suport DNS 01 but this PR replace the calls to the DNS 01 provider by a self signed certificat when it is not defined so it is not a problem anymore. (And this has the benefit of fixing #235, but I don't know if there are any hidden side-effects)
yes that's correct
9e5df1ab056fc224b359Sorry about the commit mess, I'm not on my usual computer.
This simplify a little the handling of wildcard certificate and fix #235 without breaking the integration tests.
No worries, and thank you for working on this! I had a quick look 2 days ago and stumbled upon some issues where I'll yet have to confirm whether or not they're caused by this PR; I think they probably have been caused somewhere earlier.
From my point of view, we can remove all mentions of acme.mock.directory from the code (and just leave it in the docs) with this PR.
Alright, I've tried it out, issues were due to misconfiguration on my side, but now it seems to work fine. No errors in local development without NO_DNS_01, and no issues with NO_DNS_01 and no provider, and no issues with NO_DNS_01 and a provider being set. It helps a lot that the PAGES_DOMAIN is since some time only treated as a suffix and when called directly is handled completely as a custom domain - the earlier approach is where the code mentioned in #296 is coming from.