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

Upgrade to latest OpenSSL #51

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open

Upgrade to latest OpenSSL #51

wants to merge 37 commits into from

Conversation

arcrose
Copy link
Contributor

@arcrose arcrose commented Aug 26, 2016

This update is a bit messy, I appologize in advanced for the lack of structure. I ended up doing a lot of hacking to try to address a few bugs I was dealing with in my own code.

Major things I've done here include:

  • Updating to the latest Rust-OpenSSL
    • This fixes a bug wherein creating secure data connections would fail, due to a segfault in rust-openssl (or openssl itself)
  • Switch the type contained by FtpError::SecureError from SslError to Box<Error> since the latest rust-OpenSSL contains several different errors in different modules, but we don't want to have to distinguish them
  • Have FtpStream now contain an SslContext since it implements Clone, and Ssl doesn't
  • Add an FTP server written in Python (using pyftpdlib) to test rust-ftp against
  • Add a certificate and key to test TLS (FTPS) against
  • Add an example program that uses the test cert to connect to the FTP server included to make sure encrypted control and data channels work

I know it's not great to have things all mixed up like this and again, I'm sorry for the lack of organization. Since I use this library for work, a lot of what I'm doing with it is being added in incrementally according to my immediate needs. I hope you'll agree that, even with an FTP server written in Python instead of Rust, all of these additions add value to the project.

Let me know if there's anything I can do to make this better.

Zack Mullaly and others added 30 commits July 18, 2016 15:06
…will contain ssl configuration information to be used in data_command
I discovered that there were some compilation errors I hadn't noticed that arise
when building with --features "secure". They have been taken care of.
… the codebase and have multiple copies of definitions of things to work around a feature flag when the tradeoff is having a lot more usable SSL
…es very similar to the into() method (https://doc.rust-lang.org/std/convert/trait.Into.html#tymethod.into), I changed their names to into_secure and into_insecure to be a little clearer about how they work
…type for other cases. The new type allows us to retain (and require) information about an Ssl configuration only when the user wants to supply one. Need a way to avoid so much code duplication though
@mattnenterprise
Copy link
Owner

@zsck Looks like the build is broken.

@arcrose
Copy link
Contributor Author

arcrose commented Aug 31, 2016

cargo test is trying to run the example I included which uses TLS, but we don't have any setup to have that happen.

@mattnenterprise
Copy link
Owner

I am ok with the server being in python. Just having a server to test with I think is a big plus. Are you going the fix the example ?

@arcrose
Copy link
Contributor Author

arcrose commented Aug 31, 2016

The example isn't broken, there just isn't any setup in place to have the Python server running before it runs. It doesn't compile because cargo test isn't using the secure feature flag, so openssl isn't being compiled. I don't have any ideas in mind for taking care of that right now.

@mattnenterprise
Copy link
Owner

I would like the build to pass before merging this in. Could we have the tests startup the python server ? Could we just have the test use the secure feature flag ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants