Closed Bug 68877 Opened 24 years ago Closed 20 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)

Product:
MailNews Core
Component:
Networking: SMTP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: briggs, Assigned: ch.ey)

References

Details

(Keywords: fixed1.7)

Attachments

(1 file, 20 obsolete files)

another attempt 6
8.08 KB, 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) — — 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) — — 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) — — 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) — — 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) — — 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) — — 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) — — 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) — — 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) — — 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) — — 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) — — 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) — — 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
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 @@)
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) — — 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) — — 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) — — 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) — — 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) — — 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) — — 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!
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: 20 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

Creator:
Created:
Updated:
Size: