Closed Bug 44995 Opened 24 years ago Closed 23 years ago

[fix in hand] SOCKS - mailnews implementation

Categories

(MailNews Core :: Networking, defect, P1)

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.9

People

(Reporter: justin, Assigned: bbaetz)

References

Details

(Whiteboard: [Hixie-P0])

Attachments

(1 file, 5 obsolete files)

The various mailnews protocols (nntp, smtp, pop, and imap) do not use the SOCKS proxy. The following patch fixes this.
setting status to new and adding patch keyword.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: patch
we should check this in for beta3. Triaging. Thanks for contributing these changes for us Justin.
Status: NEW → ASSIGNED
Keywords: correctness, nsbeta3
Target Milestone: --- → M18
Keywords: mail2
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.
We won't hold our release for this, but if you can get it in and working on time, go for it. nsbeta3-
Whiteboard: [nsbeta3-]
sorry for the extra email. Removing mail2 keyword.
Keywords: mail2
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?
Adding mail3 keyword for inclusion or consideration in next release.
Keywords: mail3
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.
Is this supposed to be fixed in Mozilla 0.6 ??? Because it still won't work for me!
marking nsbeta1-. Though, if the patch works, we should get this in.
Keywords: nsbeta1-
If this worked in Communicator, I don't see how this could be dropped without some justification.
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...
Target Milestone: M18 → ---
updated Summary
Summary: mailnews protocols do not support SOCKS proxy → SOCKS - mailnews implementation
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.
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.
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!
Depends on: 89500
A binary patch would be great. Is there a chance to get it?
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.
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.
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.
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.
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.
Keywords: helpwanted
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.
Severity: minor → major
Whiteboard: [nsbeta3-] → [Hixie-P0][Hixie-1.0]
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.
Assignee: mscott → bbaetz
Status: ASSIGNED → NEW
Keywords: helpwantedreview
Target Milestone: --- → mozilla0.9.6
Attached patch patch (obsolete) (deleted) — Splinter Review
Attachment #11160 - Attachment is obsolete: true
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);
Attachment #52626 - Flags: needs-work+
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.
Attached patch new patch (obsolete) (deleted) — Splinter Review
Attachment #52626 - Attachment is obsolete: true
well, the patch works for me, thanks bbaetz! :-D
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.
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.
Status: NEW → ASSIGNED
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?
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.
Attached patch new patch (obsolete) (deleted) — Splinter Review
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 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?
Attachment #55505 - Flags: needs-work+
>> 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.
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.
Attached patch patch (obsolete) (deleted) — Splinter Review
Comment on attachment 56162 [details] [diff] [review] patch r/sr=darin
Attachment #56162 - Flags: superreview+
This missed 0.9.6. Seth has OK'd it for 0.9.7 during the mailnews freeze.
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Attached patch Updated (deleted) — Splinter Review
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...
Attachment #53292 - Attachment is obsolete: true
Attachment #55505 - Attachment is obsolete: true
Attachment #56162 - Attachment is obsolete: true
Attachment #60442 - Flags: superreview+
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
Assignee: bbaetz → mscott
Status: ASSIGNED → NEW
Priority: P3 → P1
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.
Target Milestone: mozilla0.9.7 → mozilla0.9.8
mscott: This is on your list because I'm off on holiday soon, and all it needs is your review...
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Summary: SOCKS - mailnews implementation → [fix in hand] SOCKS - mailnews implementation
Whiteboard: [Hixie-P0][Hixie-1.0] → [Hixie-P0]
Target Milestone: mozilla0.9.9 → mozilla1.2
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.
Assignee: mscott → bbaetz
Status: ASSIGNED → NEW
Target Milestone: mozilla1.2 → mozilla0.9.9
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.
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....
Status: NEW → ASSIGNED
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 on attachment 60442 [details] [diff] [review] Updated r=mscott
Attachment #60442 - Flags: review+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
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
QA Contact: lchiang → stephend
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.
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.
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.
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
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?
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.
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?
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.
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?
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.
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
Status: RESOLVED → VERIFIED
*** Bug 124107 has been marked as a duplicate of this bug. ***
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: