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

Acme2 similar names #1628

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

ksperling
Copy link
Contributor

When using ACME2 with an order that contains multiple similar names (e.g. "api.example.com" and "api-example.com") the code that looks up the authorization state for a particular name can end up looking at the wrong entry due to the way grep was used without escaping the "." placeholder. I.e. the grep can end up returning multiple map entries:

[Mon 28 May 2018 15:48:27 NZST] Getting webroot for domain='video.alpha.example.com'
[Mon 28 May 2018 15:48:27 NZST] _w='dns_aws'
[Mon 28 May 2018 15:48:27 NZST] _currentRoot='dns_aws'
[Mon 28 May 2018 15:48:27 NZST] response='{"identifier":{"type":"dns","value":"video-alpha.example.com"},"status":"valid","expires":"2018-06-27T03:35:25Z","challenges":[{"type":"http-01","status":"pending","url":"https://acme-v02.api.letsencrypt.org/acme/challenge/WZ0RBOI0habN9pVqLX1G_h-TjTTcdjm5NRK1tlK_50o/4844224903","token":"aVdLaEuvhoAy7d6MHfwKT8eaz9q7afx7LqSmQCfqNLo"},{"type":"dns-01","status":"valid","url":"https://acme-v02.api.letsencrypt.org/acme/challenge/REDACTED/REDACTED","token":"REDACTED","validationRecord":[{"hostname":"video-alpha.example.com"}]}]}
{"identifier":{"type":"dns","value":"video.alpha.example.com"},"status":"pending","expires":"2018-06-04T03:34:35Z","challenges":[{"type":"dns-01","status":"pending","url":"https://acme-v02.api.letsencrypt.org/acme/challenge/REDACTED/REDACTED","token":"cdhmmNBqh9QWH1Dq6pDAEbZfUyeIQw6pHeETndepde4"},{"type":"http-01","status":"pending","url":"https://acme-v02.api.letsencrypt.org/acme/challenge/REDACTED/REDACTED","token":"REDACTED"}]}'

If one of these authorizations is valid while another is still pending, acme.sh will erroneously consider both of them valid, skip authorization, and then fail to issue the cert.

The dns_aws change is unrelated, but I found it while debugging this issue.

@Neilpang
Copy link
Member

Neilpang commented Jun 8, 2018

Hi, @ksperling
Thanks for your PR, it's indeed a bug.

The following 2 case can both grep successfully:

root@localhost# echo "api.example.com" | grep "api.example.com"
api.example.com

root@localhost# echo "api-example.com" | grep "api.example.com"
api-example.com

The reason is that the dot('.') in the grep expression is explained as a regExp wildcard char, so it matches any char.

It seems that your code can not work for this case also.

I think we need to think about another fix.

Thanks.
-Neil

@ksperling
Copy link
Contributor Author

ksperling commented Jun 8, 2018

I'm not sure what you mean, the fact that "." is a regex meta character is exactly what my patch fixes by escaping each "." in the domain name as "\." before passing the string to grep.

@Neilpang
Copy link
Member

Neilpang commented Jun 8, 2018

@ksperling
can you please try these code ?

root@st:~# d=api.example.com

root@st:~# echo "api.example.com" | grep "^${d//./\.}"
api.example.com

root@st:~# echo "api-example.com" | grep "^${d//./\.}"
api-example.com

There is no difference.

@ksperling
Copy link
Contributor Author

Ah, apologies, when I read your initial response on github somehow the markdown parser must have swallowed some of the backslashes. Just tried out your code, this seems to be a difference in behaviour between GNU grep and BSD grep:

karsten@qualifier:~$ grep --version; echo -e "api.example.com\napi-example.com" | grep "^${d//./\.}"
grep (BSD grep) 2.5.1-FreeBSD
api.example.com

karsten@deep4:~$ grep --version; echo -e "api.example.com\napi-example.com" | grep "^${d//./\.}"
grep (GNU grep) 2.25
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by Mike Haertel and others, see <http://git.sv.gnu.org/cgit/grep.git/tree/AUTHORS>.
api.example.com
api-example.com

I'll see what I can do to make this work with GNU grep as well... I had assumed escaping with backslash was defined by POSIX so didn't think to test multiple platforms.

@ksperling
Copy link
Contributor Author

Actually the issue seem to be with the pattern replacement, not grep itself, the following works on both platforms (extra backslash in the replacement):

grep --version; d=api.example.com; echo -e "api.example.com\napi-example.com" | grep "^${d//./\\.}"

I'll amend the pull request.

@ksperling
Copy link
Contributor Author

Could you please have a look at the updated PR? Cheers Karsten

@Neilpang
Copy link
Member

please merge the latest code.

@Neilpang Neilpang deleted the branch acmesh-official:dev January 19, 2022 12:56
@Neilpang Neilpang closed this Jan 19, 2022
@Neilpang Neilpang reopened this Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants