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.

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

Implement SSL/TLS certificate checking #782 #6664

Closed
wants to merge 10 commits into from
Closed

Implement SSL/TLS certificate checking #782 #6664

wants to merge 10 commits into from

Conversation

FedericoCeratto
Copy link
Member

@FedericoCeratto FedericoCeratto commented Oct 31, 2017 •
edited
Loading

Closes #782 and #784

@FedericoCeratto FedericoCeratto force-pushed the ssl-cert-verify branch 4 times, most recently from 0e82e56 to 0f01b01 Compare November 5, 2017 23:33
dom96
dom96 requested changes Nov 5, 2017
View reviewed changes
@dom96
Copy link
Contributor

dom96 commented Nov 7, 2017

Btw, were you able to test this on macOS and Windows?

vegansk
vegansk reviewed Nov 9, 2017
View reviewed changes
@FedericoCeratto FedericoCeratto force-pushed the ssl-cert-verify branch 3 times, most recently from 174f997 to 26d42cc Compare November 13, 2017 23:38
@FedericoCeratto FedericoCeratto force-pushed the ssl-cert-verify branch 2 times, most recently from 8543fdf to 797bbf5 Compare November 23, 2017 01:15
@FedericoCeratto FedericoCeratto mentioned this pull request Dec 2, 2017
Closed
@FedericoCeratto
Copy link
Member Author

FedericoCeratto commented Dec 2, 2017 •
edited
Loading

@dom96 as discussed on IRC, the cert validation requires OpenSSL 1.1 which is not available on the buildbots (and therefore breaks Nimble). Do we want to:
a) add a define to disable the cert validation imports for older OpenSSL libs
b) or: detect the OpenSSL version dynamically at load time (how?) and enable cert validation for 1.1 only
c) or: backport cert validation to older versions using the available functions. This can be non-trivial.
...or something else? This changes intersect with #5000
Perhaps we should get this PR merged sooner and fix #5000 separately.

@perpetual-hydrofoil
Copy link

@FedericoCeratto any idea how python et al are handling this currently, since they've all started doing cert validation?

@FedericoCeratto
Copy link
Member Author

@jamiesonbecker replying to you in #5000

@FedericoCeratto FedericoCeratto force-pushed the ssl-cert-verify branch 2 times, most recently from 133d33f to 4ad51bc Compare December 17, 2017 23:07
@Araq
Copy link
Member

Araq commented Feb 2, 2018

Rebase? Next release is just around the corner.

@FedericoCeratto
Copy link
Member Author

Rebased. We need to discuss what to do to fix tests and support Nimble:
a) Add a define to disable SSL check in Nimble
b) Add a define to enable SSL check in Nimble (disabled by default)
c) Add a flag in a Nimble configuration
d) Add a flag in a Nimble configuration (disabled by default)

@Araq
Copy link
Member

Araq commented Feb 4, 2018

We need to discuss what to do to fix tests and support Nimble

Unfortunately I don't have an opinion on this question. :-)

@FedericoCeratto FedericoCeratto force-pushed the ssl-cert-verify branch 3 times, most recently from 8db9168 to 095d12a Compare February 17, 2018 12:47
@narimiran narimiran reopened this May 14, 2019
@FedericoCeratto FedericoCeratto force-pushed the ssl-cert-verify branch 2 times, most recently from a37365a to 0f51d44 Compare July 4, 2019 23:12
@vegai vegai mentioned this pull request Feb 7, 2020
Closed
@FedericoCeratto
Copy link
Member Author

Rebased.

timotheecour
timotheecour reviewed Feb 8, 2020
View reviewed changes
lib/wrappers/openssl.nim Outdated
let cert = d2i_X509(certbytes)
let encoded = cert.i2d_X509()
assert encoded == certbytes
echo "OK"
Copy link
Member

Choose a reason for hiding this comment

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

remove that line?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a test..

Copy link
Member

Choose a reason for hiding this comment

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

even if it's a test, it's not needed; see for example isMainModule tests inside lib/pure/math.nim
logging should be reserved for the exceptional, not the expected is a good rule of thumb

timotheecour
timotheecour reviewed Feb 8, 2020
View reviewed changes
tests/untestable/thttpclient_ssl_env_var.nim Outdated

else:
echo "ERROR: unexpected env variable value"
quit(1)
Copy link
Member

@timotheecour timotheecour Feb 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

replace all instances of quit with doAssert false, optional_informative_string
=> much friendlier for debugging when things go wrong thanks to stacktrace etc; plus doesn't break try/catch etc
see quit considered evil: bypasses defer and try/catch/finally which might do important cleanups - Nim forum : quit considered evil: bypasses defer and try/catch/finally which might do important cleanups - Nim forum

Copy link
Contributor

@Clyybber Clyybber Mar 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is a test.. We don't want a stacktrace here because it can change with unrelated changes

@FedericoCeratto
Copy link
Member Author

Related: https://github.com/FedericoCeratto/ssl-comparison

@FedericoCeratto
Copy link
Member Author

Rebased.

@FedericoCeratto FedericoCeratto mentioned this pull request Mar 19, 2020
Merged
Federico Ceratto added 2 commits March 19, 2020 13:52
Implement SSL/TLS certificate checking nim-lang#782
f2134db
SSL: Add nimDisableCertificateValidation
290d671 
Remove NIM_SSL_CERT_VALIDATION env var
tests/untestable/thttpclient_ssl.nim ran successfully on Linux with libssl 1.1.1d
Araq
Araq reviewed Mar 19, 2020
View reviewed changes
@FedericoCeratto
Copy link
Member Author

Status update: the tests are passing on Linux and OSX in #13697 and the Github action allows running the (heavy) tests only on changes related to SSL. The build is broken on Windows possibly due to the SSL version.

@FedericoCeratto
Copy link
Member Author

The contents of this PR, plus a dedicated Github action to test it have been merged in #13697

@FedericoCeratto FedericoCeratto deleted the ssl-cert-verify branch March 20, 2020 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dom96 dom96 dom96 requested changes

@Araq Araq Araq left review comments

@perpetual-hydrofoil perpetual-hydrofoil perpetual-hydrofoil left review comments

@timotheecour timotheecour timotheecour left review comments

@vegansk vegansk vegansk left review comments

@Clyybber Clyybber Clyybber left review comments

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

Successfully merging this pull request may close these issues.

[$150] httpclient SSL doesn't verify certificate chain
8 participants
@FedericoCeratto @dom96 @perpetual-hydrofoil @Araq @timotheecour @vegansk @Clyybber @narimiran