44995 - [fix in hand] SOCKS - mailnews implementation

Status: VERIFIED FIXED

(MailNews Core :: Networking, defect, P1)

Description

[justin]

The various mailnews protocols (nntp, smtp, pop, and imap) do not use the SOCKS 
proxy. The following patch fixes this.

Comment 1

[justin]

Attachment: A patch to add SOCKS support to the mailnews protocols

Comment 2

[Asa Dotzler [:asa]]

setting status to new and adding patch keyword.

Comment 3

[Scott MacGregor]

we should check this in for beta3. Triaging. Thanks for contributing these
changes for us Justin. 

Comment 4

[Scott MacGregor]

I haven't had time to check this in for Justin because I've been working on the 
beta2 branch. Justin, look for this fix to go in this week. thakns again for the 
patch. 

Comment 5

[selmer (gone)]

We won't hold our release for this, but if you can get it in and working on 
time, go for it.  nsbeta3-

Comment 6

[scottputterman]

sorry for the extra email. Removing mail2 keyword.

Comment 7

[[e-mail]]

Can someone give a code/engineering level update as to the status of this patch?

Also, I'd like some clairfication if the the code supports v4, v5, or both?

Comment 8

[kmurray1115]

Adding mail3 keyword for inclusion or consideration in next release.

Comment 9

[[e-mail]]

Comments from someone at the CPD off-site said that there is no SOCKS V4 support
in the networking code of Mozilla.

SOCKS V4 is not a subset of SOCKS V5, so I think we need to drop support of
SOCKS V4 and support only V5.

Comment 10

[Daniel.Lemire]

Is this supposed to be fixed in Mozilla 0.6 ??? Because it still won't work for
me!

Comment 11

[scottputterman]

marking nsbeta1-. Though, if the patch works, we should get this in.

Comment 12

[[e-mail]]

If this worked in Communicator, I don't see how this could be dropped without 
some justification.

Comment 13

[benc]

nominating for mozilla0.8

If it's a old Communicator 4.x feature, I'd prefer to see it retargeted to some 
milestone w/ a reason. 

This bug needs to have the milestone and keywords cleaned up too...

Comment 14

[[e-mail]]

updated Summary

Comment 15

[Dave Royal]

Is this bug still outstanding? Was the patch ever included?
I can't send mail at all - never have been able to with Mozilla.
I get 'Sending of mail failed' - but whether it's this or bug 19482 (before I
was using a SOCKS/5 proxy) I don't know.
The browser has always worked fine with SOCKS.

Comment 16

[Bradley Baetz (:bbaetz)]

We want to move the socks stuff away from having to be implemented by each
client. see bug 89500, which will contain some real suggestions soon.

Comment 17

[Bradley Baetz (:bbaetz)]

I'm probably going to convert one of the mailnews protocols to socks as part of
89500, just to convince myself that my API is flexible enough (mailnews doesn't
create a new channel from the ioservice, I believe, so I need to check that I
can deal with that. I should, but API review on bug 89500 is welcome, although I
already know of a few changes to make)

Depending on some API changes I'm considering, the method signature to
CreateTransport may change, so that this will all Just Work without any extra
effort on the part of mailnews (I'll take care of it in my patch). If not, then
there will be very few changes needed.

Stay Tuned!

Comment 18

[Joerg Sievers]

A binary patch would be great. Is there a chance to get it?

Comment 19

[benc]

If we are going to turn this on, and there is going to be no pref (there was no
pref in Comm4), then please make sure mailnews QA and eng knows what you are doing.

I did not appreciate PAC debugging getting spanked, I would not want to wish it
on anyone else.

Comment 20

[Steve Meredith (gone)]

I don't think a UI change is needed for this. If SOCKS is specified in the
Proxies prefs page, then all protocols should be passed through it, except those
specified in the "No Proxies For:" pref.

Comment 21

[Bradley Baetz (:bbaetz)]

No patch yet. We can just relnote it, since I believe this is what 4.x did, and
if you have the no proxy for stuff set, it won't trigger at all for internal
servers.

I mentioned it to sspitzer last night on IRC, and his response was: "nice!" It
will be mentioned on .netlib, since at least some APIs will be changed.

Comment 22

[benc]

I'm not saying there should be a pref, I'm saying "assuming there is no pref,
(because if there was, we'd have to have another long conversation...) then the
next step is to make sure mailnews understands the behavior...) They are going
to have to qa this feature for all network connections you wrap.

Comment 23

[Bradley Baetz (:bbaetz)]

The fix for bug 89500 went onto the trunk last night. I fixed mail so that it
compiled, but didn't update it to support using socks. Fixing this should be a
couple of hours work for someone - the patches are reasonably streightforward.
+helpwanted. I may take this after I get net access at home.

Comment 24

[Hixie (not reading bugmail)]

I cannot read my mail with Mozilla at the moment because of the lack of SOCKS5
support for IMAP. This is therefore preventing my use of dogfood. Since this is
a major loss of functionality, changing severity as per bugzilla guidelines.

Comment 25

[Bradley Baetz (:bbaetz)]

OK, I have a patch. Initially I thought that I'd have to pass the proxyInfo all
the way from necko, but it turns out that thats not needed.

However: pop and smtp don't work. Thats because they create urls, but don't
register themselves as protocol handlers. pop uses pop3 as a scheme, but pop as
the registration, and smtp uses smtp as the scheme, but mailto for the registration.

Without this, we don't find the protocol handler, so we don't realise that its a
proxyable protocol.

I know that this broke things a few months ago for pop when dougt's port
blocking landed - is there a reason for this situation? The other option is a
bunch of special casing, and I really really really don't want that.

taking. mscott, can you please comment on what I said, and r/sr the patch? It
does work for nntp, and imap.

Comment 26

[Bradley Baetz (:bbaetz)]

Attachment: patch

Comment 27

[Darin Fisher]

Comment on attachment 52626 [details] [diff] [review]
patch

>Index: mailnews/base/util/nsMsgProtocol.cpp

>+  nsCOMPtr<nsIProxyInfo> pi;
>+  rv = pps->ExamineForProxy(aURL, getter_AddRefs(pi));
>+  if (NS_FAILED(rv)) return rv;
>+
>+  nsXPIDLCString spec;
>+  aURL->GetSpec(getter_Copies(spec));
>+
>+  printf("%s: pi is %p\n", spec.get(), pi.get());

this printf needs to go away, or be replaced with a PR_LOG, or whatever.

>Index: mailnews/compose/src/nsSmtpProtocol.cpp

>-  rv = OpenNetworkSocketWithInfo(hostCStr, aPort, nsnull, callbacks);
>+
>+  // create a uri
>+  nsAutoString spec;

why isn't this a nsCAutoString?  there is a |const char *| version of NS_NewURI

>+  spec.Assign(NS_LITERAL_STRING("smtp:"));
>+  spec.Append(aHost);
>+  spec.Append(PRUnichar(':'));
>+  spec.AppendInt(aPort);
>+
>+  nsCOMPtr<nsIURI> uri;
>+  rv = NS_NewURI(getter_AddRefs(uri), spec);

Comment 28

[Bradley Baetz (:bbaetz)]

OK, new patch coming up.

This will not work for pop or smtp, because they don't register themselves as
protocol handlers properly (see my other comments). Unless there is a reason for
this, I'll file another bug.

Comment 29

[Bradley Baetz (:bbaetz)]

Attachment: new patch

Comment 30

[Hixie (not reading bugmail)]

well, the patch works for me, thanks bbaetz! :-D

Comment 31

[Scott MacGregor]

Sorry for the delay, I've been ignoring the trunk completely and focusing on 094
until today. Just getting caught up on reviews.....

1) In nsMsgProtocol,
+  rv = pps->ExamineForProxy(aURL, getter_AddRefs(pi));
+  if (NS_FAILED(rv)) return rv;
+
+  return OpenNetworkSocketWithInfo(hostName, port, connectionType,
+                                   pi, callbacks);

I'd prefer us to ignore any error code from ExamineForProxy. If that routine
fails for some reason I still want us to open the network socket, just passing
in null for pi. so maybe if rv failed, set pi = null.
Furthermore, if we can't get the proxy service for some reason I don't want us
kicking out either. We should still try to open the network socket if for some
reason the proxy service isn't available with a null proxy object.

2) You have several local variables representing proxy info called "pi". Can you
rename those to proxyInfo or something more descriptive than pi? 

3) Same issue as in (1), this time in nsSmtpProtocol, nsImapProtocol,
nsNntpProtocol, nsPop3Protocol

Some of the url schemes used in mail (like smtp:) are private schemes used only
internally. They aren't really public schemes that someone is going to want to
create through the IO service. That's why they aren't registered. If we need to
register them for this bug then I need to think about that for a bit to make
sure there aren't any ramifications.

Comment 32

[Bradley Baetz (:bbaetz)]

1) Ok, will do.
2) ditto
3) ditto

WRT to registering protocol handlers, I think that you do have to do this. In
the case of pop vs pop3, there should be no problem. Note that that already bit
us once before (briefly, as a snoketest blocker), when dougt checked in the port
blocking stuff, and we couldn't get the nsIProtocolHandler, because he used the
wrong string.

smtp is trickier, because you don't really want it as a valid url. However, we
do want to be able to ask the proxy service for a socks proxy, and since pac
filters on uris, we have to give it one.

darin, would an nsIProtocolHandler which returned NS_ERROR_UNKNOWN_PROTOCOL for
newchannel be legal? That way an smtp url typed in the url bar would still give
the error, I think.

Comment 33

[benc]

re: unable to make proxy connection, I'm wondering how this compares w/ 4xp. If
someone has selected a SOCKS server, it is usually because they are on an
isolated lan and cannot connect without a valid SOCKS server. 

Is this for only SMTP, POP, IMAP and NNTP? So no encrypted SIMAP, SNEWS, etc?

Comment 34

[Bradley Baetz (:bbaetz)]

benc: No, this is for everything. snews via an SSL proxy won't work, and
pop/smtp needs some other changes, independant of this patch.

Comment 35

[Bradley Baetz (:bbaetz)]

Attachment: new patch

Comment 36

[Bradley Baetz (:bbaetz)]

Oops, I though I'd uploaded this last week.

Anyway, I've fixed mscott's nits, and changed the pop url to match what its
registered as. I also changes the smtp to use mailto, which is wrong, but it
works. It would only be noticable by someone using a pac file.

Comment 37

[Darin Fisher]

Comment on attachment 55505 [details] [diff] [review]
new patch

>Index: netwerk/base/public/nsNetUtil.h

>+inline nsresult
>+NS_ExamineForProxy(const char* scheme, const char* host, PRInt32 port, 
>+                   nsIProxyInfo* *proxyInfo)
>+{
>+    nsresult rv;
>+
>+    static NS_DEFINE_CID(kPPSServiceCID, NS_PROTOCOLPROXYSERVICE_CID);
>+    nsCOMPtr<nsIProtocolProxyService> pps = do_GetService(kPPSServiceCID,&rv);
>+
>+    if (NS_FAILED(rv)) return rv;
>+
>+    nsCString spec(scheme);

nsCAutoString?


>+    spec.Append(':');
>+    spec.Append(host);
>+    spec.Append(':');
>+    spec.AppendInt(port);
>+    
>+    nsCOMPtr<nsIURI> uri;
>+    rv = NS_NewURI(getter_AddRefs(uri), spec);
>+
>+    return pps->ExamineForProxy(uri, proxyInfo);
>+}
>+



>Index: mailnews/base/util/nsMsgProtocol.cpp

> 
>-  return OpenNetworkSocketWithInfo(hostName, port, connectionType, callbacks);
>+  nsresult rv;
>+
>+  nsCOMPtr<nsIProxyInfo> proxyInfo = nsnull;

no need to assign nsnull to a COMPtr.


>+  nsCOMPtr<nsIProtocolProxyService> pps =
>+      do_GetService("@mozilla.org/network/protocol-proxy-service;1", &rv);
>+
>+  if (NS_SUCCEEDED(rv)) {

how about just checking for a non-null pps since you don't care about returning rv?

if (pps)
    pps->ExamineForProxy(aURL, getter_AddRefs(proxyInfo));

no need to capture nsresults, i would think.


>Index: mailnews/compose/src/nsSmtpProtocol.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/compose/src/nsSmtpProtocol.cpp,v
>retrieving revision 1.123
>diff -u -r1.123 nsSmtpProtocol.cpp
>--- nsSmtpProtocol.cpp	2001/09/28 20:06:56	1.123
>+++ nsSmtpProtocol.cpp	2001/10/29 04:35:42
>@@ -68,6 +68,7 @@
> #include "nsMsgUtils.h"
> #include "nsIPipe.h"
> #include "nsMsgSimulateError.h"
>+#include "nsNetUtil.h"
> 
> #include "nsISSLSocketControl.h"
> /* sigh, cmtcmn.h, included from nsIPSMSocketInfo.h, includes windows.h, which includes winuser.h,
>@@ -1779,7 +1780,12 @@
>   nsCOMPtr<nsISmtpUrl> smtpUrl(do_QueryInterface(m_runningURL));
>   if (smtpUrl)
>       smtpUrl->GetNotificationCallbacks(getter_AddRefs(callbacks));
>-  rv = OpenNetworkSocketWithInfo(hostCStr, aPort, nsnull, callbacks);
>+
>+  nsCOMPtr<nsIProxyInfo> proxyInfo;
>+  rv = NS_ExamineForProxy("mailto", hostCStr, aPort, getter_AddRefs(proxyInfo));
>+  if (NS_FAILED(rv)) proxyInfo = nsnull;

likewise, proxyInfo can be assumed NULL if ExamineForProxy fails.  i think this is
a safe assumption.


>Index: mailnews/imap/src/nsImapProtocol.cpp

>+        nsCOMPtr<nsIProxyInfo> proxyInfo;
>+        rv = NS_ExamineForProxy("imap", hostName.get(), port, getter_AddRefs(proxyInfo));
>+        if (NS_FAILED(rv)) proxyInfo = nsnull;

likewise, proxyInfo can be assumed NULL if ExamineForProxy fails.


>Index: mailnews/local/src/nsPop3Protocol.cpp

>+    nsCOMPtr<nsIProxyInfo> proxyInfo;
>+    rv = NS_ExamineForProxy("pop", hostName.get(), port, getter_AddRefs(proxyInfo));
>+    if (NS_FAILED(rv)) proxyInfo = nsnull;

likewise, proxyInfo can be assumed NULL if ExamineForProxy fails.


>Index: mailnews/news/src/nsNNTPProtocol.cpp

>+    nsCOMPtr<nsIProxyInfo> proxyInfo;
>+    rv = NS_ExamineForProxy("nntp", hostName.get(), port, getter_AddRefs(proxyInfo));
>+    if (NS_FAILED(rv)) return rv;

should not return rv, right?

Comment 38

[Bradley Baetz (:bbaetz)]

>> nsCAutoString?

OK

>       
>> how about just checking for a non-null pps since you don't care about
>> returning rv?
Can I assume that?

> likewise, proxyInfo can be assumed NULL if ExamineForProxy fails.  i think
> this is a safe assumption.

Well, here I can assume that, because I wrote the function. It seems better to
keep it explicit, though.

> should not return rv, right?

Oops, missed that one.

Comment 39

[Bradley Baetz (:bbaetz)]

OK, I have a newer patch. I've kept the explicit null assignments, because thats
how XPCOM works - we can't assume anything about whether the out param has been
modified or not.

Also, the fact that mailnews creates nsIURIs with a scheme of smtp, but does not
register an smtp protocol handler is bad. For example, nsSmtpUrl::Clone (which
just calls nsStdURL) fails, because we can't find a protocol handler to call
newURI on. I've got arround this by using do_CreateInstance and setSpec - this
is OK, since I'm not actually going to open any network channels with this uri.
I've tested this on imap, pop, nntp, and sending mail.

Comment 40

[Bradley Baetz (:bbaetz)]

Attachment: patch

Comment 41

[Darin Fisher]

Comment on attachment 56162 [details] [diff] [review]
patch

r/sr=darin

Comment 42

[Bradley Baetz (:bbaetz)]

This missed 0.9.6. Seth has OK'd it for 0.9.7 during the mailnews freeze.

Comment 43

[Bradley Baetz (:bbaetz)]

Attachment: Updated

This patch is identical to previous patches. It just removes the fuzz from
other changes.

I'll go ping mscott again - I don't want this to slip another milestone...

Comment 44

[Bradley Baetz (:bbaetz)]

Sigh. I've sent mail with no response, and I'm going to be on holiday for about
a month, starting RSN.

->mscott for review and checkin, or reassignment to someone else for review

Comment 45

[Scott MacGregor]

didn't notice this in my list. We'll start looking at new features after .9.8
when the footprint & performance freeze for mailnews is lifted. 

Comment 46

[Bradley Baetz (:bbaetz)]

mscott: This is on your list because I'm off on holiday soon, and all it needs
is your review...

Comment 47

[Bradley Baetz (:bbaetz)]

Retargeting back to 0.9.9. There is a patch here. There has been a patch here
for 2 and a half months.

I'll check that this hasn't bit rotted before mailing for review Yet Again....

Taking.

Comment 48

[Scott MacGregor]

Comment on attachment 60442 [details] [diff] [review]
Updated

before you post a new patch, what do you think about registering smtp as a
protocol handler (nntp too?) but make sure they return errors if you try to
create a channel from them. That should allow you to remove a lot of special
cased code you had to add to work around that fact. 

We can probably leverage the existing protocol handlers for news and mailto.

Comment 49

[Bradley Baetz (:bbaetz)]

The problem is that you sort of want to create a channel from smtp (are smtp
urls valid?), and you also want to for nntp urls. theres also mailnews' internal
protocol handler stuff, and I'm not too sure how that works.

I think that it should be done (and possibly the mailnews specific stuff should
be removed), but thats a separate bug. The hacks weren't too great, really, but
I'm not sure what would break. To remove all the hacks, you'd also have to have
::Clone work properly on all the mailnews urls.

There is a bug on making mailnews not do all this. I can't find it now, though.
Bug 31241 is part of it. I'd prefer not to do this now....

Comment 50

[Bradley Baetz (:bbaetz)]

I tested nntp, smtp, imap, and pop, with and without a socks server enabled.

My uni's mail server doesn't support ssl. I did test it earlier though, and it
worked. I may see if I can set a dummy connection up via curl/netcat later, just
to test that the connection gets through.

The patch still applies.

Comment 51

[Scott MacGregor]

Comment on attachment 60442 [details] [diff] [review]
Updated

r=mscott

Comment 52

[Bradley Baetz (:bbaetz)]

Checked in.

Comment 53

[lchiang]

stephend - when you have time to verify, can you work with tever, benc to verify
this feature for nntp, smtp, pop, and imap protocols?

bradley - is there any UI to control this feature? 

Thanks

Comment 54

[Bradley Baetz (:bbaetz)]

UI is the socks proxy setting in prefs->advanced->proxies. There is no protocol
specificy proxying possible for mailnews, so there is not a separate option like
there is for specifiying an http proxy.

Comment 55

[benc]

Lisa:

For installer, we provide a proxy and technical support to the installer team.
Would we be able to do that for mailnews as well? If Tom or I tested this
feature, we would essentially have to run all the tests that mailnews does already.

Most of this feature is typing a hostname and port number into prefs. Everything
else should be transparent. We can help them with aspects of SOCKS they don't
understand.

Comment 56

[lchiang]

Thanks, Ben.  Stephend: pls help coordinate some testing on the mail side by
having folks perform the basic functional test, for example, with going through
SOCKS.  Thanks.

Comment 57

[Ken]

fyi, as an embedding client, when I now include v 1.49 of nsNetUtil.h I get:

necko\nsNetUtil.h(766) : error C2668: 'Append' : ambiguous call to overloaded 
function

Comment 58

[Bradley Baetz (:bbaetz)]

This compiles on all platforms mozilla builds on, which includes windows, unix,
mac, and os2.

What are the choices it gives you for the functions, and are you using mozilla's
string implementations?

Comment 59

[Michael Wardle]

I've been having problems downloading mail with nightlies since 0.9.8 (such as
20020225 and 20020226), and it has been suggested it's because of this.

Briefly, I'm using a PAC, and it seems that MailNews is now (probably
erroneously) trying to use a SOCKS proxy for mail.

See bug 127977.

Comment 60

[Ken]

I am hitting the error in comment #57 when compiling the embedding client. My 
code still uses the nsString.h header in the obsolete folder. Has there been a 
change in string handling in the last couple of months? Perhaps I am missing a 
new compiler directive in my makefile and I'm using the wrong string 
implementation?

Comment 61

[Bradley Baetz (:bbaetz)]

The first attachment to use that was made in early december last year.

You really should be using up to date strings if you are trying to use stuff
from the rest of the browser. Does your compiler give you a list of what the
ambiguous choices were?

If you are using current string sources, then please open a new bug. Assign it
to me, but I'll probably reassign it to an embedding person.

Comment 62

[benc]

Stephen: there are implementation bugs coming in about this feature. Do you want
to verify this bug or make it a meta bug for those problems, that seem to relate
to the feature design?

Comment 63

[[e-mail] (gone - use [e-mail] instead)]

This bug will be verified by the mail QA team when we have the information
needed to do so (Ben, I sent you mail).  If our verification of the basic
functionality fails with this feature enabled, I will re-open the bug and it
should remain open until the basics are fixed.  Otherwise, new, specific bugs,
should be filed.

Comment 64

[[e-mail] (gone - use [e-mail] instead)]

Setup Configuration:

Enter socks5qa.mcom.com and port 1080 into Edit | Preferences | Advanced |
Proxies | Manual Configuration, the SOCKS textfield.

Testing / Success Criteria:
 
Client, with SOCKS enabled per above instructions, will connect in a seamless,
transparent fashion to all protocols listed below on each platform.
Using netstat, verify that the client is making a connection to
carbuncle.nscp.aoltw.net:1080.
Build Information:

This feature was tested daily, with the current trunk builds, on all platforms,
until final verification.

Platform      | IMAP | POP3 | NNTP | AOL Mail | WebMail | SMTP | NEWS/IMAP/POP3
over SSL * |
-------------------------------------------------------------------------------------------|
Win 2000      |  Y   |  Y   |  Y   |    Y     |    Y    |   Y  |          N    
           |
OS X 10.1.3   |  Y   |  Y   |  Y   |    Y     |    Y    |   Y  |          N    
           |
Win 9.x       |  Y   |  Y   |  Y   |    Y     |    Y    |   Y  |          N    
           |
Mandrake 8.1  |  Y   |  Y   |  Y   |    Y     |    Y    |   Y  |          N    
           |
Mac OS 9.2.2  |  Y   |  Y   |  Y   |    Y     |    Y    |   Y  |          N    
           |
-------------------------------------------------------------------------------------------|

Known Issues (the 1st two adversely affect mail) and related links:

Connection / Severe:

Bug 134304 - Migration values fail from 4.7x
* Bug 133434 - SOCKS doesn't work over SSL (mail)
Bug 122752 - Proxy authentication support

Proxy Auto Configuration (PAC):

Bug 78176 - SOCKS return value is not version specific
Bug 112969 - SOCKS directive PAC hangs/crashes on startup

Links:

http://www.socks.nec.com/socksprot.html

There exist many more bugs on SOCKS - specifically, robust UI for controlling
individual connections,
see bug 51472.

All bugs dealing with the SOCKS Proxy support can be easily queried with 'SOCKS'
in the summary field of Bugzilla.

Verified FIXED with:

Mac OS X 10.1.3 - 2002-04-01-08
Mac OS 9.2.2 - 2002-04-01-08
Windows 2000 - 2002-04-01-03
Mandrake 8.1 - 2002-04-01-08
Windows 98 - 2002-04-01-03

- Stephen

Comment 65

[Bradley Baetz (:bbaetz)]

*** Bug 124107 has been marked as a duplicate of this bug. ***