Closed Bug 68877 Opened 24 years ago Closed 21 years ago

"HELO/EHLO domain of sending mail account" should be "HELO/EHLO host.domain of machine running Mozilla"

Categories

(MailNews Core :: Networking: SMTP, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: briggs, Assigned: ch.ey)

References

Details

(Keywords: fixed1.7)

Attachments

(1 file, 20 obsolete files)

(deleted), patch
darin.moz
: review+
mscott
: superreview+
chofmann
: approval1.7+
Details | Diff | Splinter Review
The argument to the SMTP HELO command is supposed to the be fully qualified domain name corresponding to the IP address of the interface on which the host is originating the SMTP connection. The current system uses the domain component only, not the full host name. That is, from host xxx.parc.xerox.com, the HELO command should be "HELO xxx.parc.xerox.com", not "HELO parc.xerox.com" as it currently does. p.s., there's no Networking: SMTP in the component list.
Reporter what version of Mozilla are you using? Have you tried with the latest nightly builds and with a new Profile/ does the problem still appear?
Summary: SMTP "HELO domain" should be "HELO host.domain" → SMTP: "HELO domain" should be "HELO host.domain"
Reassigning to mailnews, where this bug belongs
Assignee: neeti → mscott
Component: Networking → Networking - SMTP
Product: Browser → MailNews
QA Contact: tever → esther
Marking NEW.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I recommend sending a bracketed dotted quad: HELO [128.2.15.1]
> HELO [128.2.15.1] That should be a last resort. It's deprecated in the relevant RFCs. You're supposed to send the fully qualified primary name associated with the IP address of the interface on which you are speaking. This SHOULD match the fully qualified name retrieved with a reverse DNS lookup -- e.g., in your example, the PTR type query on 1.15.2.128.in-addr.arpa. These days, some sites block mail from anyone (typically spammers) who won't HELO with the FQDN for the host/interface.
Address literals are not deprecated in draft-ietf-drums-smtpupd-13.txt, the latest SMTP standard. They are perfectly legal. (HELO, on the other hand, is deprecated in favor of EHLO.) The argument to HELO/EHLO provides no useful information. Obtaing the correct FQDN of the client is extremely difficult on some MUA platforms. Blocking sites based on the argument to HELO/EHLO is not a rational anti-spam strategy, as spammers can get this information just as easily if not more easily as legitimate MUAs can.
The drums draft says: 4.1.1.1 Extended HELLO (EHLO) or HELLO (HELO) These commands are used to identify the SMTP client to the SMTP server. The argument field contains the fully-qualified domain name of the SMTP client if one is available. In situations in which the SMTP client system does not have a meaningful domain name (e.g., when its address is dynamically allocated and no reverse mapping record is available), the client SHOULD send an address literal (see section 4.1.3), optionally followed by information that will help to identify the client system. If you CAN get the FQDN of the client, you should; if you can't, you should send the address literal. Note that my original complaint was that for a host whose name was host.a.b, the mozilla SMTP client was issuing a "HELO a.b", or "EHLO a.b", neither of which conform with any possible reading of ANY of the SMTP specs.
BTW, I didn't find any correct way to find host's FQDN. I can get hostname using NSPR, but I cannot get domain name of the host mozilla's running on (currently, mozilla uses user's email address to get domain name, but this is not always the same as localhost's domain name)
*** Bug 159875 has been marked as a duplicate of this bug. ***
*** Bug 179905 has been marked as a duplicate of this bug. ***
Can't we use our own ip-address, and then do a reverse-lookup ? Or would that be too difficult on multi-home hosts ?
Doing reverse name lookups stinks. The current way of doing it is incorrect though: (RFC 2821) The domain name given in the EHLO command MUST BE either a primary host name (a domain name that resolves to an A RR) or, if the host has no name, an address literal as described in section 4.1.1.1. Maybe someone ought to check what other clients do. As for the address literal, I think that should be doable, as the string has to be sent over a network socket, so you could get the source address from that socket.
Since we can't do reverse lookups (unreliable and slow), my vote is for the ip-address too. It's more correct that the current domainname. We can use getsockname() to get the address from the socket that was opened to the mail-server. The fix has to be done in nsSmtpProtocol.cpp (functions nsSmtpProtocol::LoginResponse and nsSmtpProtocol::ExtensionLoginResponse, for HELO and EHLO)
I traced the smtp connection from mozilla mail to my mail server today (Mozilla 1.2.1 Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2.1) Gecko/20030119) and got the following: 220 starwolf.biz ESMTP Sendmail 8.11.6/8.11.6; Tue, 28 Jan 2003 14:43:52 -0500 EHLO (none) 501 5.0.0 Invalid domain name HELO (none) 501 5.0.0 Invalid domain name MAIL FROM:<"bartof"@(none)> 250 2.1.0 <"bartof"@(none)>... Sender ok RCPT TO:<bartof@rpi.edu> 550 5.7.1 <bartof@rpi.edu>... Relaying denied I don't know where it got (none) from for the EHLO and HELO lines, becuase I know that my hostname is set to bartof.no-ip.com locally
It's the default in the new account wizard.
It is a well known fact (I hope) that mozilla puts this address (domain taken from 1st mail account) into EHLO. The problem is, it should not. Can anyone tell me, why fixing this problem takes THAT long? Anyone (including me) can not use Messenger if his account domain is different than actual host ip domain... Lukasz
> Can anyone tell me, why fixing this problem takes THAT long? Because it's assigned to someone who was on sabbatical for most of the last year.
If this is not easily fixed the correct way, is it possible to at least provide an override in the prefs.js to set the HELO string on a per-SMTP server basis. Remember that the HELO FQDN has nothing to do with the sender's email address, which is what Moz currently uses to compute the HELO string. If the hostname of my outbound IP is abc.acme.com and my sender email address is john@generic.org, then the HELO string should still be "abc.acme.org". Also as I manage SMTP servers and am imvolved in writing anti-SPAM filtering software, I can tell you that SMTP servers which enforce the standards with regards to the HELO/EHLO parameters are currently very effective at eliminating a lot of SPAM. So I think it is very important that the Moz MUA tries to follow the standard exactly so SPAM filters don't block it.
Based on dupes this bug is All/All
OS: Solaris → All
Hardware: Sun → All
*** Bug 4425 has been marked as a duplicate of this bug. ***
Deron, Lukasz, putting the interfaces ip of the machine which runs Mozilla in the HELO/EHLO request shouldn't be the problem. Resolve it to an FQDN is much more difficult. But it's nearly impossible to get a ip the SMTP server likes if the machine running Mozilla is behind a NAT router or similar (-> e.g. 192.168.0.7). Would adding the current ip of the machine to HELO/EHLO satisfy SPAM filters? At least if the machine directly dialed in and has a unique ip?
Christian, Deron's suggestion of adding a possibility to explicitly set FQDN for EHLO would solve at least my problem perfectly :) I get an ip always from the same domain, as lots of other ppl getting ips from a provider's dhcp, that always resolve to the same domain. I think we could live with solution that is good in >90% cases; possibility of manually setting FQDN would solve the rest. The only annoying situation I can imagine is when used ip is not correctly resolvable (by any means we could use) AND _at the same time_ its FQDN differs at least from time to time. Lukasz
Yes, at a minimum, and default, it should place the IP address of the interface used into the HELO command. This is acceptable per the SMTP RFC if there is no easy or reliable way to get an FQDN. And it can't be [127.0.0.1] unless you're running your own MTA (which makes this problem irrelavent anyway). Just be sure to follow the RFC's for the syntax (both for IPv4 and IPv6). However if you do want to use a reverse DNS query to try to get a FQDN I see no problem with using that answer, only revert to IP address if you get no DNS answer. If DNS returns a "wrong" answer, well that's not Mozilla's fault. I would not worry about the issues that NAT's may raise, that's outside the scope of what a client application should have to worry about (or even know about). If a NAT point has to be crossed between Mozilla and the SMTP server, then it is the responsiblity of the router/firewall administrator to do the appropriate poxying (or better yet run an MTA which is multihomed across the NAT point). I guess my point is that you MUST figure out which interface will be used and then ATTEMPT to determine if there is a unique and valid reverse DNS entry to find an FQDN. If you can't determine the FQDN use the IP of the interface. This lookup should be done for each SMTP connection, as it may change in DHCP environments. A manual override in prefs.js might still be nice though.
Slightly offtopic, but I guess I can also provide some example of bogus HELO's that is very frequent to find in use with spammers (hope they don't see this 'cause I want them to keep ignoring the RFC)....some spammer examples... HELO dYu2sW37 <-- spammers love random nonsense HELO [127.0.0.1] <-- remote MTA can't be on my loopback HELO acme.com <-- when MY own site is "acme.com" HELO 66.55.44.33 <-- No [] syntax I find this can catch about 30%-40% of all spam and almost never thows out legit MTAs. Furthermore when running a gateway MTA, I can also catch spam coming from an outside address claiming it's from inside (e.g., HELO inside.pc.acme.com). If Mozilla follows the RFC, then it will look less like these spammers.
I just want to add my 2 cents for those who underestimate the importance of this bug. Mozilla doesn't just send the domain name instead of the host name - it sends the domain extracted from the sender's e-mail address, which amounts to forgery of the headers, which may be *illegal* in some jurisdictions. I know a guy who uses Mozilla to write e-mails from his yahoo.com address. His messages have a header: Received: from yahoo.com (his.real.hostname [192.168.0.2]) Spamassassin marks those messages as having a forged yahoo header, and rightly so. That's 4.9 points in Spamassassin 2.53 even after taking into account a valid Mozilla signature: X-Spam-Status: Yes, hits=4.9 required=5.0 tests=CONFIRMED_FORGED,FORGED_RCVD_TRAIL,FORGED_YAHOO_RCVD, RCVD_FAKE_HELO_DOTCOM,USER_AGENT_MOZILLA_UA version=2.53 That's 0.1 below the default limit. What should I tell that guy? Stop using Mozilla for Mail?
Flags: blocking1.4b?
Summary: SMTP: "HELO domain" should be "HELO host.domain" → "HELO domain of sending mail account" should be "HELO host.domain of machine running Mozilla"
Flags: blocking1.4b? → blocking1.4b-
This is _not_ a bug. Mozilla mail works perfectly this way. In fact it works better than using the proposed idea. Please close this bug report and forget the idea. If the machine from you are sending mail doesn't have a FQDN and the mail server requires a FQDN in HELO, the proposed idea will fail. Resolving the name it's a very bad idea: - It's something from other layer (DNS/IP) not from SMTP - It breaks when the name of the computer is not FQDN (as many dial-ins do) and the SMTP server does strict EHLO/HELO checking as stated before. - It breaks computers with a TCP tunnel to another host from the connection is originated if the relay does strict EHLO/HELO checking. - It breaks computers using NAT, the host that sees the server is not the one that sends the message if the relay does strict EHLO/HELO checking. - It's considered spyware as you are sending information some companies or people don't want to say: the internal structure of the network. Netscape Mail has done this for years and it has worked always. Netscape Mail is the reference SMTP client implementation.
> Netscape Mail is the reference SMTP client implementation. Up to there, your comment could be viewed as reasonable....
*** Bug 209999 has been marked as a duplicate of this bug. ***
OK, Netscape's mail servers now apparently filter out mails with bogus HELO lines. People are reporting mail sent from Mozilla to @netscape.com addresses bouncing due to this bug. This makes mailnews unusable as dogfood for developers working on the project, which is rather sub-optimal. Could we get some traction on this, please?
jgmyer's suggestion of "HELO [ip]" could be implemented using the nsIDNSService's myIPAddress attribute.
I implemented EHLO [ip] using nsIDNSService's GetMyIPAddress() in April for testing purposes. It worked and still does as far as I can test. But I requeued it because 1. sending a FQHN was the major aim and 2. the result wasn't compatible with IPv6.
Attached patch suggestion for a patch (obsolete) (deleted) — Splinter Review
Code to send the ip-address literal instead the domain of sending mail account with HELO and EHLO. IPv4 style only. I kept the current code to have a fallback if GetMyIPAddress() fails for some reason.
Better to use the connection's local IP address. A multihomed client could easily send the wrong address if it uses the one from the DNS client.
Regardless of how this is ultimately solved, I would still highly recommend that the EHLO identification be overridable via a configurable preference on a per-SMTP server basis. This is unfortunately part of the SMTP spec which the IETF failed to provide any reasonable solution (and why almost all MTA software also makes this a configurable setting). Having a per-SMTP server preference will allow those who have unusal multi-homed, natted, proxied, or servers with agressive spam filters to at least have a workaround available to them. Again as this can actually prevent the ability to send mail at all in some network settings, we at least need some form of workaround soon! So unless this is easy to fix correctly (which I suspect it is not), I would like to see a configurable preference in as soon as possible, and work on obtaining the real network interface identity later.
John, you're welcome to write a patch doing this. At the moment I don't know how to as this seems to be a more-gecko-thing. Deron, I'll add such a pref soon.
The pref itself is no problem. But implementing it two questions came up. Should it be a per server pref or a general one? Having a per server pref it's more flexible but bothersome if it has to be changed. While for a new SMTP UI (see http://bugzilla.mozilla.org/attachment.cgi?id=120897&action=view) I did a per server, I don't think this is necessary. Doing a general pref I've no clue where to put it in a UI.
Christian, I would still argue that this should be per-SMTP server since a different network path may be taken for each and the DNS for them may see a different "world" (think an internal mail server behind a NATing firewall and a public one out on the internet, secured against relaying of course, and perhaps a third reached through a VPN). If you're going to provide a config to help out those with tricky networking scenarios, then you should do it per server I think... unless of course that's too hard to do. As far as a GUI, that would be great, but I wouldn't be too upset if it just has to be manually added in to the prefs file (or via about:config). I'll leave that decision to the Mozilla developers. It's certainly an advanced option though. You need to present it in a way that won't confuse users...it's changing the SMTP EHLO greeting, not any email addresses in any way. That needs to be very clear. Also about your GUI mockups, "userdefined" is probably not needed (of course anything entered on this screen is obviously "user defined"). Perhaps the word "custom"? Also rather than "computername", I'd probably stick with "hostname". Computer name is often associated with a SMB (Windows) node name, and not a DNS FQDN. Oh, and this may not be your doing, but doesn't "user name and password" belong under the "security" tab? Anyway thanks for working on this while the network gurus contemplate the harder problem.
A per server pref isn't much harder to do and I understand your statement. > As far as a GUI, that would be great, but I wouldn't be too upset if it > just has to be manually added in to the prefs file (or via about:config). Ok, so I'll omit this for now. > Also rather than "computername", I'd probably stick with "hostname". I chose "computername" because there's already the hostname pref for smtpservers though in the UI it's named "Server Name". I don't like "computername" too but the panel in bug 202468 is just a raw suggestion. > but doesn't "user name and password" belong under the "security" Hm, yes, that should be better. I note that for a new version, but as stated above, the panel is just a raw concept for a new all-in-one SMTP-UI. > Anyway thanks for working on this while the network gurus contemplate the > harder problem. Having a makeshift isn't always good because it can lead to "why looking at this, it's working". But we've a real problem if some servers don't accept Mozilla mails, so that should be ok.
Attached patch suggested patch v2 (obsolete) (deleted) — Splinter Review
So here's the patch with the new pref added. The pref is named mail.smtpserver.smtpx.machine_name where x is the server No. We can talk about machine_name - but hostname is already given. Maybe FQDN or machine_domain or something.
Attachment #126113 - Attachment is obsolete: true
Attached patch suggested patch v3 (obsolete) (deleted) — Splinter Review
Eliminated helper var myIPLiteral (which wasn't freed).
Attachment #126450 - Attachment is obsolete: true
Attachment #126920 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 126920 [details] [diff] [review] suggested patch v3 I'm not into network foo but: I think you should construct the [address] at about line 300. I think your string contatenation foo is wrong. I think machineName should have a setter. I think you should add a pref mail.smtpserver.default.machine_name (although with an empty value) so that you can change the default for all the servers at once.
Attachment #126920 - Flags: review?(neil.parkwaycc.co.uk)
> I think you should construct the [address] at about line 300. Er, why that? We've GetUserDomainName() to create this string. > I think your string contatenation foo is wrong. As you might have seen in the last bug I'm not good in string objects. I could also do m_machineName.Assign("["); m_machineName.Append(myIP); m_machineName.Append("]"); if that's better. > I think machineName should have a setter. Hm, I can't see why as long as we've no UI. > I think you should add a pref mail.smtpserver.default.machine_name > (although with an empty value) so that you can change the default for > all the servers at once. No prob to do. I left this out because we have no getDefaultCharPref() there. Should I add such a helper function or would it be ok to just do a hardcoded prefs->CopyCharPref(mail.smtpserver.default.machine_name, machineName) in GetMachineName()?
Attached patch patch v4 (obsolete) (deleted) — Splinter Review
Patch v3 with changes addressing Neils remarks 2 to 4. The setter is unused for now but ready for implementing a UI.
Attachment #126920 - Attachment is obsolete: true
Attachment #127528 - Flags: review?(sspitzer)
Comment on attachment 127528 [details] [diff] [review] patch v4 >+ if(!m_machineName.IsEmpty()) >+ return (const char *)m_machineName; >+ else No else after return. Also, use .get() rather than (const char *) cast -- the latter is deprecated (see string/public/nsXPIDLString.h). > if (m_runningURL) > { >Index: mailnews/compose/src/nsSmtpProtocol.h >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/compose/src/nsSmtpProtocol.h,v >retrieving revision 1.45 >diff -u -r1.45 nsSmtpProtocol.h >--- mailnews/compose/src/nsSmtpProtocol.h 8 May 2003 18:54:25 -0000 1.45 >+++ mailnews/compose/src/nsSmtpProtocol.h 11 Jul 2003 12:20:46 -0000 >@@ -173,6 +173,7 @@ > PRUint32 m_addressesLeft; > char *m_verifyAddress; > nsXPIDLCString m_mailAddr; >+ nsXPIDLCString m_machineName; Looks like people have been violating the modeline, both here (tabs) and elsewhere (two-space c-basic-offset) -- and in all these files. Thanks for upholding it. May be worth fixing and using diff -w for reviewers. Sspitzer, what do you think? > NS_IMETHODIMP >+nsSmtpServer::GetMachineName(char * *machineName) >+{ >+ nsresult rv; >+ nsCAutoString pref; >+ NS_ENSURE_ARG_POINTER(machineName); >+ nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv)); >+ if (NS_FAILED(rv)) >+ return rv; >+ getPrefString("machine_name", pref); >+ rv = prefs->CopyCharPref(pref.get(), machineName); >+ if (NS_FAILED(rv)) >+ { >+ rv = prefs->CopyCharPref("mail.smtpserver.default.machine_name", machineName); >+ if (NS_FAILED(rv)) >+ *machineName = nsnull; >+ } Should failures be suppressed here? Don't they mean out-of-memory or something worse (and therefore worth returning)? >+nsSmtpServer::SetMachineName(const char * machineName) >+{ >+ nsresult rv; >+ nsCAutoString pref; >+ nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv)); >+ if (NS_FAILED(rv)) >+ return rv; >+ getPrefString("machine_name", pref); >+ if (machineName) >+ return prefs->SetCharPref(pref.get(), machineName); >+ else No else after return. /be
>+ if(!m_machineName.IsEmpty()) >+ return (const char *)m_machineName; >+ else > No else after return. Hm, I find it more clear to see what's the alternative to the if. But ok, no prob to change. > Also, use .get() rather than (const char *) cast -- the latter is > deprecated (see string/public/nsXPIDLString.h). Ah, ok. I used what is used down in the current function. > Looks like people have been violating the modeline, both here (tabs) and > elsewhere (two-space c-basic-offset) -- and in all these files. Thanks for > upholding it. May be worth fixing and using diff -w for reviewers. I'm using modeline params in new functions and single new lines but preserving the mode used in functions I'm changing. Strictly following the modeline or changing current lines has been criticized by reviewers in the past. What sould be the benefit of a -w here? >Should failures be suppressed here? Don't they mean out-of-memory or something >worse (and therefore worth returning)? For the > rv = prefs->CopyCharPref(pref.get(), machineName); I'd say clearly no. And for > rv = prefs->CopyCharPref("mail.smtpserver.default.machine_name", machineName); I don't know.
> Hm, I find it more clear to see what's the alternative to the if. You and others weren't using it consistently, it overindents the else clause, which is often the normal-termination case (the bulk of the body of a function or method, frequently), and it doesn't read right to anyone who is thinking about control flow expressed in a C-like language (I was schooled in Pascal, I've been there -- don't mean to disparage). It's important to execute mentally when reading. > But ok, no prob to change. Thanks. > What sould be the benefit of a -w here? diff -wu will not show trivial whitespace changes, so you can expand tabs and reindent at will. Reviewers may stipulate minimal change near a freeze, but should be open to modeline conformance fixes otherwise. You should attach a diff -u version of the patch for applying, not for reviewing, if you do fix lots of whitespace glitches. cvs diff -p is good to use in conjunction with -u, btw. If you add diff -pu to your .cvsrc file, you'll always get it (I use -pu8). It shows function or method (or goto-label) tags introducing each hunk of diffs. I'm still not convinced that suppressing pref allocation errors is the right thing. Is there an ambiguity problem where some failure codes mean "pref not found" while others mean "out of memory" or something worse? /be
> diff -wu will not show trivial whitespace changes, so you can expand tabs and > reindent at will. Er, yes, I know what it does. And I use it sometimes (e.g. if reindenting complete blocks), but I meant in this case. Anyway, I now got what you meant in your first comment. And I agree. > cvs diff -p is good to use in conjunction with -u, btw. Hey, that -p is great. I'll add this to the default. > I'm still not convinced that suppressing pref allocation errors is the right > thing. Is there an ambiguity problem where some failure codes mean "pref not > found" while others mean "out of memory" or something worse? I'm not convinced too. But CopyCharPref() returns NS_ERROR_UNEXPECTED if the pref wasn't found. That shouldn't happen for mail.smtpserver.default.machine_name as we have a default in mailnews.js.
CopyCharPref can never "fail" as such for a known char pref, the best it can do is to set your pointer to null when it fails to strdup the value. Perhaps you could do better by setting the pointer to null anyway before calling CopyCharPref, that way you don't have to check the return value at all.
Neil, shouldn't strdup failure cause NS_ERROR_OUT_OF_MEMORY to be returned? On small memory (no VM) systems, this can happen. /be
yes please
Er, I fear I don't understand. Timeless, yes please what? Neil, do you think of something like that: rv = prefs->CopyCharPref(pref.get(), machineName); if (NS_FAILED(rv)) { *machineName = nsnull; prefs->CopyCharPref("mail.smtpserver.default.machine_name", machineName); } return NS_OK;
rv = prefs->CopyCharPref(pref.get(), machineName); if (NS_FAILED(rv)) { *machineName = nsnull; prefs->CopyCharPref("mail.smtpserver.default.machine_name", machineName); } return NS_OK; What is so hard to understand? (1) The above ignores the (single-use rv variable, in the excerpt) failure result, suppressing out-of-memory errors wrongly. (2) The second CopyCharPref's return value is completely ignored, also wrong. If you wish to cope with default or *unset* prefs, you have to test for a default or empty string value. A failure result is an error, which should be propagated. /be
So I've to propagate the rv to the calling function. Ok, easy to do. But in case of the first CopyCharPref(), the pref might not be set. So need to test for an error which is not out of memory, yes? rv = prefs->CopyCharPref(pref.get(), machineName); if (rv == NS_ERROR_UNEXPECTED) { *machineName = nsnull; rv = prefs->CopyCharPref("mail.smtpserver.default.machine_name", machineName); } return rv; Testing if machineName is empty after CopyCharPref() wouldn't bring anything cause that can have multiple reasons. And then? The calling code: rv = smtpServer->GetMachineName(getter_Copies(m_machineName)); if (NS_FAILED(rv)) return;
Looks good -- PREF_ERROR is returned by PREF_CopyCharPref for no such pref name, and it is mapped to NS_ERROR_UNEXPECTED. /be
Attachment #127528 - Flags: review?(sspitzer)
Attached patch patch v5 (obsolete) (deleted) — Splinter Review
Ok, so next try. I hope, this patch addresses all issues and requests. This is a -puw patch, -pu on request.
Attachment #127528 - Attachment is obsolete: true
> nsSmtpServer::GetMachineName(char * *machineName) please use: nsSmtpServer::GetMachineName(char * *aMachineName) this is how we mark arguments so that they aren't confused with local variables. fwiw i filed Bug 213692 about the wrong allocator being used for the preferences code.
Assignee: mscott → ch.ey
So if I change this variables name, do you think the patch will survive the review?
Attached patch patch v6 (obsolete) (deleted) — Splinter Review
Changed argument name of GetMachineName()/SetMachineName(). This is a -puw patch, -pu on request.
Attachment #128228 - Attachment is obsolete: true
Attachment #129273 - Flags: superreview?(timeless)
Attachment #129273 - Flags: review?(brendan)
Comment on attachment 129273 [details] [diff] [review] patch v6 Please use module owner or peer for reviews. /be
Attachment #129273 - Flags: review?(brendan) → review?(sspitzer)
Comment on attachment 129273 [details] [diff] [review] patch v6 i'm not an sr (there's a list) neil is about to vacation i just got back and need to catch up on bugmail (so i'm not going to do a real review until i catch up - i.e. when i get *this* bugmail, about 9000bugmails from now) >Index: mailnews/compose/src/nsSmtpProtocol.cpp >+static NS_DEFINE_CID(kDNSServiceCID, NS_DNSSERVICE_CID); please use the contractid instead of the cid. >@@ -332,6 +337,26 @@ const char * nsSmtpProtocol::GetUserDoma >+ m_machineName.Assign("["); >+ m_machineName.Append(myIP); >+ m_machineName.Append("]"); I'd prefer a single string operation, but neil didn't like attachment 126920 [details] [diff] [review]'s code...
Attachment #129273 - Flags: superreview?(timeless)
Attachment #129273 - Flags: superreview?(sspitzer)
Attachment #129273 - Flags: review?(timeless)
Attachment #129273 - Flags: review?(sspitzer)
>+ m_machineName.Assign("["); >+ m_machineName.Append(myIP); >+ m_machineName.Append("]"); m_machineName = NS_LITERAL_CSTRING("[") + myIP + NS_LITERAL_CSTRING("]");
Timeless: Why should I use the contractid instead the cid? I find the cid more describing and it's used in sources too. Yes, const char kDNSService_CONTRACTID[] = "@mozilla.org/network/dns-service;1"; works, but why to use and how to know this? Darin: That doesn't work because myIP is char *. If there's a workaround, I don't know it.
CIDs identify implementations, contract-ids identify sets of interfaces. Use the latter unless you *really* mean to dependon a particular implementation. Here, you just want the DNS service, latest implementation. So contract-id is best. The raw char* can be wrapped in an nsDependentCString, no? I love our verbose string classes. /be
Attached patch patch v7 (obsolete) (deleted) — Splinter Review
Now using contract-id and new string assign to m_machineName according to Timeless and Darin (thanks Brendan for explanation). This is a -puw patch, -pu for checkin on request.
Attached patch patch v7 (obsolete) (deleted) — Splinter Review
Now using contract-id and new string assign to m_machineName according to Timeless and Darin (thanks Brendan for explanation). This is a -puw patch, -pu for checkin on request.
Attachment #129273 - Attachment is obsolete: true
Attachment #130407 - Attachment is obsolete: true
Attachment #130408 - Flags: superreview?(sspitzer)
Attachment #130408 - Flags: review?(timeless)
Attachment #129273 - Flags: superreview?(sspitzer)
Attachment #129273 - Flags: review?(timeless)
Comment on attachment 130408 [details] [diff] [review] patch v7 >+ // hope this works even on machines without _PR_GET_HOST_ADDR_AS_NAME darin didn't answer this question? :( >+ nsCOMPtr<nsIDNSService> dns = do_GetService(kDNSService_CONTRACTID, &rv); >+ if (NS_SUCCEEDED(rv)) >+ { >+ char *myIP; >+ rv = dns->GetMyIPAddress(&myIP); this leaks ^^, use an xpidlcstring instead then you don't need the nsdependentcstring below >+ if (NS_SUCCEEDED(rv)) >+ { >+ m_machineName = NS_LITERAL_CSTRING("[") + nsDependentCString(myIP) + NS_LITERAL_CSTRING("]"); >+ return m_machineName.get(); >+ } >+ } >+nsSmtpServer::GetMachineName(char * *aMachineName) >+ nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv)); >+ if (NS_FAILED(rv)) >+ return rv; for this failure you don't null out aMachineName >+ rv = prefs->CopyCharPref(pref.get(), aMachineName); >+ if (rv == NS_ERROR_UNEXPECTED) >+ { >+ *aMachineName = nsnull; >+ rv = prefs->CopyCharPref("mail.smtpserver.default.machine_name", aMachineName); for this failure you do. should you be consistent? >+ } >+ return rv; >+} The patch looks good
Attachment #130408 - Flags: review?(timeless) → review-
it looks like mac classic is the only environment that defines _PR_GET_HOST_ADDR_AS_NAME, so we shouldn't have to worry.
>it looks like mac classic is the only environment that defines >_PR_GET_HOST_ADDR_AS_NAME, so we shouldn't have to worry. Ok, so I don't. >>+ char *myIP; >>+ rv = dns->GetMyIPAddress(&myIP); >this leaks ^^, use an xpidlcstring instead then you don't need the >nsdependentcstring below Done, is now + nsXPIDLCString myIP; + rv = dns->GetMyIPAddress(getter_Copies(myIP)); >>+ if (rv == NS_ERROR_UNEXPECTED) >>+ { >>+ *aMachineName = nsnull; >>+ rv = prefs->CopyCharPref("mail.smtpserver.default.machine_name", aMachineName); >for this failure you do. should you be consistent? Yes, I should. I must admit I initially just copied nsSmtpServer::GetUsername that is inconsistent (and lacks to handle out-of-memory errors). Since GetMachineName's rv is checked and the calling function exited if wrong, I could omit nulling aMachineName out.
Attachment #130408 - Flags: superreview?(sspitzer)
Yeah, don't null out params on failure, in general (QueryInterface is a special case; there may be one or two others like it). /be
Attached patch patch v8 (obsolete) (deleted) — Splinter Review
Ok, so here's another revision of the patch addressing all issues. This is a -puw patch again, -pu for checkin on request.
Attachment #130408 - Attachment is obsolete: true
Attachment #130493 - Flags: review?(timeless)
Comment on attachment 130493 [details] [diff] [review] patch v8 some comments: 1) + nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv)); nsIPref is deprecated http://lxr.mozilla.org/mozilla/source/modules/libpref/public/nsIPref.idl#48 2) + if (rv == NS_ERROR_UNEXPECTED) I'd prefer: if (NS_FAILED(rv)) I'm not sure if prefs is guaranteed to return that exact error code. 3) why is there a setter? is there part of the patch I'm missing? shouldn't this be: "readonly attribute string machineName;"
looking upwards, brendan commented about the pref stuff, and steered you away from if (NS_FAILED(rv)), because it will mask the out of memory errors. yowsa, there's a lot of mailnews code that does it this way. (probably elsewhere too)
> 1) > > + nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv)); > > nsIPref is deprecated > > http://lxr.mozilla.org/mozilla/source/modules/libpref/public/nsIPref.idl#48 Uh, nsSmtpServer.cpp if full of it. But ok, I see. > 3) why is there a setter? is there part of the patch I'm missing? > > shouldn't this be: > > "readonly attribute string machineName;" See Neil's comment #42. But if a patch with UI (there should be one sooner or later) has the chance to go through, I can also do one. > looking upwards, brendan commented about the pref stuff, and steered you away > from if (NS_FAILED(rv)), because it will mask the out of memory errors. > > yowsa, there's a lot of mailnews code that does it this way. (probably > elsewhere too) There's already code for every call or if I did and I nevertheless got advice not to do this and that. Because of the lack of experience I don't know who's right (and maybe noone can say).
Comment on attachment 130493 [details] [diff] [review] patch v8 I suppose I should have flagged nsIPref, but if you look at the file you'd see that all but one of the pref users use nsIPref. I think we can leave that fix for a file cleanup. if you post a new patch and seth sr's it and the only change is the nsIPref then you can carry my r= forward, otherwise perhaps seth will just sr this and leave the file cleanup for later.
Attachment #130493 - Flags: superreview?(sspitzer)
Attachment #130493 - Flags: review?(timeless)
Attachment #130493 - Flags: review+
I'd like to do a planned cleanup with a new service function for the whole nsSmtpServer as a separate task but get this patch in now. So Seth, if nsIPref is your only caveat, shouldn't this be acceptable and let this patch have your sr?
BTW, will the patch fix Thuderbird as well? Lukasz
BTW no 2. Which version is the patch supposed to be implemented in? 1.5 I hope? You know, it is really nice to use and support Mozilla - when at least the basics work. Lukasz
Lukasz, 1.5 just branched, so this patch won't make it there - maybe 1.6a. Thunderbird - I think yes, the code base is still quite the same.
Attachment #130493 - Flags: superreview?(sspitzer)
Attached patch patch v9 (obsolete) (deleted) — Splinter Review
New try. This version now uses nsIPrefService/nsIPrefBranch instead of nsIPref. The new function GetPrefBranch() can also be used when migrating all other nsSmtpServer functions to nsIPrefBranch. The use of the removed GetMyIPAddress() has been replaced by combination of GetMyHostName() and Resolve(). It looks more complex but is the same the old function did in background. And I added provides a UI for the machine name.
Attachment #130493 - Attachment is obsolete: true
But unfortunately that's not how a pref branch should be used... nsCOMPtr<nsIPrefBranch> mKeyBranch; nsCOMPtr<nsIPrefBranch> mDefaultBranch; nsSmtpServer::SetKey(const char * aKey) { mKeyBranch = nsnull; mKey = aKey; return NS_OK; } nsresult /* NOT NS_IMETHODIMP */ nsSmtpServer::EnsurePrefBranch() { nsresult rv = NS_OK; if (!mDefaultBranch || !mKeyBranch) { nsCOMPtr<nsIPrefService> prefService = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv); if (NS_SUCCEEDED(rv) && !mDefaultBranch) { rv = prefService->GetBranch("mail.smtpserver.default.", getter_AddRefs(mDefaultBranch)); } if (NS_SUCCEEDED(rv) && !mKeyBranch) { nsAutoCString prefBranchName = "mail.smtpserver."; prefBranchName += mKey; prefBranchName += "."; rv = prefService->GetBranch(prefBranchName.get(), getterAddRefs(mKeyBranch)); } return rv; } nsresult nsSmtpServer::GetCharPref(char * pref, char * *result) { nsresult rv = EnsurePrefBranch(); if (NS_SUCCEEDED(rv)) { rv = keyBranch->GetCharPref(pref, result); if (rv == NS_ERROR_UNEXPECTED) rv = defaultBranch->GetCharPref(pref, result); } return rv; } NS_IMETHODIMP nsSmtpServer::GetMachineName(char * *aMachineName) { return GetCharPref("machine_name", aMachineName); }
Hm, I did know that the power lies within GetBranch()'s aPrefRoot parameter. But didn't see how to use it, resp. wanted to avoid tow nsIPrefBranch vars. So I did want I found in several other files. But ok, your code is nice though the hardwired get of the default value isn't what I'd do in this case. I'll use it.
Attached patch patch v10 (obsolete) (deleted) — Splinter Review
New version using EnsurePrefBranch() from comment #81. I didn't put get of user value and default value in one function because this combination is only used in GetMachineName().
Attachment #132789 - Attachment is obsolete: true
Attachment #132869 - Flags: review?(timeless)
Comment on attachment 132869 [details] [diff] [review] patch v10 the ui text isn't good enough to ship with a product, but i think a new bug could be filed to clean that up. if an sr objects to that then well, try again :).
Attachment #132869 - Flags: review?(timeless) → review+
Well, any suggestions for a better text are welcome.
Attachment #132869 - Flags: superreview?(bienvenu)
Comment on attachment 132869 [details] [diff] [review] patch v10 Offhand "Report machine name as:" Report is of course not ideal, but ...
The string I used in the patch is from KMail and accurate. Yes, it's quite long, but better this than confusing a user. Or what's the point of your criticism? Hm, what about "Identify this machine as:"
The string in your patch will confuse our target audience. (and it's too long) Comment 87's text seems ok to me.
Status: NEW → ASSIGNED
Maybe just "Send as hostname in HELO/EHLO:" Lukasz
> "Send as hostname in HELO/EHLO:" Ehm, no. I'd like this for myself. But nearly no user knows what HELO/EHLO is. And which hostname? The servers hostname or what? The problem is to describe what's meant understandable for all kind of users without more than four words ...
My point was, if somebody wants to use this option, he/she must know what is going on. On the other hand, if somebody knows what the problem is, he/she may be mislead by description like "Identify this machine as". The servers' error messages to erroneous FQDN in EHLO do not point to the solution. But when you find the reason for them, "Send as hostname in HELO/EHLO" is instantly obvious. Just my 2PLN. Lukasz
Yes, Lukasz, you can be right with this. Though I don't know what such error messages say if the server don't like the FQDN. As I wrote, I'd feel comfortable with this for myself. We could use it for alpha, wait and see. A help entry should also be added for this UI element.
FYI, I have come across the following types of answers (different servers from different SMTP providers): - Relaying denied - You are not authorized to use this server - Wrong account name/password - Some generic errors like "Server error", but I do not remember exactly the error texts - And sometimes Mozilla goes into endless loop, exchanging the same command sequence with server over and over again. Lukasz
Er, did the servers send these messages because of a wrong FQDN? And endless loop? That can happen with an old release if the password is wrong and saved. We've changed that in the newer releases.
Yes. I checked all my accounts with Outlook Express and was able to use SMTP with the same credentials as provided in Mozilla. I can not imagine other reason for these errors. In time, the responses from the same server sometimes changed (server upgrades?), but I cannot recollect single message like "Wrong host name" or "Wrong FQDN in EHLO" or like. Maybe the loop is no more in the current version. This bug plagues me from early Mozillas, I have not checked everything every release. And I stopped checking since I track this bug.
Sorry if some of this is re-hashing old comments do we really want a failure to get the machine name to abort the initialize process? + rv = smtpServer->GetMachineName(getter_Copies(m_machineName)); + if (NS_FAILED(rv)) + return; I think user-defined, not userdefined. Am I missing the point of having a default machine name? Are we ever going to set it? It's specific to each user's machine, isn't it? So no default is ever going to be meaningful. "mail.smtpserver.default.machine_name" I marginally prefer computer name over machine name - maybe because that's what Windows calls it and windows users would be our most confused users :-) + NS_ENSURE_ARG_POINTER(aMachineName); + + nsresult rv = EnsurePrefBranch(); + + if (NS_SUCCEEDED(rv)) { + if (aMachineName) + return mKeyBranch->SetCharPref("machine_name", aMachineName); + + mKeyBranch->ClearUserPref("machine_name"); + } + + return rv; since we've already checked if aMachineName is null (NS_ENSURE_ARG_POINTER(), this code doesn't look right. Do we want to remove NS_ENSURE_ARG_POINTER? Or should we have clients send in "" to clear the pref? In which case, we could remove the ClearUserPref...
> + rv = smtpServer->GetMachineName(getter_Copies(m_machineName)); > + if (NS_FAILED(rv)) > + return; > > do we really want a failure to get the machine name to abort the initialize > process? Since a rv!=0 should mean out-of-memory here, I think yes. > Am I missing the point of having a default machine name? Are we ever going to > set it? It's specific to each user's machine, isn't it? So no default is ever > going to be meaningful. Hm, a small net behind a NAT? Then you could set the default to a (from outside) resolvable name. > I think user-defined, not userdefined. > > "mail.smtpserver.default.machine_name" > > I marginally prefer computer name over machine name - maybe because that's > what Windows calls it and windows users would be our most confused users :-) Whatever you want - in both cases. > since we've already checked if aMachineName is null (NS_ENSURE_ARG_POINTER(), > this code doesn't look right. Do we want to remove NS_ENSURE_ARG_POINTER? Or > should we have clients send in "" to clear the pref? In which case, we could > remove the ClearUserPref... I'd like to have the pref cleared if aMachineName is empty. So what about this: + NS_ENSURE_ARG_POINTER(aMachineName); + + nsresult rv = EnsurePrefBranch(); + + if (NS_SUCCEEDED(rv)) { + if (*aMachineName) + return mKeyBranch->SetCharPref("machine_name", aMachineName); + + mKeyBranch->ClearUserPref("machine_name"); + } + + return rv; What do you think about the string in the UI? Which would you prefer?
Or even better without NS_ENSURE_ARG_POINTER(aMachineName); but if (aMachineName && *aMachineName)
Attachment #132869 - Flags: superreview?(bienvenu)
Attached patch patch v11 (obsolete) (deleted) — Splinter Review
New version, new luck to get it in. This version now uses computer_name instead of machine_name, the UI says "Identify this computer as:" and SetComputerName() has been changed according to comment #98.
Attachment #132869 - Attachment is obsolete: true
Attachment #137764 - Flags: review?(bienvenu)
It looks OK, but I don't see the need for the whole default pref branch thing. Doesn't the pref code do that for you?
No, if the pref doesn't exist, GetCharPref() return failure and a null string. Maybe there's a way to instruct the pref service code to take the default if the pref doesn't exist. But neither do I nor seems Neil to know it (as he posted the code in comment #81).
since we're defaulting the name to the empty string anyway, I still don't see the need for complicating the code this way. I'd really rather not add more code for this feature...
I'm not sure what you're talking about. The default code in EnsurePrefBranch() are two lines: + if (NS_SUCCEEDED(rv) && !mDefaultBranch) { + rv = prefService->GetBranch("mail.smtpserver.default.", getter_AddRefs(mDefaultBranch)); The code in GetComputerName() are atmost two lines: + if (rv == NS_ERROR_UNEXPECTED) + rv = mDefaultBranch->GetCharPref("computer_name", aComputerName); And please see the possible reason in my comment #97. An admin could change the default in an installation script. And lastly we'll need the default code at least in EnsurePrefBranch() at the latest when moving away from nsIPref. But if you want I can remove these lines and handle NULL for m_computerName in GetUserDomain().
I wonder how it's possible Mozilla was ever created when it takes months to settle such a small ****... Lukasz, with useless Mozilla Mail for years now...
(In reply to comment #25) > that is very frequent to find in use with spammers (hope they don't see this > 'cause I want them to keep ignoring the RFC)....some spammer examples... > > HELO dYu2sW37 <-- spammers love random nonsense > HELO [127.0.0.1] <-- remote MTA can't be on my loopback > HELO acme.com <-- when MY own site is "acme.com" > HELO 66.55.44.33 <-- No [] syntax Sorry, I don't understand. Are You think what "HELO 66.55.44.33" isn'f RFC-proper ? I think You are wrong. Please see RFC2821, 4.1.3 Address Literals: IPv4-address-literal = Snum 3("." Snum) "[]" isn't present in specifically. "HELO 66.55.44.33" is right when 66.55.44.33 unresolved.
> IPv4-address-literal = Snum 3("." Snum) more sorry, i miss 4.1.2 Command Argument Syntax :-( thanks for Briggs. I'm wrong.
Sergey, glad you caught your mistake. It is a very easy one to make as the BNF is spread across several sections (I myself had to reread the RFC carefully several times). Fortunately for antispam purposes though it seems that many spammers also tend to make your same mistake of missing the required brackets, while most legitimate MTA's/MUA's seem to have gotten it correct. That's why I mentioned it as a possible indicator of spam, and hence why Mozilla should do it correctly if it must resort to IP literals.
Changing subject (adding EHLO) to make it easier to find.
Summary: "HELO domain of sending mail account" should be "HELO host.domain of machine running Mozilla" → "HELO/EHLO domain of sending mail account" should be "HELO/EHLO host.domain of machine running Mozilla"
*** Bug 238890 has been marked as a duplicate of this bug. ***
*** Bug 239104 has been marked as a duplicate of this bug. ***
(In reply to comment #111) > *** Bug 239104 has been marked as a duplicate of this bug. *** I don't understand why I had not found this bug :/ Sorry for the duplicate. Well, is there some kind of status information and when this bug will be fixed / the patches are applied?
Never. Plain and simple. Especially considering this bug is 3 years old. After some deep investigation I discovered, that this bug is maintained by MozillaAdversariesTeam (R)(TM) to make sure, that anybody can point out this bug as a proof Mozilla is as buggy as hell. And that Mozilla bug patching is as defined process as weather changes. Follow my steps and buy TheBat for your e-mail needs, 'cos this bug is gonna to stay. Lukasz
Flags: blocking1.8a+
Flags: blocking1.7+
Flags: blocking1.8a+
Flags: blocking1.7+
I'll put in a patch for this, when I get a moment
Attached patch fix w/o default value for machine name (obsolete) (deleted) — Splinter Review
this is exactly the same as Christian's patch, but w/o the default value stuff. If later on it becomes clear that a default value is useful, we should do it in such a way that we use common routines for all the pref getting/setting, in the interest of cutting down on bloat. We should clean up this file in general to share more of the boiler-plate pref code (and use nsIPrefBranch throughout).
Comment on attachment 145095 [details] [diff] [review] fix w/o default value for machine name can you omit the first hunk of compose/src/nsSmtpServer.cpp (@@ -85,6 +85,7 @@)
Attached patch same fix again w/o spurious extra line (obsolete) (deleted) — Splinter Review
Attachment #145095 - Attachment is obsolete: true
Attachment #137764 - Flags: review?(bienvenu)
That patch would be ok for me, I can live with it.
Attachment #145096 - Flags: superreview?(mscott)
I'm strongly against adding this kind of UI for the average user to fill out. They aren't (and shouldn't) have to fill out a computer name or IP address when setting up an SMTP server. I don't see any such UI in outlook or outlook express. I'm pretty sure eudora doesn't either. If anything, we should have a low level necko call where we can ask what the IP address or computer name is for the users's machine. I'm sure there is a windows API call to do just this, if someone knows of it please speak up here. What about GTK2 on linux?
Christian, what was wrong with your original patch (http://bugzilla.mozilla.org/attachment.cgi?id=126113&action=view) which gets the IP address from the DNS server and uses that with the HELO command? did it just need some formatting tweaks to adhere to the RFC? That seems like the correct approach to me. We should not have UI for this feature, it is way to advanced. We should try to do the right thing without the user having to fill in this information in a geeky advanced UI panel. It isn't something a normal user should be filling in.
Scott, the patch implements getting the hostname and resolve its address. The pref with UI for a computer name to present is only additionally and I implemented it after some requests. It's not mandatory to fill something in. For me nothing was wrong with my original patch. And it was RFC compliant. Read all the comments what people (reviewers among them) didn't like. My patch is around for nearly a year now and people still come up with "new" ideas and requests to change it. I swear that if we reduce it to simply "EHLO [IP]" people come up with "but in KMail I can enter a name" stuff again.
I think it would be time to fix this bug because it makes the creators of the best mail client(tm) look like they wouldn't know how to use SMTP commands... please :)
The default should be to use the ipaddress that was used to open the connection to the SMTP-server, not GetMyIPAddress or something similar. That doesn't work on multi-homed hosts or hosts with many ipaddresses (f.i. with dual IPv4/IPv6 network stacks). Just open the connection first, then use getsockname(). That ipaddress should be turned into a FQDN with a DNS-lookup. If this is the default, then most users would report the correct hostname in their SMTP-headers, and this would help in the war against SPAM (SMTP-servers should insists on that !). Now, Mozilla is breaking all the rules, we send only the domainname, not even a FQDN. If people need to override that (f.i. when they're behind a NAT that doesn't translates), they can do that as a hardcoded preference, but I wouldn't encourage that, so I don't think a GUI is needed. BTW : unless you have a static (and public) ipaddress, you will always provide a *fake* name (in a preference, GUI, or from gethostname), because the ipaddress/name will be different everytime. If your SMTP-server really wants to check the header, then you still can't send a mail. So we still need this 'automatic' setting!
It looks everyone knows how to do it but dump me is stumpling around for almost a year nw. Why don't you do it yourself? I've to dig in how to use getsockname() (Passing socket id from SMTP protocol handler? That's fun.), how to distinguish v4 from v6 addresses a.s.o.
Comment for late-comers to this party: Consider an ISP whose SMTP server enforces the following rules on HELO/EHLO: a) if it contains a DNS name, then a lookup on that DNS name must return an IP address matching the IP address returned by getpeername() on that connection's socket, or b) if it contains an IP address, that IP address must match the one returned by getpeername() on that connection's socket. Now, imagine that the mozilla client sending the message is behind an in-home NAT router, so that the IP adddress seen on that host's local interfaces, and /or returned by getsockname() are NOT the address seen by the ISP's SMTP server when it calls getpeername.
(In reply to comment #125) > I've to dig in how to use getsockname() (Passing socket id from SMTP protocol > handler? That's fun.), how to distinguish v4 from v6 addresses a.s.o. You want this here, I believe: http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsISocketTransport.idl#64 this assumes that the mailnews code there uses nsISocketTransport...
Attached patch possible fix (obsolete) (deleted) — Splinter Review
Here's a different strategy after talking to Jo and Christian Eyrich. It removes the UI and just does the following: 1) Get the IP address from the current socket transport 2) Do a DNS lookup on this IP address (does this meet Jo's reverse DNS lookup requirement?) 3) Use the resulting IP address from the DNS lookup if it succeeded, otherwise fall back to the IP address we got for the current socket transport. Do we have to do any special casing for IPv6? I would think the call to PR_NetAddrToString takes care of all that.
fwiw my mail server runs on 127.0.0.1 (actually it's an ssh forward). i guess that means i'll always resolve to localhost.
proposal of comment 128 insufficient for users behind NAT routers.
Nelson, we're almost out of time for 1.7, and I would rather have a patch that works for most configs, and get a NAT bug fixed later. What does the client's DNS think of the untranslated IP address returned by getsockname? If reverse-lookup gives back a name other than one that maps to the NAT'd address on the mail server end, what can be done? How can this situation be detected on the mail client host? /be
(In reply to comment #126) > Comment for late-comers to this party: > > Consider an ISP whose SMTP server enforces the following rules on HELO/EHLO: > > a) if it contains a DNS name, then a lookup on that DNS name must return an > IP address matching the IP address returned by getpeername() on that connection's > socket, or > > b) if it contains an IP address, that IP address must match the one returned > by getpeername() on that connection's socket. > > Now, imagine that the mozilla client sending the message is behind an in-home > NAT router, so that the IP adddress seen on that host's local interfaces, and > /or returned by getsockname() are NOT the address seen by the ISP's SMTP > server when it calls getpeername. > Nelson, I program access servers and border gateways (= comparable to NAT) for a living. The problem is that when you have a SMTP-server that wants to check the EHLO header (or a service like Spamcop that wants to check the SMTP-header later), then you want to provide accurate data. But that's actually impossible if you have NAT, so we still need to provide the data manually. And even if you don't have NAT, you still have to provide the *real* ipaddress and name, which will probably change every time if you log on, unless you have a static ipaddress, or use dynamic DNS (in which case you have to provide th ename manually, the reverse DNS-llokup won't find it). There's only 1 solution to this problem, and that is to use a NAT that will transparently change the EHLO header or (better) a NAT that implements a SMTP server on its own (or a transparent proxy, so that it can accept your headers, and add its own. But that's very rare, at least for the NAT's that the public use. I'm using a transparent proxy in my own software. Scott, we will always have people that will have trouble with pedantic SMTP-servers (they can't use any version of Mozilla or Netscape at the moment). We can use the current proposal as a default, and we can provide a preference to override that for people who need it. Attachment 145096 [details] [diff] wasn't so bad after all. We only need to change the default, becuase that wasn't correct (we don't call GetMyHostName anymore, but use getsockname). I would vote not to provide a GUI for it, though.
+ aResult.Assign(NS_LITERAL_CSTRING("[") + resolvedIP + NS_LITERAL_CSTRING("]")); hmm. shouldn't you only use [] for IP addresses, and leave them out for domain names?
Er, no Scott. Firstly socketTransport->GetAddress() gets the address of the remote end, not ours. And secondly, EHLO with IPv6 must no use "[IP]" but "IPv6:IP". I can't test IPv6 nets so I don't know if the system functions already return the prefix or if we have to put it there. Nelson, I know about the NAT problem, see comment #22. But 1. everything's better than the current obviously wrong one and 2. give us a hint how to get our outside IP and we'll implement it.
> Er, no Scott. Firstly socketTransport->GetAddress() gets the address of the > remote end, not ours. Argh, this is my fault for misleading mscott. We were looking at nsServerSocket::GetAddress, which does calls PR_GetSockName (which calls getsockname, not getpeername). We should have been looking at nsSocketTransport::GetAddress. Two methods with the same name and opposite function. Darin, save us! Everyone stay cool -- this bug will be fixed for 1.7 or my head will explode. /be
Re: comment 126 An ISP that does this is not implementing SMTP properly. RFC 2821 sec. 4.1.4 para. 6 says: An SMTP server MAY verify that the domain name parameter in the EHLO command actually corresponds to the IP address of the client. However, the server MUST NOT refuse to accept a message for this reason if the verification fails: the information about verification failure is for logging and tracing only. Unless you know of an ISP that actually does this, it's a waste of time to try to work around it.
Re: comment 131 Brendan, I agree *completely* that this bug should be fixed in 1.7, even if the fix doesn't work for some users behind NAT routers. A 95% fix is better than a 0% fix. IMO, reverse DNS should not be attempted for addresses in the "private address" space defined in RFC 1918, which is what most systems behind a NAT router will find. mozilla can detect the NAT problem by checking the result of getsockname against those 3 subnets and treating them differently. Re: comment 132 and comment 132=4 I disagree with the remark that "There's only 1 solution to this problem,". A large portion of the NAT-using (or NAT-victim :) population would be satisfied if they could have a way (perhaps via a preference) to specify the value that should be sent in the HELO/EHLO request. Remember, we're trying to find solutions that work for large subsets of the user base. We're NOT trying to hold off all solutions until the perfect one for all users can be found. (Right, Brendan?) Re: comment 126, There are broadband ISPs that are doing this now as an anti-spam measure. Their attitude is that stopping spam is much more important than RFC-compliance for this issue. In some cases, the ISPs see it as a necessary measure to keep themselves out of various RBLs. Given that there *IS* a relatively simple solution that would enable mozilla users to workaround this problem without either (a) finding new ISPs (they may have only one choice for broadband ISP), or (b) replacing their NAT boxes with ones that rewrite outgoing HELO/EHLO (do any exist?), I think mozilla should implement such a solution. But I wouldn't hold up 1.7 for it.
Attached patch another attempt (obsolete) (deleted) — Splinter Review
After talking to Darin and Brendan, this patch creates a new method on nsISocketTransport: [noscript] PRNetAddr getSelfAddr(); which returns the value of PR_GetSockName on the open smtp socket. For clarity, I also renamed getAddress to be getPeerAddr to avoid confusion. Still to do: IPv6 formatting of the string
Attachment #137764 - Attachment is obsolete: true
Attachment #145096 - Attachment is obsolete: true
Attachment #145727 - Attachment is obsolete: true
Comment on attachment 145845 [details] [diff] [review] another attempt >+++ netwerk/base/public/nsISocketTransport.idl 11 Apr 2004 03:25:40 -0000 >@@ -65,7 +65,13 @@ > * Returns the IP address for the underlying socket connection. This > * attribute is only defined once a connection has been established. Existing comment, I realize, but you clone it below. Nit: "only defined" should be "defined only". Fix that and then the question is, what's the return value if there is no established connection? >+NS_IMETHODIMP >+nsSocketTransport::GetSelfAddr(PRNetAddr *addr) >+{ >+ nsresult rv = NS_OK; >+ if (mFDconnected) >+ PR_GetSockName(mFD, addr); >+ else >+ rv = NS_ERROR_FAILURE; // trying to get the address before we have reached the connected state? >+ >+ return rv; > } How about early return for errors, check PR_GetSockName's r.v., and no rv local needed: { if (!mFDconnected || PR_GetSockName(mFD, addr) == PR_FAILURE) return NS_ERROR_FAILURE; return NS_OK; } Also, uniform four-space indentation. /be
Great, Scott. That works though I couldn't test it on a multihomed machine. It looks we've no flag or so to see what version the IP is. So would this reasonable (a port and therefore a colon shouldn't be present in the IP, right)? if (strchr(resolvedIP, ':')) format IPv6 else format IPv4 I'd also like to modify the comment to something like +void nsSmtpProtocol::GetUserDomainName(nsACString& aResult) { + // should return the interface ip of the SMTP connection Also I'm not sure if we should reverse resolve the interface IP to a name at all. The resulting name may be resolveable locally but not from outside (NAT). While the IP also may not have any meaning outside, it should be considered as ok by most servers. But I think the probability to generate problems is higher when sending out an unresolveable domain name.
Attachment #145096 - Flags: superreview?(mscott)
Attached patch updated patch (obsolete) (deleted) — Splinter Review
This updated patch addresses Brendan's review comments and changes the comment in nsSmtpProtocol per Christian. Christian, I'm not sure about the DNS look up on our IP address either. I can't see what that is doing for us. In my limited testing scenarios (at home with a router and at work) the DNS lookup of my IP address is ALWAYS giving me back the same IP address. I don't think we'll ever get back a 'name' instead of an IP address here because the PRNetAddr object returned from the DNS request only holds IPv4 or IPv6 strings. So it would never give us a name anyway. My preference is to take that part out, and coded to handle IPv6 and get this into 1.7.
Attachment #145845 - Attachment is obsolete: true
Attached patch another attempt v3 (obsolete) (deleted) — Splinter Review
In hope to push things forward, this is a modified version of Scott's last patch. It adds generation of EHLO argument for IPv6 addresses (see my comment #140) and removes the resolve as its benefit is disputable.
Attachment #146107 - Attachment is obsolete: true
Comment on attachment 146701 [details] [diff] [review] another attempt v3 No no, stupid me. That doesn't even compile.
Attachment #146701 - Attachment is obsolete: true
Attached patch another attempt v4 (obsolete) (deleted) — Splinter Review
Next try, see comment #142 but tested this time.
Christian, thanks for finishing off this patch. I just applied your patch but I can no longer send mail with it. At least for me, we never caclulate a name to pass to the EHLO command anymore. the log looks like: 0[294280]: SMTP Send: EHLO 0[294280]: SMTP entering state: 0 0[294280]: SMTP Response: 501 Syntax: EHLO hostname
(In reply to comment #140) [...] > Also I'm not sure if we should reverse resolve the interface IP to a name at > all. The resulting name may be resolveable locally but not from outside (NAT). You shouldn't care about NAT in my opinion: A NAT user normally has an SMTP Server to send mail out, regardless of what helo/ehlo header is set. Either the client authenticates through an AUTH command with valid credentials or it is allowed to send from his natted IP. In both cases an SMTP server rejecting a relay due to an invalid helo/ehlo should be regarded as badly configured. If a NAT user doesn't authenticate in no way and wants to send non-local mail, the server should deny in any case, unless it's an open relay (which would not do header check most probably). Sending local mail without authentication could be denied, because of a malformed helo/ehlo header. This case may happen rarely, since a regular eMail user gets any form of valid authentication from his/her ISP. Thus for NAT users the contents of the helo/ehlo header is not important. Security concerns like the one, that internal network information is presented to the outside world, are not related to the MUA. If security is an issue, one should use a firewall, doing Message-ID Masquerade and hiding internal addresses, etc. For non-NAT communication the proposed way of reverse looking up the IP Address and using IP litterals in square brackets, is a perfect solution. The user configurable option is pure luxury, provided my arguments are coherent.
Comment on attachment 146908 [details] [diff] [review] another attempt v4 > At least for me, we never caclulate a name to pass to the EHLO command anymore. What the heck? Sorry for muddling this up - again. I did apply your "another attempt" but only the nsSmtpProtocol.cpp part from the "updated patch". So I missed that you changed GetSelfAddr() in the latter and it worked for me. The patch I uploaded is the "updated patch" with the part for nsSmtpProtocol.cpp replaced. To cut a long story short nsSocketTransport::GetSelfAddr(PRNetAddr *addr) { if (mFDconnected || PR_GetSockName(mFD, addr) == PR_FAILURE) return NS_ERROR_NOT_AVAILABLE; else return NS_OK; } always returns error and therefore no IP address is used. I guess you meant something like if (mFDconnected && PR_GetSockName(mFD, addr) == PR_SUCCESS) return NS_OK; else return NS_ERROR_NOT_AVAILABLE; BTW, in PR_NetAddrToString() I saw the address tested for IPv6 with PR_AF_INET6 == addr->raw.family Maybe we also can use this instead strchr(ipAddressString, ':') If iaddr->raw.family is PR_AF_INET6, does ipAddressString always contain a IPv6 address (and vice versa)? Or might an address like 0:0:0:0:0:FFFF:129.144.52.38 become 129.144.52.38 in PR_NetAddrToString?
actually I meant to say if (!mFDConnected || ....) My original tree with this patch has a !, but the patch I attached to this bug doesn't. Zoinks. Good catch.
> if (!mFDConnected || ....) Yes, that's the other alternative. I had a blackout (yes, another one), thinking in this case the second condition would get evaluated if mFDConnected is false. And I still prefer the "positive assumption" I posted, but that's a personal preference. Can someone (Darin?) can anything about the question how to evaluate the address for IPv6?
Comment on attachment 146908 [details] [diff] [review] another attempt v4 We'll need Darin to review the network changes anyway. Darin can you also answer Christian's IPv6 question if you know the answer :) I'll sr after Darin okay's the changes we made to nsISocketTransport.
Attachment #146908 - Flags: superreview?(mscott)
Attachment #146908 - Flags: review?(darin)
fixed on the 0.6 tbird branch
Comment on attachment 146908 [details] [diff] [review] another attempt v4 else after return in GetSelfAddr, wahhhh! /be
please change the IID of netwerk/base/public/nsISocketTransport.idl when changing it, so users of it won't crash when this interface changes.
Comment on attachment 146908 [details] [diff] [review] another attempt v4 >Index: mailnews/compose/src/nsSmtpProtocol.cpp >+const char kDNSService_CONTRACTID[] = "@mozilla.org/network/dns-service;1"; nit: nsNetCID.h defines NS_DNSSERVICE_CONTRACTID, so perhaps you want to use that instead? >+void nsSmtpProtocol::GetUserDomainName(nsACString& aResult) ... >+ nsresult rv = NS_OK; > >+ nsCOMPtr <nsIDNSService> dns = do_GetService(kDNSService_CONTRACTID, &rv); nit: you don't need to initialize |rv| here. it will be assigned a value by do_GetService. >Index: netwerk/base/public/nsISocketTransport.idl > * Returns the IP address for the underlying socket connection. This >+ * attribute is defined only once a connection has been established. nit: how about this instead: "Returns the IP address of the socket connection peer." >+ * Returns the IP address on the initiating end. This >+ * attribute is defined only once a connection has been established. nit: looks like attribute could be moved up to the preceding line. >Index: netwerk/base/src/nsSocketTransport2.cpp >+NS_IMETHODIMP >+nsSocketTransport::GetSelfAddr(PRNetAddr *addr) >+{ >+ if (mFDconnected || PR_GetSockName(mFD, addr) == PR_FAILURE) >+ return NS_ERROR_NOT_AVAILABLE; // trying to get the address before we have reached the connected state? >+ else >+ return NS_OK; > } this code is a problem. mFD has to be protected by mLock, otherwise you could crash if mFD is closed while this thread is inside PR_GetSockName. here's a better implementation: NS_IMETHODIMP nsSocketTransport::GetSelfAddr(PRNetAddr *addr) { // we must not call any PR methods on our file descriptor // while holding mLock since those methods might re-enter // socket transport code. PRFileDesc *fd; { nsAutoLock lock(mLock); fd = GetFD_Locked(); } if (!fd) return NS_ERROR_NOT_CONNECTED; nsresult rv = (PR_GetSockName(fd, addr) == PR_SUCCESS) ? NS_OK : NS_ERROR_FAILURE; { nsAutoLock lock(mLock); ReleaseFD_Locked(fd); } return rv; } NOTE: I have not tested this function, but I believe that it should work :)
Attachment #146908 - Flags: review?(darin) → review-
Comment on attachment 146908 [details] [diff] [review] another attempt v4 >Index: mailnews/compose/src/nsSmtpProtocol.cpp >+ // turn it into a string >+ char ipAddressString[64]; >+ if (PR_NetAddrToString(&iaddr, ipAddressString, sizeof(ipAddressString)) == PR_SUCCESS) >+ { >+ if (strchr(ipAddressString, ':')) // IPv6 style address? >+ aResult.Assign(NS_LITERAL_CSTRING("[IPv6:")); >+ else >+ aResult.Assign(NS_LITERAL_CSTRING("[")); >+ >+ aResult.Append(nsDependentCString(ipAddressString) + NS_LITERAL_CSTRING("]")); >+ } >+ } since you have a PRNetAddr, i recommend using the |af| field to determine the address family instead of searching for a ':' in the address string. - if (strchr(ipAddressString, ':')) // IPv6 style address? + if (iaddr.raw.family == PR_AF_INET6) // IPv6 style address? FYI, in the past Necko stored IPv4 addresses as IPv4-mapped IPv6 address. since the switch to using getaddrinfo under the hood, we now no longer use IPv4-mapped IPv6 addresses, so testing the address family should give you correct results. you might want to add this assertion: NS_ASSERTION(PR_IsNetAddrType(&iaddr, PR_IpAddrV4Mapped) == PR_FALSE, "unexpected IPv4-mapped IPv6 address");
Attached patch another attempt 5 (obsolete) (deleted) — Splinter Review
Ok, next round. Thank you for looking through this and answering my question. This patch incorporates everything from comment #153 to #155. As far as I was able to test, GetSelfAddr() does what it should - but Scott's implementation did too since the difference only shows in the worst case scenario if I understood correctly.
Attachment #146908 - Attachment is obsolete: true
Comment on attachment 147184 [details] [diff] [review] another attempt 5 We can get rid of this altogether can't we since we don't use it? +#include "nsIDNSService.h" + nsCOMPtr <nsIDNSService> dns = do_GetService(NS_DNSSERVICE_CONTRACTID, &rv); + if (NS_SUCCEEDED(rv))
Comment on attachment 147184 [details] [diff] [review] another attempt 5 Oh, er, yes of course. Does everything else now ok for all of you?
Attachment #146908 - Flags: superreview?(mscott)
yeah everything else looks good to me and you've fixed all of Darin's comments. If you can post one last patch that removes the call to the DNS service, I'll put an sr on that patch and I presume Darin will be okay putting an r on it as well. Then we can finally close this one out. Thanks Christian!
Attached patch another attempt 6 (deleted) — Splinter Review
So, here it is.
Attachment #147184 - Attachment is obsolete: true
Comment on attachment 147315 [details] [diff] [review] another attempt 6 Thanks for making these changes Christian.
Attachment #147315 - Flags: superreview+
Attachment #147315 - Flags: review?(darin)
Comment on attachment 147315 [details] [diff] [review] another attempt 6 >+ /** >+ * Returns the IP address on the initiating end. This attribute >+ * is defined only once a connection has been established. >+ */ >+ [noscript] PRNetAddr getSelfAddr(); I think this comment should say "Returns the IP address of the initiating end." That is, just change "on" to "of" r=darin with that minor change.
Attachment #147315 - Flags: review?(darin) → review+
Attachment #147315 - Flags: approval1.7?
fixed on the trunk
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment on attachment 147315 [details] [diff] [review] another attempt 6 a=chofmann for 1.7
Attachment #147315 - Flags: approval1.7? → approval1.7+
fixed on the tbird 0.6 branch , mozilla 1.7 branch and the trunk
Keywords: fixed1.7
Product: MailNews → Core
Sending the IP address for EHLO is causing my mail to be flagged as SPAM by servers using Spam Assassin. This is bad. I filed a bug against sending IP addresses for EHLO commands since it is in violation of the relevant RFC's AND it is causing legitimate mail to be lost. See bug 279525.
*** Bug 238275 has been marked as a duplicate of this bug. ***
Product: Core → MailNews Core
(In reply to Jerry Baker from comment #166) > Sending the IP address for EHLO is causing my mail to be flagged as SPAM by > servers using Spam Assassin. This is bad. I filed a bug against sending IP > addresses for EHLO commands since it is in violation of the relevant RFC's > AND > it is causing legitimate mail to be lost. See bug 279525. Then your Spam against needs to have its scoring corrected. You're asking to change the behavior of Thunderbird, which does the right thing, because your Spam filters are broken. Two wrongs don't make a right.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: