Skip to content

Add the ssl stdlib module #1823

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Apr 1, 2020
Merged

Add the ssl stdlib module #1823

merged 17 commits into from
Apr 1, 2020

Conversation

coolreader18
Copy link
Member

@coolreader18 coolreader18 commented Mar 21, 2020 •
edited
Loading

This is all VERY flaky, with many uses of unsafe. I'm not sure if this is a good idea at this point, but it does work, albeit not certificate verification. EDIT: certificate verification does work, just not with google.com. @palaviv what's the deal? 😄

from urlllib.request import urlopen
print(urlopen("https://rustpython.github.io").read().decode())

@coolreader18 coolreader18 force-pushed the coolreader18/stdlib-ssl branch 8 times, most recently from a61f0ed to a37c9c8 Compare March 22, 2020 04:09
@coolreader18 coolreader18 marked this pull request as ready for review March 22, 2020 04:13
@coolreader18
Copy link
Member Author

Okay, I think this is in half-decent shape now. If anyone has experience with openssl, I'd love to hear any ideas about why certificate verification isn't working.

@coolreader18
Copy link
Member Author

Nevermind, certificate verification does work, just not with google.com for some reason.

@coolreader18 coolreader18 mentioned this pull request Mar 24, 2020
Merged
@coolreader18 coolreader18 force-pushed the coolreader18/stdlib-ssl branch from 80dd6ff to 6281f9c Compare March 25, 2020 03:41
@coolreader18 coolreader18 requested review from palaviv and youknowone and removed request for palaviv March 25, 2020 19:50
palaviv
palaviv approved these changes Mar 27, 2020
View reviewed changes
Copy link
Contributor

@palaviv palaviv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very impressive work!!!

vm/Cargo.toml
@@ -89,6 +91,8 @@ subprocess = "0.2.2"
num_cpus = "1"
socket2 = { version = "0.3", features = ["unix"] }
rustyline = "6.0"
openssl = "0.10"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we considered something pure rust? Maybe RusTLS?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into it, but _ssl's API pretty directly corresponds to OpenSSL's API; I don't think it would be guaranteed that everything we needed would be present in rustls.

@coolreader18 coolreader18 force-pushed the coolreader18/stdlib-ssl branch from 95e92fe to 509ac5a Compare March 27, 2020 16:03
youknowone
youknowone approved these changes Mar 27, 2020
View reviewed changes
@coolreader18 coolreader18 force-pushed the coolreader18/stdlib-ssl branch from 509ac5a to 5b7ee53 Compare March 27, 2020 17:41
@coolreader18 coolreader18 force-pushed the coolreader18/stdlib-ssl branch from 5b7ee53 to 1486b65 Compare March 27, 2020 19:40
@coolreader18 coolreader18 force-pushed the coolreader18/stdlib-ssl branch from 277ac0b to d10714d Compare March 27, 2020 20:24
@coolreader18
Copy link
Member Author

Is this good to merge?

youknowone
youknowone reviewed Mar 30, 2020
View reviewed changes
vm/src/stdlib/ssl.rs Outdated
#[pymethod]
fn set_ciphers(&self, cipherlist: PyStringRef, vm: &VirtualMachine) -> PyResult<()> {
let ciphers = cipherlist.as_str();
if ciphers.contains('0') {
Copy link
Member

@youknowone youknowone Mar 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably ciphers.contains(0 as char)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😳 yep, I meant '\0'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having a test for this method call will be good to ensure this is correctly working

@coolreader18
Copy link
Member Author

Also, would it be best to vendor openssl by using the vendored feature for rust-openssl? It would increase binary size, but it would also get rid of a lot of issues with linking to openssl (I don't think any of the weird Windows stuff would be necessary). Maybe best to only vendor on windows, if possible?

@youknowone
Copy link
Member

I have no idea about rust-openssl part but this PR looks good to me

@coolreader18 coolreader18 force-pushed the coolreader18/stdlib-ssl branch from d10714d to 7051b25 Compare April 1, 2020 03:40
@coolreader18
Copy link
Member Author

I think I'll look into vendoring-only-on-windows by contributing something to rust-openssl, I don't think it's possible otherwise, and it works for now.

@coolreader18 coolreader18 merged commit c1c9631 into master Apr 1, 2020
@coolreader18 coolreader18 deleted the coolreader18/stdlib-ssl branch April 1, 2020 16:34
@palaviv
Copy link
Contributor

palaviv commented Apr 4, 2020

Also, would it be best to vendor openssl by using the vendored feature for rust-openssl? It would increase binary size, but it would also get rid of a lot of issues with linking to openssl (I don't think any of the weird Windows stuff would be necessary). Maybe best to only vendor on windows, if possible?

This fails on my PC without the vendored feature. I would suggest adding this by default to avoid problems with different systems and versions of OpenSSL.

@palaviv palaviv mentioned this pull request Apr 4, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowone youknowone youknowone approved these changes

@palaviv palaviv palaviv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@coolreader18 @youknowone @palaviv