Opened 9 years ago

Closed 8 years ago

#2611 closed enhancement (fixed)

Remove OpenSSL dependency

Reported by: chiiph Owned by: chiiph
Priority: High Milestone:
Component: Archived/Vidalia Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This patch removes everything related to OpenSSL, since it's not needed anymore.

Patch by Sebastian, btw.

FWIW, I've already tested this.

Child Tickets

Attachments (3)

0001-remove-openssl-checkpoint.patch (29.4 KB) - added by chiiph 9 years ago.
vidalia.zlib.patch (77.9 KB) - added by chiiph 8 years ago.
Dependency removal for zlib
vidalia.openssl.patch (11.0 KB) - added by chiiph 8 years ago.
Yet another openssl patch

Download all attachments as: .zip

Change History (12)

Changed 9 years ago by chiiph

comment:1 Changed 9 years ago by chiiph

Status: newneeds_review

comment:2 Changed 8 years ago by edmanm

I don't understand how you concluded that Vidalia doesn't need OpenSSL anymore. The code that downloads bridge addresses automatically for users obviously uses SSL to do so. The (currently unused) code that uploads Breakpad crash dumps uses SSL to do so. Just grep for QSslSocket and you can see this. If you ship a Vidalia package on Windows with a Qt that doesn't have OpenSSL static-linked into it (and the user doesn't already have OpenSSL installed in their %PATH%), then this patch is going to break Vidalia distributions on Windows.

That said, the portions of this patch related to removing the TorSslSocket class and USE_QSSLSOCKET #define are good. I'm surprised that stuff is still in there. I thought I had removed it awhile ago. ZlibByteArray and all zlib-related stuff could go, too.

comment:3 Changed 8 years ago by chiiph

Qt tries to load OpenSSL at runtime, but that doesn't mean we need to link to OpenSSL itself.
We can distribute the Windows binaries with the needed dlls as it is done already and that should be it.

Changed 8 years ago by chiiph

Attachment: vidalia.zlib.patch added

Dependency removal for zlib

comment:4 Changed 8 years ago by chiiph

Attached are the OpenSSL removal patch modified to leave the dlls and LICENSE files as they are right now, since we need to distribute OpenSSL. I've tested this in Windows 7, just throughing openssl's dlls where Vidalia executable is is enough for QSslSocket to load them when they are needed.

Also, a simple patch to remove the last bits of everything zlib-ish. We can consider this ticket a dep-removal one :)

comment:5 Changed 8 years ago by chiiph

Any comments on this one?

comment:6 Changed 8 years ago by edmanm

For some reason your Zlib-related patch includes the patch for some other ticket that was about disabling the dirport for bridges. If you cut that out, feel free to commit the rest of the Zlib patch.

As for the new OpenSSL-related patch, you shouldn't be deleting FindOpenSSL.cmake and the related INSTALL reference for -DOPENSSL_LIBRARY_DIR=/path/to/openssl entirely. The Windows installer files still need the OPENSSL_LIBRARY_DIR macro in order to find the OpenSSL files that get included in the installer. You just want to cut out the non-Windows stuff. The TorSslSocket class removal stuff looks good, though.

comment:7 in reply to:  6 Changed 8 years ago by chiiph

Replying to edmanm:

For some reason your Zlib-related patch includes the patch for some other ticket that was about disabling the dirport for bridges. If you cut that out, feel free to commit the rest of the Zlib patch.

Ohj, it keeps happening, sorry.
I'll commit the fixed patch for zlib.

As for the new OpenSSL-related patch, you shouldn't be deleting FindOpenSSL.cmake and the related INSTALL reference for -DOPENSSL_LIBRARY_DIR=/path/to/openssl entirely. The Windows installer files still need the OPENSSL_LIBRARY_DIR macro in order to find the OpenSSL files that get included in the installer. You just want to cut out the non-Windows stuff. The TorSslSocket class removal stuff looks good, though.

Right. The INSTALL reference for OPENSSL_LIBRARY_DIR should change to something like:
"Specify the location of OpenSSL's libraries when building on Windows."

I'll attach the updated patch in a bit.

Changed 8 years ago by chiiph

Attachment: vidalia.openssl.patch added

Yet another openssl patch

comment:8 Changed 8 years ago by edmanm

This looks good to me now.

comment:9 Changed 8 years ago by chiiph

Resolution: fixed
Status: needs_reviewclosed

Great, committed.

Note: See TracTickets for help on using tickets.