API for OpenSSL #3865

bararchy · 2017-01-10T13:01:39Z

No description provided.

jhass · 2017-01-10T14:11:50Z

src/openssl.cr

+#   # - https://www.owasp.org/index.php/Transport_Layer_Protection_Cheat_Sheet#Rule_-_Only_Support_Strong_Cryptographic_Ciphers
+#   # - https://cipherli.st/
+#   # - Full list is available at: https://wiki.openssl.org/index.php/Manual:Ciphers(1)#CIPHER_STRINGS
+#   context.ciphers = "EECDH+AESGCM:EDH+AESGCM:AES256+EECDH:AES256+EDH"


I'm not sure about adding a valid value here, people should be forced to actually follow the links given rather than copy pasting whatever is here. Also it should be made clear that we do provide a sane enough default.

jhass · 2017-01-10T14:13:03Z

src/openssl.cr

+#   context.certificate_chain = "/tmp/certificate.crt"
+#   # Those options are to enhance the security of the server by not using deprecated SSLv2 and SSLv3 protocols
+#   # It is also advised to disable Compression and enable only TLS1.2
+#   context.add_options(OpenSSL::SSL::Options::NO_SSLV2 | OpenSSL::SSL::Options::NO_SSLV3)


Same, those recommendations might get outdated and we do provide sane enough defaults. Or if we don't we should.

jhass · 2017-01-10T14:14:03Z

src/openssl.cr

+#   context = OpenSSL::SSL::Context::Client.new
+#   context.ciphers = "EECDH+AESGCM:EDH+AESGCM:AES256+EECDH:AES256+EDH"
+#   context.add_options(OpenSSL::SSL::Options::NO_SSLV2 | OpenSSL::SSL::Options::NO_SSLV3)
+#   context.verify_mode = OpenSSL::SSL::VerifyMode::NONE


I wouldn't even mention this option in a usage example. People should pay reasonable research effort into this, so they understand what they're doing with it.

- removed valid ciphers from code - moved links to the upper part of the doc

bararchy · 2017-01-10T14:59:52Z

@jhass , better ?

ysbaddaden · 2017-01-10T15:00:11Z

I'll be more harsh: drop the security tuning examples completely (ciphers, options, verify mode), except for specifying the keys; document that we do our best to define good defaults (Mozilla Intermediate) in the stdlib with a link to the Mozilla wiki page for more details, along with encouragements to notify us / send a pull request if we get out-of-date, have ciphers enabled that shouldn't be, etc.

Note that we don't exactly follow the Mozilla Intermediate configuration, for example we don't configure the secp384r1 and secp521r1 curves. If someone knows how to enable/initialize them, please step in!

bararchy · 2017-01-10T15:01:29Z

@ysbaddaden I just did , removed all security tuning.

bararchy · 2017-01-10T15:10:28Z

@jhass and @ysbaddaden .
I added more info regarding the Crystal supplying the sane defaults, also that if any issues are found in the choices we made, an issue should be opened

I would encourage opening a crystal-security mailing list so that security issues could be notified secretly and remain unpublished until they are fixed.

ysbaddaden · 2017-01-10T15:12:49Z

src/openssl.cr

+# - Full list is available at: https://wiki.openssl.org/index.php/Manual:Ciphers(1)#CIPHER_STRINGS
+#
+# Do note that
+# - Crystal do provide sane defaults for all Ciphers and protocols


I propose:

Crystal does its best to provide sane configuration defaults (see Mozilla Intermediate).

ysbaddaden · 2017-01-10T15:13:22Z

src/openssl.cr

+#   socket = TCPServer.new(5555) # Bind new TCPSocket to port 5555
+#   context = OpenSSL::SSL::Context::Server.new
+#   # Define which ciphers to use with OpenSSL
+#   context.ciphers = "VALID_CIPHER_STRING"


context.ciphers leftover.

ysbaddaden · 2017-01-10T15:13:58Z

src/openssl.cr

+# def client
+#   socket = TCPSocket.new("127.0.0.1", 5555)
+#   context = OpenSSL::SSL::Context::Client.new
+#   context.ciphers = "VALID_CIPHER_STRING"


ysbaddaden · 2017-01-10T15:23:51Z

src/openssl.cr

@@ -1,5 +1,56 @@
 require "./openssl/lib_ssl"

+# ### Example


Maybe have a header? Something as basic as # OpenSSL integration

Maybe also have an introduction that says that:

TLS sockets need a context, potentially with keys (required for servers) and configuration.

TLS sockets will wrap the underlying TCP socket, and any further communication must happen through the OpenSSL::SSL::Socket only.

Then continue with:

the default configuration (sane defaults, uses Mozilla Intermediate, best effort, please contribute)

the server example —I'm not sure we should add an example about how to generate the certificates, one should buy a certificate instead;

the client example.

bararchy · 2017-01-10T15:38:53Z

@ysbaddaden cool ?

bararchy · 2017-01-11T16:33:58Z

@ysbaddaden @jhass
Is this ok from your end ? or more changes needed ?

Sija · 2017-01-11T16:51:02Z

src/openssl.cr

+# - Linked version of OpenSSL need to be checked for supporting specific protocols and ciphers
+# - If any configurations or choices in Crystal regarding SSL settings and security are found to be lacking or need
+#   improvement please open an issue and let us know
+# ### Server side


Please add a newline before and after the heading ###.

Sija · 2017-01-11T16:51:15Z

src/openssl.cr

+#   end
+# end
+# ```
+# ### Client side


Sija · 2017-01-11T16:51:50Z

src/openssl.cr

+#   ssl_socket.write("Testing".to_slice)
+# end
+# ```
+


Please remove redundant newline.

bararchy · 2017-01-11T17:01:22Z

@Sija , done.

bararchy · 2017-01-14T16:04:10Z

@ysbaddaden @jhass @Sija

So ... are we done ? can this be merged ?

ysbaddaden · 2017-01-14T18:14:07Z

Looks OK to me.

Sija · 2017-01-14T19:18:16Z

src/openssl.cr

+#   socket = TCPServer.new(5555) # Bind new TCPSocket to port 5555
+#   context = OpenSSL::SSL::Context::Server.new
+#   context.private_key = "/path/to/private.key"
+#   context.certificate_chain = "/path/to/public.cert?"


is ? at the end of the path intentional?

Sija · 2017-01-14T19:20:27Z

src/openssl.cr

+#
+# ### Server side
+#
+# ```crystal


no need for crystal prefix here

Sija · 2017-01-14T19:20:35Z

src/openssl.cr

+#
+# ### Client side
+#
+# ```crystal


Sija · 2017-01-14T19:52:04Z

LGTM

ysbaddaden · 2017-01-16T09:45:26Z

Build has failed on x86-linux. Restarted.

api for openssl

e1b1314

bararchy mentioned this pull request Jan 10, 2017

[OpenSSL] Add a server and client usage example #3810

Closed

added full cipher string list link

c6b6002

jhass requested changes Jan 10, 2017

View reviewed changes

Changes to ciphers

4b5d006

- removed valid ciphers from code - moved links to the upper part of the doc

Added more info regarding defaults

fb91626

ysbaddaden requested changes Jan 10, 2017

View reviewed changes

bararchy added 2 commits January 10, 2017 17:32

Changed wording and removed cipher usage

51d5217

removed cli example

072de90

Sija reviewed Jan 11, 2017

View reviewed changes

src/openssl.cr

# end

# end

# ```

# ### Client side

Copy link

Contributor

Sija Jan 11, 2017

Choose a reason for hiding this comment

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

ditto

Sija reviewed Jan 11, 2017

View reviewed changes

src/openssl.cr

# ssl_socket.write("Testing".to_slice)

# end

# ```

Copy link

Contributor

Sija Jan 11, 2017

Choose a reason for hiding this comment

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

Please remove redundant newline.

newlines

262dbd2

Sija reviewed Jan 14, 2017

View reviewed changes

src/openssl.cr

#

# ### Server side

#

# ```crystal

Copy link

Contributor

Sija Jan 14, 2017

Choose a reason for hiding this comment

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

no need for crystal prefix here

Sija reviewed Jan 14, 2017

View reviewed changes

src/openssl.cr

#

# ### Client side

#

# ```crystal

Copy link

Contributor

Sija Jan 14, 2017

Choose a reason for hiding this comment

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

ditto

removed crystal suffix from markdown

c97eab0

ysbaddaden approved these changes Jan 16, 2017

View reviewed changes

jhass approved these changes Jan 16, 2017

View reviewed changes

ysbaddaden merged commit 87317f2 into crystal-lang:master Jan 16, 2017

spalladino added this to the Next milestone Jan 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API for OpenSSL #3865

API for OpenSSL #3865

bararchy commented Jan 10, 2017

jhass Jan 10, 2017 •

edited

jhass Jan 10, 2017

jhass Jan 10, 2017 •

edited

bararchy commented Jan 10, 2017

ysbaddaden commented Jan 10, 2017

bararchy commented Jan 10, 2017

bararchy commented Jan 10, 2017

ysbaddaden Jan 10, 2017

ysbaddaden Jan 10, 2017

ysbaddaden Jan 10, 2017

ysbaddaden Jan 10, 2017

bararchy commented Jan 10, 2017

bararchy commented Jan 11, 2017

Sija Jan 11, 2017

Sija Jan 11, 2017

Sija Jan 11, 2017

bararchy commented Jan 11, 2017

bararchy commented Jan 14, 2017

ysbaddaden commented Jan 14, 2017

Sija Jan 14, 2017

Sija Jan 14, 2017

Sija Jan 14, 2017

Sija commented Jan 14, 2017

ysbaddaden commented Jan 16, 2017

API for OpenSSL #3865

API for OpenSSL #3865

Conversation

bararchy commented Jan 10, 2017

jhass Jan 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhass Jan 10, 2017 • edited

Choose a reason for hiding this comment

bararchy commented Jan 10, 2017

ysbaddaden commented Jan 10, 2017

bararchy commented Jan 10, 2017

bararchy commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bararchy commented Jan 10, 2017

bararchy commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bararchy commented Jan 11, 2017

bararchy commented Jan 14, 2017

ysbaddaden commented Jan 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sija commented Jan 14, 2017

ysbaddaden commented Jan 16, 2017

jhass Jan 10, 2017 •

edited

jhass Jan 10, 2017 •

edited