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

Fix problem with *() patterns if extglob is on. #522

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gmoniker
Copy link

@gmoniker gmoniker commented Mar 3, 2020

Running from cronjobs is usually fine, but on interactive shell there is a good chance that extglob is "on" and that will give *() and several other () combo's a totally different meaning in substition pattern matching.

@gmoniker
Copy link
Author

gmoniker commented Mar 4, 2020

Can someone rerun the tests? I don't think the failure has anything to do with this PR.
DUCKDNS_TOKEN not set
getssl: DNS_ADD_COMMAND failed for domain getssl.duckdns.org

@timkimber
Copy link
Member

Can someone rerun the tests? I don't think the failure has anything to do with this PR.
DUCKDNS_TOKEN not set
getssl: DNS_ADD_COMMAND failed for domain getssl.duckdns.org

It's not related to your change, it's because I don't have admin access to this repo so cannot set the required github secret ;-(

I'm in the middle of paralleling the tests, once that's done I'll try and make it not fail

@timkimber
Copy link
Member

Hi @gmoniker

Can you give some more details about the error you're seeing.

I've tested with extglob on and off and have seen no difference in behaviour. All the tests and my own servers have extglob=on.

@gmoniker
Copy link
Author

gmoniker commented Mar 5, 2020

At the moment I cannot reproduce it anymore on the account where I saw it. At that moment it was switching from a APIv1 requested cert to a APIv2 requested cert, so that may be a factor. I think Bash complained about an unexpected EOF happening in the first line of these:

  if [[ "$needbase64" && ${response##*()} != "{"* ]]; then
    # response is in base64 too, decode
    response=$(urlbase64_decode "$response")
  fi

So, I looked at the pattern and I thought well that should look for parentheses, but when I tried it on a commandline that didn't work, and then I discovered that with extglob off it did work as expected, and probably as was intended by the code.

@gmoniker
Copy link
Author

gmoniker commented Mar 5, 2020

Come to think of it, it may also be that I tried the updated getssl with the version 1 API, to see if I could first swap the scripts and then swap the api setting in the configuration of the domains.

@gmoniker
Copy link
Author

gmoniker commented Apr 3, 2020

By following the flows, I have concluded that this error won't show up in tests, because the cert(chain) has already been written then. So, in all productions it won't make a difference. And when v1 disappears it won't matter anyway.

With v1 the cert appears in binary DER form, and the response variable won't hold the correct data anyway, because a construction like VAR=$(cat file) will filter all nulls (and in newer BASH produce a warning).

@rdebath
Copy link
Contributor

rdebath commented Apr 7, 2020

AFAIK bash does not start with extglob turned on; one of the startup scripts has to turn it on. It is normal to link it to the interactive option (set by bash -i) but if your .bashrc isn't correctly conditioned this can cause problems.

BTW: shopt -u extglob works on bash 2.05a.0(1)-release -- Debian woody, so it should be safe to force it off if required.

On a related note I've recently noticed that git-bash on Windows turns -i on if you start a *.sh file from the GUI; so that can cause issues too.

@gmoniker
Copy link
Author

gmoniker commented Apr 14, 2020

@rdebath, From Ubuntu 16.04 in interactive shells the extglob option is 'on' by default. Without it you get errors on readline completion, because there are autocomplete dropins that use it.

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.

3 participants