-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
ACME V2 Integration #3063
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
ACME V2 Integration #3063
Conversation
5c4092f to
7796017
Compare
acme/acme.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SANs are not allowed
emilevauge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @nmengin !
Few comments though ;)
acme/localStore.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/OldRegistration/ACMEv1Registration may be more explicit
acme/acme.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is an issue in this test
acme/acme.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/getValidDomain/getValidDomains
Gopkg.lock
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any intentions to fork this for stability? Not really excited to use a feature branch on a third party repo as a basis for prod use...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @dtomcej.
We are not enthusiastic either but, for the moment it's the only way we have if we want to integrate ACME V2.
Moreover, I tested a lot and the branch seems to be stable.
We are following the LEGO repo and, if a big problem appears, we will be able to fork the repo with a stable version thanks to the vendoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
acme/acme.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this fully backwards compatible? Is it something that we should be defaulting back to v1 somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dtomcej It's not backward compatible.
That's why getAccount methods have been modified.
They can detect V1 Account format and delete is from the ACME structure.
Thus, Træfik reads the new configuration and create a new V2 ACME Account.
However, of course, ACME certificates generated in V1 are compatibles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
65e605e to
89478ab
Compare
docs/user-guide/examples.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't do that (wildcard with sans) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No you can't do it for the moment!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really ?
mmatur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @nmengin 👏
Few comments in the review
acme/acme_test.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a new line please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
docs/configuration/acme.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space missing between DNS-02 and Challenge
acme/localStore.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even instead of ven
docs/configuration/acme.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a new line for this part ? The note in the documentation is broken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's another note but a space is missing I add it.
docs/user-guide/examples.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should maybe use "https://acme-staging-v02.api.letsencrypt.org/directory instead of http://172.18.0.1:4000/directory
docs/user-guide/examples.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can not do that. If main domain is a wildcard you can not use sans
provider/acme/provider_test.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please remove this line
fae09e6 to
8dbfd5a
Compare
acme/acme.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleteUnnecessariesDomains instead of deleteUnecessariesDomains
8dbfd5a to
fb1417e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Great job @nmengin 👏
emilevauge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @nmengin !
Very much awaited feature ;)
LGTM
|
thanks! literally much awaited feature in my project! [[acme.domains]]
main = "mydomain.com"
sans = ["*.mydomain.com"]and getting error so who then should I define wildcard? |
|
Hello @sarkistlt , Many thanks for your feedback. For the moment, we do not support wildcard certificates with/in SANs. You can use the configuration described below : If you still have problems, I suggest you to open an issue or to join our Slack workspace for more community #support. |
|
thanks for response, I just downloaded source code of traefik and it still uses |
|
actually just saw version |
|
getting another error |
|
You can find info on how to configure the wildcard certs here: https://docs.traefik.io/v1.6/configuration/acme/. Let's Encrypt uses a DNS challenge for issuing wildcard certificates, which requires setting a DNS TXT record. Traefik can automate this via the API of your DNS provider (if it is supported), for which it requires an API key. |
|
You can also join our Slack workspace for more community #support. Remember, we dedicate the issue tracker to bug reports and feature requests only |
| [acme] | ||
| # ... | ||
| [[acme.domains]] | ||
| main = "*local1.com" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this PR do?
This PR allows Træfik to use the new ACME version.
It uses the new lego branch to access to the ACME V2 API.
ACME V2 API main features :
TLS-SNI-01challengeDNS-02challengeMotivation
Fix #2674
Allowing users to do ACME wildcard certificates
More
Additional Notes