Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connections upgraded with STARTTLS are not reused #422

Closed
ehlertjd opened this issue Sep 10, 2015 · 9 comments
Closed

Connections upgraded with STARTTLS are not reused #422

ehlertjd opened this issue Sep 10, 2015 · 9 comments

Comments

@ehlertjd
Copy link
Contributor

As stated in title, upgraded connections are not reused.
Issue appears to be in ConnectionExists of url.c. The check (needle->handler->protocol & check->handler->protocol) resolves to false because the handler was switched to Curl_handler_imaps in imap_to_imaps called from imap_perform_upgrade_tls.

Code to reproduce the issue: https://gist.github.com/ehlertjd/d10e4f050e40f77b448d
(you'll need credentials against a mail server running IMAP with STARTTLS)

Tested against 7.42.1, but appears to still be present in 7.44.0.

@bagder bagder added the TLS label Sep 12, 2015
@bagder
Copy link
Member

bagder commented Sep 12, 2015

Without checking the code closer, I could also guess that this might also apply to other STARTTLS protocols...

@captain-caveman2k
Copy link
Contributor

I've finally had some time to look at the problem and it does affect the other protocols that support TLS upgrade - currently testing SMTP here at the moment.

I believe the problem has existed since commit 710f14e when the protocol flag became a single bit rather than being the non-SSL and SSL bits or'd together which then means the check in url.c:3260 fails :(

I am still searching for an alternative fix to introducing a new protocol handler structure specifically for TLS even though I do quite like that solution ;-)

@captain-caveman2k captain-caveman2k changed the title IMAP Connections upgraded with STARTTLS are not reused Connections upgraded with STARTTLS are not reused Dec 1, 2015
captain-caveman2k added a commit to captain-caveman2k/curl that referenced this issue Mar 5, 2016
@captain-caveman2k
Copy link
Contributor

Hi @ehlertjd,

Sorry it has taken me a while to get an "alternative" fix for this but I am started looking at this issue a few times and each time I backed out my changes. However, I think I finally cracked it earlier this evening after reading your comments from 14 Oct in #484 ;-)

I have pushed commit f97194a to my own fork if you and @bagder would like to take a look.

I haven't pushed it to badger/curl as I would like feedback from you guys as well as giving myself the chance to test it on my Linux VMs against the test harness - so far I have only tested this against an Exchange server using SMTP via curl command line and the --next option.

Note: The fix currently doesn't include FTP either - as I couldn't find whether the handler is changed to the SSL handler in ftp.c - any pointers would be very welcome ;-)

Kind Regards

Steve

@captain-caveman2k
Copy link
Contributor

Just a quick update... I've ran 'make test' and all but Test 46 (which is currently broken) run okay - so 1000 out of 1001.

@bagder
Copy link
Member

bagder commented Mar 6, 2016

The fix currently doesn't include FTP either - as I couldn't find whether the handler is changed to the SSL handler in ftp.c

The handler is not modified within ftp.c, as it isn't changed dynamically. It only uses the ftps handler struct if used with an explicit ftps:// URL.

@captain-caveman2k
Copy link
Contributor

The handler is not modified within ftp.c, as it isn't changed dynamically. It only uses the ftps handler struct if used with an explicit ftps:// URL.

Do you happen to know why we dynamically change it for the email protocols? I'm now wondering if we need to do this or whether we should do the same as FTP and only use the SSL handler for explicit TLS/SSL connections.

@bagder
Copy link
Member

bagder commented Mar 6, 2016

I can't recall the exact reasoning for it. As they're much newer implementations I think it was simply made like that because it seemed simpler or more appropriate, while the FTP code was moved into the handler "architecture" from an older style.

captain-caveman2k added a commit that referenced this issue Mar 8, 2016
Regression since commit 710f14e.

Bug: #422
Reported-by: Justin Ehlert
@captain-caveman2k
Copy link
Contributor

My fix has been pushed.

@ehlertjd
Copy link
Contributor Author

ehlertjd commented Mar 8, 2016

Sorry I didn't reply sooner, the changes look nice and simple to me, I
don't see any issues looking at the code.

Thanks for fixing this!

On Tue, Mar 8, 2016 at 1:46 PM, Steve Holme notifications@github.com
wrote:

Closed #422 #422.


Reply to this email directly or view it on GitHub
#422 (comment).

@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging a pull request may close this issue.

3 participants