-
Notifications
You must be signed in to change notification settings - Fork 27
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
v2.3.2 MDMessageCmd :: Mailsend is broken #198
Comments
That's very strange. The only change in that area is that a couple of environment variables are passed to your script: The error says that your script is being run, but is unable to connect to your mail server on Are you sure that the mail server is running when the error occurs? It would be helpful to know what specific error Does If you open a command window, does Does that work from the account that Anything in your firewall logs? I'll be back in a couple of hours... |
Mailserver is running, all is fine . When I switch back to 2.2.7 or 2.3.1 mail is sent. with -v
Bottom line is that it stops working with 2.3.2 |
Hmmm. Can you add a That would log the environment variables that the batch job see and show us if this is the case... I'd do this, but I don't have a Windows environment with httpd... |
With 2.3.1, mail sent:
With 2.3.2 only these, no mail sent:
|
Thanks. That's what I suspected. Instead of adding the new variables to the environment, the change put ONLY those into the environment! I didn't make that change (though I asked for it). I'll see if I can come up with a patch. Thanks for the report and for your help. |
You can try this (untested & ugly) patch.
|
Adding the environment variables for MDMessageCmd and friends had the unintended side-effect of passing ONLY those variables (and a couple of others) to the child. This means no Path on Windows, among other things. icing#198 reported this issue. This patch should work - except that technically, the environment variables aren't thread-safe. The right thing is probably to add a mutex - but figuring out how APR spells that is for another day. @icing?
Sending mail again now with the patch. But no MOD_md env's :
|
We're on the right track. I'm not surprised that it's only a partial fix - the whole environment area is very OS-dependent. @icing has to make a call - there are only bad choices. I looked at what
Also note that copying the parent's environment table is not a great idea - but even if one wanted to, there is no portable way to list all the entries. (there's an extra argument to This leaves several unpleasant alternatives:
These are all bad - just different flavors of bad. Since I don't own |
Did not tested mdnotifycmd. Same issue ? Ps. |
Yes, same issue, common code.
Funny you should ask :-) This started when I wanted to add just one arg, and submitted a very small Pull request. I was overruled. And, to be fair, when it works, EnvVars are the more scalable solution. The test suite was updated, and passed. It checked that in fact the new items I'm sorry for the difficulty that it caused you. Thanks again for your help with |
Interesting. Thanks for the summary! Now what to do... |
It's a tough call, with no good choices. That's why you're the project leader, and I'm just the helper. I'm willing to discuss further if it would help. Here, or off-line (my e-mail is in commits.) |
I still believe in the env thingie. I will make a change later, after I get the last merge working here. |
I agree that setting up a workable env is the best place to end up. It's how to get there that has no good choices. On the merge - it should just work. On the mod_ssl patch, it is based on the 2.4.41 mod_ssl with the client verification patch that you know about. I'll be away for a bit, but back later. Let me know if I can do anything to make merges easier. |
@SteffenAL, could you check if v2.3.3 fixes the problem for you? Thanks! |
No. In the .bat file the mail is now send. This is what is passed
But a Powershell call in the .bat is not executed:
Looks like the script needs more env's (it is missing a few). Powershell is nowadays mostly used) Please pass all ? |
I would pass all, I just do not know how to iterate them. |
Do not understand ? Because you can pass now 6. |
I can lookup a variable if I know the name. But I cannot get all names of existing variables on all operating systems in a common way. |
Thanks for explain. Also the command Ping is not working: I think we need all. otherwise I prognose that quit some scripts from our users are not working anymore. |
You can't. At least not portably. As I noted above, there are several mechanisms, and we can't catch them all. Further, they're not all necessarily desirable. As I pointed out several replies back, the code that sets up for CGI has lots of OS-dependent conditionals to find the "important" variables. But it requires a request, which we don't have. And we don't want to maintain a duplicate list. I'm leaning toward choice 4 as the quickest fix in the short term, but longer term reducing the maintenance effort. It really ought to be common code - for loggers, etc. There is a choice 5 that you might also try. Run the command in a shell - e.g. pass "sh -c properly-quoted command". Then the shell will setup the usual variables. It's not very efficient, but it might buy some time to get the core work done. There are also issues with shell escape characters - but since we can trust the admin who is setting up ... |
After some more thought, i want to give up on using the environment. Even if we have a (shared) list of names, that doesn't preclude someone from introducing other important (to her) variables that are needed. So, I think it is time to revert back to @tlhackque original idea of just passing one additional argument, but with a twist: We add a
With
Does this work for everyone? |
No version arg ? Extra arg's no problem here. MDNotifyCmd same as with 2.2.7 ? Should work for me. MDEventCmd is a better name. Just check that I understand : And MDMessageCmd no directive anymore ? |
We'll keep If you want, we can add the parameter |
I the post above you said Version should be useful here. Special with testing I sometimes are mixed up when report an issue. Now we get two almost the same (differ two exta args) two directives. |
I was afraid that's where we'd end up. But then, I've been through a lot of these problems :-( That might work; effectively we are passing an environment of the form var=value (and the script/program can actually do something like: It does limit us to the maximum line length of the shell - probably not too bad. We do, however, already have more than one var - and problem to solve. As noted (and in my code), "domain": is currently ambiguous. It's the certificate issuing domain for events like renew, but it's the DNS name to be updated for a DNS01 challenge - and that's a problem. I went to some trouble to get the issuing domain to DNS01 too. It's very important to always have the cert domain - by that I mean the <MDDomain foo> value. That's how we can find out the right data - including the directory where the certs are stored. The DNS domain for a challenge is useless for that (but necessary to update the DNS.) I really want one common argument format for all callouts to a script. And the cert issuing domain needs to always be there. So, I suggest this:
This allows the program to get it's options as currently. It ensures that the program ALWAYS knows what cert domain is the subject of the event. It doesn't make sense to add names for things that we absolutely know that we need - e.g. for a DNS challenge, we need the DNS domain and token; for an HTTP-01 challenge we need the host, token-name and token contents. The program knows what to expect if it handles that event. All the var=val does for those is add complexity for the program - and consume command line space. For the unexpected or useful to a more limited audience, like version and MDStoreDir, I like your idea of var=val. |
I think it is better and more clear to keep It does not make sense to give the DNS names in events for me. Do you want 20 'renewed' and 'installed' events for a certificate with that many alt names? If you want to use a single script, give it a first parameter 'event' or 'dns-01'. |
Deprecated means "not prefered, but still works". Might be removed eventually. But people have time to adapt.
There is a limit - it's larger than it used to be. I think on the order of several KB. The nice thing about @icing's proposal is that for things we don't think of now, the data is self-identifying. So a script written today can display or log without knowing what it is. Also, it's like having named parameters to a subroutine rather than counting arguments. I think it's a good idea. My suggestion is a mixture - use fixed parameters where we know, from the event definition, what's required. Use named parameters for the helpful, "nice to have", and we don't know what we want today... |
If we end up with a lot of parameter, we can always add a |
I do want to handle everything in one script. There's a lot commonality. Events are things that happen. They all need data about the certificate that triggered them. And yes, if I have 20 names, I probably have at least 10 hosts (www.example.net & example.net); more if they're SMTP or other services. Remember that if they are remote, I need to know the names and push the certificates to the remote hosts. This is possible (for me common) for DNS challenges. For http-01, it's the same situation, just a different way of installing the challenge. If mod_md doesn't provide an event per target, the progam will have to find the certificate (or CSR), parse it (probably running openssl), and then work over the SANs. This seems unnecessary, since mod_md already has all this information parsed... The way the md_events script now works is that it has a hierarchy of configuration:
Effectively that's what I'm calling the cert domain (to distinguish from the dns challenge domain name). I don't think we will end up with a lot of parameters coming directly from mod_md. We need the event name. We need the things that are intrinsic to the event. And we need enough environmental information to find out how to handle the event - which includes the targets. (Which can be hosts now; in the near future, IP addresses are coming; and some provider will deliver e-mail addresses - all these are SubjectAlternativeNames.) Finally, the optional args allows the script to get data from the admin - like "use this global configuration file - e.g. because that goes with customer a". I suppose eventually we might want some information from the mod_md database - like next renewal. But that is available as json from the md-status handler.... Beyond those, information lives in configuration files or a database - mod_md doesn't really have it. I've been working all night - it's now breakfast time here. Expect some delay on further response. ... |
Oh, and for that, as a placeholder, I currently accept |
This discussion ties in to a document that I committed back on 12-Mar -- I've updated it, the latest version is in my repo. It's a bit rough (I'm short on sleep), but I think worth reading. Also note that |
…NotifyCmd. This prevented the inheritance of existing environment variables as there seems to be no portable way to iterate those on all platforms. This led to a regression on Windows, see #198.
@SteffenAL could you test the lastest 2.3.x release if the mail command is working again? I remove the meddling with the environment settings. Thanks! |
Oeps missed this one question. v2.3.5-git: looks ok, get Installed, OCSP-renewed, expiring. But dealing with no renew, see #235 No MDNotifyCmd message seen and missing MDMessageCmd renewed |
In HTTPD 2.4.48 all my issues are solved. And with nice additions. Thanks! |
Have a messagecmd.bat file to send a mail, in contains:
with v2.2.7 and v2.3.1 all is working fine
With 2.3.2 no mail is sent and in the log:
Running Windows.
The text was updated successfully, but these errors were encountered: