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

Support SNI. #7

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@avodonosov

This comment has been minimized.

Show comment
Hide comment
@avodonosov

avodonosov Aug 10, 2014

Contributor

Hi. Thank you for the contribution.

But I can't commit it as it is, because:

  1. From what I googled about the functions used in this patch,
    I guess the lisp-ssl-servername-cb callback is necessary
    for server-side, to allow the server use different certificates
    for different requested server names. But the patch
    installs the callback from make-ssl-client-stream - what relation
    does it have to client streams? Also, I do not understand
    the logic implemented in the callback.

  2. Is the SNI a stable API?
    I see in this changelog - http://git.openssl.org/gitweb/?p=openssl.git;a=blob_plain;f=CHANGES - that SSL_get_servername and friends were introduced in 2010 with "subject to change" status.
    Are they still subject to change or you have a link saying they are stable?

  3. Where can I read more or less complete guideline, or explanation about SNI API in OpenSSL?

  4. After the above is clarified, the proposed function handle-server-name needs to be re-factored. What this function actually does in the patch is: install server-name callback on global context; set the server-name parameter to the client request; call ssl-connect.

    It would be better to install the callback in another place, where the global context is initialized. In the make-ssl-client-stream only small piece remains:

    (when server-name (set-servername-param handle servername))
    (ensure-ssl-funcall stream handle #'ssl-connect handle)
  5. The documentation should be updated to mention the SNI functionality.

  6. I will need the information, how it is tested that:

    • new version doesn't break existing functionality
    • new functionality works.
Contributor

avodonosov commented Aug 10, 2014

Hi. Thank you for the contribution.

But I can't commit it as it is, because:

  1. From what I googled about the functions used in this patch,
    I guess the lisp-ssl-servername-cb callback is necessary
    for server-side, to allow the server use different certificates
    for different requested server names. But the patch
    installs the callback from make-ssl-client-stream - what relation
    does it have to client streams? Also, I do not understand
    the logic implemented in the callback.

  2. Is the SNI a stable API?
    I see in this changelog - http://git.openssl.org/gitweb/?p=openssl.git;a=blob_plain;f=CHANGES - that SSL_get_servername and friends were introduced in 2010 with "subject to change" status.
    Are they still subject to change or you have a link saying they are stable?

  3. Where can I read more or less complete guideline, or explanation about SNI API in OpenSSL?

  4. After the above is clarified, the proposed function handle-server-name needs to be re-factored. What this function actually does in the patch is: install server-name callback on global context; set the server-name parameter to the client request; call ssl-connect.

    It would be better to install the callback in another place, where the global context is initialized. In the make-ssl-client-stream only small piece remains:

    (when server-name (set-servername-param handle servername))
    (ensure-ssl-funcall stream handle #'ssl-connect handle)
  5. The documentation should be updated to mention the SNI functionality.

  6. I will need the information, how it is tested that:

    • new version doesn't break existing functionality
    • new functionality works.
@ruricolist

This comment has been minimized.

Show comment
Hide comment
@ruricolist

ruricolist Aug 10, 2014

I really should have specified that this only implements the client side of SNI.

The code is a direct translation from s_client.c. I have not been able to find any other reference for how to use SNI with OpenSSL beside that. The callback exists, to all appearances, so the client and server can share the same code to validate the ClientHello.

As for how stable the API is, I can only point to places where it’s used: Apache uses SSL_get_servername in mod_ssl, for example, so I imagine they don't expect it to go anywhere.

There is a site for testing client SNI implementations at https://sni.velox.ch.

ruricolist commented Aug 10, 2014

I really should have specified that this only implements the client side of SNI.

The code is a direct translation from s_client.c. I have not been able to find any other reference for how to use SNI with OpenSSL beside that. The callback exists, to all appearances, so the client and server can share the same code to validate the ClientHello.

As for how stable the API is, I can only point to places where it’s used: Apache uses SSL_get_servername in mod_ssl, for example, so I imagine they don't expect it to go anywhere.

There is a site for testing client SNI implementations at https://sni.velox.ch.

@avodonosov

This comment has been minimized.

Show comment
Hide comment
@avodonosov

avodonosov Aug 20, 2014

Contributor

The code is a direct translation from s_client.c.

I've reviewed s_client.c - you are right, it is the same code

The callback exists, to all appearances, so the client and server can share the same code to validate the ClientHello.

No, server callback (in s_server.c) has different code.

I still do not understand the client callback code. If this code is suitable for any client, there would be no point to require user to implement it in a callback - OpenSSL could just have it implemented inside. In other words, if OpenSSL allows/requires to specify a callback, shouldn't cl-plus-ssl, which is a wrapper of OpenSSL, provide similar API - ask user to specify callback? The patch you propose enforces the same code for all clients.

What does it mean hn == NULL ? The client didn't requested host name, the server didn't returned host name, or the host name returned by server failed verification (is different from the domain name specified in the server certificate)? Why do we check SSL_session_reused? What all the callback parameters mean.

I can't integrate this code until I understand it. It will probably take some time until I'll find all answers. It's inconvenient that the SNI API is undocumented.

How critical it is for you to have it committed soon?

Contributor

avodonosov commented Aug 20, 2014

The code is a direct translation from s_client.c.

I've reviewed s_client.c - you are right, it is the same code

The callback exists, to all appearances, so the client and server can share the same code to validate the ClientHello.

No, server callback (in s_server.c) has different code.

I still do not understand the client callback code. If this code is suitable for any client, there would be no point to require user to implement it in a callback - OpenSSL could just have it implemented inside. In other words, if OpenSSL allows/requires to specify a callback, shouldn't cl-plus-ssl, which is a wrapper of OpenSSL, provide similar API - ask user to specify callback? The patch you propose enforces the same code for all clients.

What does it mean hn == NULL ? The client didn't requested host name, the server didn't returned host name, or the host name returned by server failed verification (is different from the domain name specified in the server certificate)? Why do we check SSL_session_reused? What all the callback parameters mean.

I can't integrate this code until I understand it. It will probably take some time until I'll find all answers. It's inconvenient that the SNI API is undocumented.

How critical it is for you to have it committed soon?

@ruricolist

This comment has been minimized.

Show comment
Hide comment
@ruricolist

ruricolist Aug 21, 2014

I do think that client-side SNI support is important in the long run. The fact that Drakma using CL+SSL can't access SNI hosts is a minor irritation right now, but it can only get worse as SNI becomes more common.

That said, having support in CL+SSL proper is not critical for us.

ruricolist commented Aug 21, 2014

I do think that client-side SNI support is important in the long run. The fact that Drakma using CL+SSL can't access SNI hosts is a minor irritation right now, but it can only get worse as SNI becomes more common.

That said, having support in CL+SSL proper is not critical for us.

@deadtrickster

This comment has been minimized.

Show comment
Hide comment
@deadtrickster

deadtrickster Sep 24, 2014

Contributor

I think SNI support (at least for clients) is 100% must have. However looks like
(ssl-ctrl ,s +SSL_CTRL_SET_TLSEXT_HOSTNAME+ +TLSEXT_NAMETYPE_host_name+ ,name)
is just enough to do the job.
In recent days I played a bit with cl+ssl (resutls can be found here: https://github.com/deadtrickster/cl-plus-ssl/tree/local_contexts_and_verifications). As a refrences I used curl source (https://github.com/bagder/curl/blob/785395b07e01eb9085655d6ef311e34c2fca2224/lib/vtls/openssl.c) and this (https://github.com/iSECPartners/ssl-conservatory/blob/master/openssl/openssl_hostname_validation.c).

Results:

  • (ssl-ctrl ,s +SSL_CTRL_SET_TLSEXT_HOSTNAME+ +TLSEXT_NAMETYPE_host_name+ ,name)
    works just fine
  • Incomplete hostname validation (requires drakma to provide hostname)
  • Error reporting though ssl-ctx-set-verify callback.
  • CTX creation helper

I'm sorry for hijacking this pull request with a somewhat unrelated stuff but

  • I want to stress importance of hostname checking since in my view it is all-or-nothing. For http requests we must validate certificate chain AND hostname.
  • When building (REST) APIs wrappers I think it is good idea to have separate context for each library because it allows not only openssl customization (sessions, methods, etc), but also I can have separate trusted stores
Contributor

deadtrickster commented Sep 24, 2014

I think SNI support (at least for clients) is 100% must have. However looks like
(ssl-ctrl ,s +SSL_CTRL_SET_TLSEXT_HOSTNAME+ +TLSEXT_NAMETYPE_host_name+ ,name)
is just enough to do the job.
In recent days I played a bit with cl+ssl (resutls can be found here: https://github.com/deadtrickster/cl-plus-ssl/tree/local_contexts_and_verifications). As a refrences I used curl source (https://github.com/bagder/curl/blob/785395b07e01eb9085655d6ef311e34c2fca2224/lib/vtls/openssl.c) and this (https://github.com/iSECPartners/ssl-conservatory/blob/master/openssl/openssl_hostname_validation.c).

Results:

  • (ssl-ctrl ,s +SSL_CTRL_SET_TLSEXT_HOSTNAME+ +TLSEXT_NAMETYPE_host_name+ ,name)
    works just fine
  • Incomplete hostname validation (requires drakma to provide hostname)
  • Error reporting though ssl-ctx-set-verify callback.
  • CTX creation helper

I'm sorry for hijacking this pull request with a somewhat unrelated stuff but

  • I want to stress importance of hostname checking since in my view it is all-or-nothing. For http requests we must validate certificate chain AND hostname.
  • When building (REST) APIs wrappers I think it is good idea to have separate context for each library because it allows not only openssl customization (sessions, methods, etc), but also I can have separate trusted stores
@avodonosov

This comment has been minimized.

Show comment
Hide comment
@avodonosov

avodonosov Sep 25, 2014

Contributor

@deadtrickster , thanks for the information and pointers to example code.
I glanced briefly through all it, but can't deal with it right now, will return to it later.

I agree that possibility to create separate contexts is useful, been thinking about that too.

Contributor

avodonosov commented Sep 25, 2014

@deadtrickster , thanks for the information and pointers to example code.
I glanced briefly through all it, but can't deal with it right now, will return to it later.

I agree that possibility to create separate contexts is useful, been thinking about that too.

@richsalz

This comment has been minimized.

Show comment
Hide comment
@richsalz

richsalz May 6, 2015

Just a brief note (it's late, I'm tired:) There are no plans to change the OpenSSL SNI-related API at this point. It is, sadly, not documented. I'm a member of the openssl dev team; see https://github.com/openssl/openssl/commits/master What are the specific questions or concerns, I'll try to answer them.

richsalz commented May 6, 2015

Just a brief note (it's late, I'm tired:) There are no plans to change the OpenSSL SNI-related API at this point. It is, sadly, not documented. I'm a member of the openssl dev team; see https://github.com/openssl/openssl/commits/master What are the specific questions or concerns, I'll try to answer them.

@avodonosov

This comment has been minimized.

Show comment
Hide comment
@avodonosov

avodonosov May 7, 2015

Contributor

@richsalz thanks.

Lets limit the question to client-side:

Is it enough for client to just SSL_set_tlsext_host_name or client should also SSL_CTX_set_tlsext_servername_callback ?

I ask because we see in the curl sources (https://github.com/bagder/curl/blob/785395b07e01eb9085655d6ef311e34c2fca2224/lib/vtls/openssl.c) callback is not specified, but in the OpenSSL s_client.c (https://github.com/openssl/openssl/blob/b548a1f11c06ccdfa4f52a539912d22d77ee309e/apps/s_client.c) the callback is used. It seems like s_client.c only uses this callback to print a message to debug output - whether or not the server acknowledges SNI extension - and nothing else.

Contributor

avodonosov commented May 7, 2015

@richsalz thanks.

Lets limit the question to client-side:

Is it enough for client to just SSL_set_tlsext_host_name or client should also SSL_CTX_set_tlsext_servername_callback ?

I ask because we see in the curl sources (https://github.com/bagder/curl/blob/785395b07e01eb9085655d6ef311e34c2fca2224/lib/vtls/openssl.c) callback is not specified, but in the OpenSSL s_client.c (https://github.com/openssl/openssl/blob/b548a1f11c06ccdfa4f52a539912d22d77ee309e/apps/s_client.c) the callback is used. It seems like s_client.c only uses this callback to print a message to debug output - whether or not the server acknowledges SNI extension - and nothing else.

@richsalz

This comment has been minimized.

Show comment
Hide comment
@richsalz

richsalz May 7, 2015

Yes, you have it exactly right. the set_tlsext sets it in the clientHello, and the callback is used to see if the server responds with the value in its response.

richsalz commented May 7, 2015

Yes, you have it exactly right. the set_tlsext sets it in the clientHello, and the callback is used to see if the server responds with the value in its response.

@avodonosov

This comment has been minimized.

Show comment
Hide comment
@avodonosov

avodonosov May 7, 2015

Contributor

@richsalz Thanks.

So, in scope of this ticket we will only add SSL_set_tlsext_host_name when initializing the request.

Contributor

avodonosov commented May 7, 2015

@richsalz Thanks.

So, in scope of this ticket we will only add SSL_set_tlsext_host_name when initializing the request.

@richsalz

This comment has been minimized.

Show comment
Hide comment
@richsalz

richsalz May 7, 2015

Glad to help. Does this mean I helped get something in to common lisp? Way cool!

richsalz commented May 7, 2015

Glad to help. Does this mean I helped get something in to common lisp? Way cool!

@avodonosov

This comment has been minimized.

Show comment
Hide comment
@avodonosov

avodonosov May 7, 2015

Contributor

Yes, it does :)

Contributor

avodonosov commented May 7, 2015

Yes, it does :)

@avodonosov

This comment has been minimized.

Show comment
Hide comment
@avodonosov

avodonosov May 15, 2015

Contributor

So, I've added the hostname parameter to cl+ssl:make-ssl-client-stream. Tested it with the following code

(pushnew "/home/anton/prj/cl+ssl/cl-plus-ssl/" asdf:*central-registry*)
(ql:quickload :cl+ssl)
(ql:quickload :usocket)

(let* ((socket (usocket:socket-connect "sni.velox.ch" 443
                                       :element-type '(unsigned-byte 8)))
       (ssl-stream (cl+ssl:make-ssl-client-stream (usocket:socket-stream socket)
                                                  :hostname "sni.velox.ch"))
       (char-stream (flexi-streams:make-flexi-stream ssl-stream
                                                     :external-format '(:utf-8 :eol-style :crlf)))
       (reply-buf (make-string 1000)))
  (unwind-protect
       (progn
         (format char-stream "GET / HTTP/1.1~%")
         (format char-stream "Host: sni.velox.ch~%~%")
         (finish-output char-stream)
         (read-sequence reply-buf char-stream)
         reply-buf)
    (usocket:socket-close socket)))

Depending on the hostname parameter presence it returns different results.

@deadtrickster lets discuss other your suggestions - validation and context creation - in separate issues (you've already created #8 for validation)

Thanks all for participation in this ticket.

Contributor

avodonosov commented May 15, 2015

So, I've added the hostname parameter to cl+ssl:make-ssl-client-stream. Tested it with the following code

(pushnew "/home/anton/prj/cl+ssl/cl-plus-ssl/" asdf:*central-registry*)
(ql:quickload :cl+ssl)
(ql:quickload :usocket)

(let* ((socket (usocket:socket-connect "sni.velox.ch" 443
                                       :element-type '(unsigned-byte 8)))
       (ssl-stream (cl+ssl:make-ssl-client-stream (usocket:socket-stream socket)
                                                  :hostname "sni.velox.ch"))
       (char-stream (flexi-streams:make-flexi-stream ssl-stream
                                                     :external-format '(:utf-8 :eol-style :crlf)))
       (reply-buf (make-string 1000)))
  (unwind-protect
       (progn
         (format char-stream "GET / HTTP/1.1~%")
         (format char-stream "Host: sni.velox.ch~%~%")
         (finish-output char-stream)
         (read-sequence reply-buf char-stream)
         reply-buf)
    (usocket:socket-close socket)))

Depending on the hostname parameter presence it returns different results.

@deadtrickster lets discuss other your suggestions - validation and context creation - in separate issues (you've already created #8 for validation)

Thanks all for participation in this ticket.

@avodonosov avodonosov referenced this pull request May 15, 2015

Open

Check host/email/ip #8

@richsalz

This comment has been minimized.

Show comment
Hide comment
@richsalz

richsalz May 15, 2015

This is cool; great to see more SNI support. Thanks.

richsalz commented May 15, 2015

This is cool; great to see more SNI support. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment