Make-context #24

deadtrickster · 2015-12-05T01:01:20Z

Provides with-global-context macro and make-context function

Can help with problems like described here:
edicl/drakma#50

Examples:

(cl+ssl:with-global-context 
    ((cl+ssl:make-context :verify-location "/etc/ssl/certs"))
  (drakma:http-request "https://google.com" :force-ssl t))

(cl+ssl:with-global-context 
    ((cl+ssl:make-context :verify-mode cl+ssl::+ssl-verify-none+))
  (drakma:http-request "https://incomplete-chain.badssl.com/"))

(cl+ssl:with-global-context 
    ((cl+ssl:make-context :verify-mode cl+ssl::+ssl-verify-peer+))
  (drakma:http-request "https://incomplete-chain.badssl.com/"))

SSL verify error: 20 X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY

(cl+ssl:with-context 
    ((cl+ssl:make-context :verify-mode cl+ssl::+ssl-verify-peer+ :verify-callback nil))
  (drakma:http-request "https://incomplete-chain.badssl.com/"))

A failure in the SSL library occurred on handle #.(SB-SYS:INT-SAP #X7FFE6825EC40) (return code: 1).
SSL error queue:
error:14090086:SSL routines:ssl3_get_server_certificate:certificate verify failed

X509_STORE_CTX_get_error SSL_CTX_set_default_verify_dir SSL_CTX_set_default_verify_file SSL_VERIFY_* constants SSL_CTX_SET_VERIFY SSL_SESS_CACHE_* constants

avodonosov · 2015-12-05T01:38:22Z

After a very superficial look: I think better name instead of make-global-context would be make-context or new-context or even context

(cl+ssl:with-global-context 
    (cl+ssl:new-context :verify-mode cl+ssl::+ssl-verify-peer+ :verify-callback nil)

  ...
  )

;; or something like

(cl+ssl:with-global-context 
    (cl+ssl:make-context :verify-mode cl+ssl::+ssl-verify-peer+ :verify-callback nil)

  ...
  )

(cl+ssl:with-global-context 
    (cl+ssl:build-context :verify-mode cl+ssl::+ssl-verify-peer+ :verify-callback nil)

  ...
  )

(cl+ssl:with-global-context 
    (cl+ssl:context :verify-mode cl+ssl::+ssl-verify-peer+ :verify-callback nil)

  ...
  )

deadtrickster · 2015-12-05T02:03:15Z

Actually it was make-context. I changed it just before creating pull request. Why?
Because *ssl-global-context* -> with-global-context -> make-global-context.

I don't think cl+ssl:(make-)context is a good name for this particular function since there is always two contexts (SSL_CTX_new and SSL_ctx). And this naming can be confusing.

avodonosov · 2015-12-05T20:56:48Z

No no, I think you probably forgetting (due to late hour?) your original, correct, reasoning.

SSL_CTX_new and SSL_ctx are not two context. SSL_CTX_new creates new context, it can create one, two or as many contexts as we want. SSL_ctx is a slot of ssl session storing a context earlier created by SSL_CTX_new.

make-context is just a convenience wrapper around SSL_CTX_new, that's all.

deadtrickster · 2015-12-05T21:04:05Z

yeah you are right, any other thoughts? I'll change the name right now

deadtrickster · 2015-12-05T21:09:38Z

done!

avodonosov · 2015-12-06T08:54:17Z

package.lisp

+           #:+ssl-verify-none+
+           #:+ssl-verify-peer+
+           #:+ssl-verify-fail-if-no-peer-cert+
+           #:+ssl-verify--client-once+


two dashes - is it typo?

avodonosov · 2015-12-06T09:22:40Z

We will also need to document new public API in the doc (index.html)

avodonosov · 2015-12-06T09:33:38Z

Makes sense to add ssl-ctx-free to the public API - the users who call make-context will want to free the context.

avodonosov · 2015-12-06T09:36:28Z

@deadtrickster do you think it would be convenient to have a auto-free option in the with-global-context macro?

deadtrickster · 2015-12-06T10:37:48Z

@avodonosov
maybe not ssl-ctx-free but

(declaim (inline free-context))
(defun free-context (context)
  (ssl-ctx-free context))

?

auto-free - why not? but only if it set to nil by default

documentation - news section is extremely outdated, as well as copyright years and holders btw :-)
I will add doc entry after ensure-initialized.

+ssl-verify-none+ +ssl-verify-peer+ +ssl-verify-fail-if-no-peer-cert+ :+ssl-verify-client-once+

avodonosov · 2015-12-06T11:24:58Z

maybe not ssl-ctx-free but

(declaim (inline free-context))
(defun free-context (context)
(ssl-ctx-free context))
?

It's too much IMO.

I don't wan't to hide real OpenSSL API, and introduce our different API. I think it's better to think of cl+ssl as a plain ffi bindings to OpenSSL + some helpers.

auto-free - why not? but only if it set to nil by default

nil by default, or course, I fully agree.

I wasn't sure about auto-free because it will complicate the invocation format of with-global-context; if the cases when auto-free is used are rare - probably most users will reuse their context more than once - then the majority of cases will suffer from complicated with-global-context while not having benefits of auto-free.

documentation - news section is extremely outdated, as well as copyright years and holders btw :-)
I will add doc entry after ensure-initialized.

Thanks.

Yes, there are outdated parts, in particular I am considering to remove the news section to avoid maintenance. But not now.

deadtrickster · 2015-12-06T11:55:06Z

I think it's better to think of cl+ssl as a plain ffi bindings to OpenSSL + some helpers.

Well given that cl+ssl implements gray streams I don't think it is plain ffi + some helpers
I'm sure coherent naming (esp without performance penalty) is way more important. If you call make-context why you should call ssl-ctx-free but not free-context?

auto-free

I played with it and you know what? It can kill you if you'll try to reuse freed context (basically on sbcl at least you are done and have to restart sbcl)! And as you rightly assume contexts indeed intended to be long-lived
Btw this is closely related to previous citation: should cl+ssl really punish its users that hard if they are trying to reuse freed contexts?

I remember when we met last time on #lisp we agreed that cl+ssl actually has two parts - plain ffi bindings and a lot of helpers ;-). Or I misunderstood something? Lets make it clear now since next step I plan is actually introduce wrapper around ssl-ctx structure with which I can at least test if it was freed, initialized or not (as I said earlier it just kills entire lisp instance in case of an error). If you think it is too much then I'll continue to contribute ffi related-stuff (there is so much to do too) but all these highlevel things will go to my separate repo.

deadtrickster · 2015-12-06T14:54:46Z

Changed some defaults, added documentation

deadtrickster · 2015-12-06T18:14:07Z

About #-asdf - I just googled how others do it.
I don't want to fight with you over free-context mostly because reevaluating my own use cases I returned :auto-free.

deadtrickster · 2015-12-06T18:22:20Z

added (hopefully concise) documentation entry for with-global-context

Make-context

avodonosov · 2015-12-06T18:28:28Z

Thank you very much for new features and for being so responsive during the review.

It's a great contribution - enables many convenient use cases, while preserving backward compatibility.

avodonosov · 2015-12-06T18:32:24Z

BTW, I forgot one important thing: do we use any new OpenSSL functions which present only in new versions of OpenSSL? (As I learned from your pull request #8, some features like verifying host/email/ip were absent in old OpenSSL and people needed to implement it themselves; later these features were added to OpenSSL). Do we now depend on some similar, newly introduced OpenSSL API?

deadtrickster · 2015-12-06T18:54:02Z

good question. recently a lot of stuff was deprecated and unfortunately openssl usually shipped very outdated. For example tls_method introduced that effectively replaces ssl_v23_method and no_sslv2|no_sslv3 flags i.e. it allows TLS only. rsa_callback deprecated too.

Now I don't think hostname verification should rely on openssl implementation. At least until we implement reliable openssl-version dependent switches. But even with these switches there is a great possibility of subtle difference between old-lisp and new-openssl implementations.

I have battle-tested erlang implementation, I'll just backport it and cover with tests.

avodonosov · 2015-12-06T19:08:32Z

I mean in this pull request, new functions we use, like SSL_CTX_set_verify, or the ciphers used in +default-cipher-list+ - are they backwards compatible, or they are only work with new OpenSSL? (I think unlikely, but just to check).

I have battle-tested erlang implementation [of hostname verfication], I'll just backport it and cover with tests.

Doesn't your pull request #8 provide this?

deadtrickster · 2015-12-06T19:23:46Z

SSL_CTX_set_verify is backwards compatible. +default-cipher-list+ should be too unless openssl EXTREMELY outdated.

Yep my pull request provide this however it was tested and used by me only. While https://github.com/deadtrickster/ssl_verify_hostname.erl is widely used and therefore I can be sure in its algorithm and tests.

avodonosov · 2015-12-06T23:45:50Z

Listen, I can't reproduce the verify-peer failure from examples in the description. Instead of the error I get normal HTTP reply:

(cl+ssl:with-global-context 
    ((cl+ssl:make-context :verify-mode cl+ssl::+ssl-verify-peer+))
  (drakma:http-request "https://incomplete-chain.badssl.com/"))

=>
"<!DOCTYPE html>
<html>
<head>
  <meta name=\"viewport\" content=\"width=device-width, initial-scale=1\">
  <link rel=\"shortcut icon\" href=\"/icons/favicon-orange.ico\"/>
  <link rel=\"apple-touch-icon\" href=\"/icons/icon-orange.png\"/>
  <title>incomplete-chain.badssl.com</title>
  <link rel=\"stylesheet\" href=\"/style.css\">
  <style>body { background: rgb(243, 121, 46); }</style>
</head>
<body>
<div id=\"content\">
  <h1 style=\"font-size: 8vw;\">
    incomplete-chain.<br>badssl.com
  </h1>
</div>

</body>
</html>
"
200
((:SERVER . "nginx/1.6.2 (Ubuntu)") (:DATE . "Sun, 06 Dec 2015 23:43:37 GMT") (:CONTENT-TYPE . "text/html") (:CONTENT-LENGTH . "506") (:LAST-MODIFIED . "Fri, 04 Dec 2015 02:46:28 GMT") (:CONNECTION . "close") (:ETAG . "\"5660fe84-1fa\"") (:CACHE-CONTROL . "no-store") (:ACCEPT-RANGES . "bytes"))
#<URI https://incomplete-chain.badssl.com/>
#<FLEXI-STREAMS:FLEXI-IO-STREAM #x302002FCD07D>
T
"OK"

avodonosov · 2015-12-06T23:46:47Z

I am trying on quicklisp 2015-09-24, CCL 1.9 on linux.

deadtrickster · 2015-12-06T23:56:31Z

Hmm at first I was thinking about my drakma patches, but:

(cl+ssl:with-global-context 
 ((cl+ssl:make-context :verify-mode cl+ssl::+ssl-verify-peer+))
 (usocket:with-client-socket (socket stream "incomplete-chain.badssl.com" 443
                                     :element-type '(unsigned-byte 8))
                             (let* ((ssl-stream (cl+ssl:make-ssl-client-stream stream
                                                                               :hostname  "incomplete-chain.badssl.com"))
                                    (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: incomplete-chain.badssl.com~%~%")
                                     (finish-output char-stream)
                                     (read-sequence reply-buf char-stream)
                                     reply-buf)
                                 (close ssl-stream)))))

also signals

SSL verify error: 20 X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY

sbcl 1.3.1 on linux too

avodonosov · 2015-12-06T23:58:32Z

This usocket example behaves the same for me

deadtrickster · 2015-12-07T00:00:30Z

ok let me install ccl too

avodonosov · 2015-12-07T00:02:55Z

By "the same" I mean signals failre - "the same" as your result

avodonosov · 2015-12-07T00:03:21Z

Only the drakma example differs - doesn't fail

deadtrickster · 2015-12-07T00:08:57Z

oh thank you :-) so looks like drakma issue. lets dive deeper into this when hostname verification merged

deadtrickster added 5 commits December 4, 2015 16:42

ignore emacs backups

ca203ae

More ffi imports

14bce4e

X509_STORE_CTX_get_error SSL_CTX_set_default_verify_dir SSL_CTX_set_default_verify_file SSL_VERIFY_* constants SSL_CTX_SET_VERIFY SSL_SESS_CACHE_* constants

need-tmp-rsa-callback -> tmp-rsa-callback

497ebae

delete trailing whitespace

7472270

new error type: ssl-error-call

1c352ba

deadtrickster force-pushed the make-context branch from af16f4f to 1e33ff8 Compare December 5, 2015 01:03

deadtrickster force-pushed the make-context branch from 1e33ff8 to 9965085 Compare December 5, 2015 21:08

deadtrickster changed the title ~~Make-global-context~~ Make-context Dec 5, 2015

avodonosov reviewed Dec 6, 2015
View reviewed changes

deadtrickster force-pushed the make-context branch from 9965085 to b388740 Compare December 6, 2015 10:32

make-context

2435a22

deadtrickster force-pushed the make-context branch 3 times, most recently from 87a3079 to 3d37dfa Compare December 6, 2015 11:14

deadtrickster added 2 commits December 6, 2015 03:19

catch SSL_CTX_new failure

f4d50bd

export make-context and constants

2d286cc

+ssl-verify-none+ +ssl-verify-peer+ +ssl-verify-fail-if-no-peer-cert+ :+ssl-verify-client-once+

deadtrickster force-pushed the make-context branch from 3d37dfa to f440b3e Compare December 6, 2015 11:19

deadtrickster added 9 commits December 6, 2015 10:12

export with-global-context

c07942e

ffi imports needed for disabling ssl methods

57dc1b3

support only tlsv1.* by default, disabled-protocols and options params

e6ea2ec

export no-{protocol} constans

7c8c22e

change default sess-cache mode

98608f0

export sess-cache-* constants

000fe54

export with-pem-password

dd308ba

rsa-callback is deprecated

cfba9d3

document make-context

fe9ee5d

deadtrickster force-pushed the make-context branch from 63cb082 to fe9ee5d Compare December 6, 2015 18:12

export ssl-ctx-free

51a7f39

document with-global-context

2bb1c39

avodonosov added a commit that referenced this pull request Dec 6, 2015

Merge pull request #24 from deadtrickster/make-context

93fcdf9

Make-context

avodonosov merged commit 93fcdf9 into cl-plus-ssl:master Dec 6, 2015

avodonosov mentioned this pull request Dec 14, 2018

Why is ssl-v23 the default method when it is deprecated upstream? #34

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make-context #24

Make-context #24

deadtrickster commented Dec 5, 2015

avodonosov commented Dec 5, 2015

deadtrickster commented Dec 5, 2015

avodonosov commented Dec 5, 2015

deadtrickster commented Dec 5, 2015

deadtrickster commented Dec 5, 2015

avodonosov Dec 6, 2015

deadtrickster Dec 6, 2015

avodonosov commented Dec 6, 2015

avodonosov commented Dec 6, 2015

avodonosov commented Dec 6, 2015

deadtrickster commented Dec 6, 2015

avodonosov commented Dec 6, 2015

deadtrickster commented Dec 6, 2015

deadtrickster commented Dec 6, 2015

deadtrickster commented Dec 6, 2015

deadtrickster commented Dec 6, 2015

avodonosov commented Dec 6, 2015

avodonosov commented Dec 6, 2015

deadtrickster commented Dec 6, 2015

avodonosov commented Dec 6, 2015

deadtrickster commented Dec 6, 2015

avodonosov commented Dec 6, 2015

avodonosov commented Dec 6, 2015

deadtrickster commented Dec 6, 2015

avodonosov commented Dec 6, 2015

deadtrickster commented Dec 7, 2015

avodonosov commented Dec 7, 2015

avodonosov commented Dec 7, 2015

deadtrickster commented Dec 7, 2015

Make-context #24

Make-context #24

Conversation

deadtrickster commented Dec 5, 2015

avodonosov commented Dec 5, 2015

deadtrickster commented Dec 5, 2015

avodonosov commented Dec 5, 2015

deadtrickster commented Dec 5, 2015

deadtrickster commented Dec 5, 2015

avodonosov Dec 6, 2015

Choose a reason for hiding this comment

deadtrickster Dec 6, 2015

Choose a reason for hiding this comment

avodonosov commented Dec 6, 2015

avodonosov commented Dec 6, 2015

avodonosov commented Dec 6, 2015

deadtrickster commented Dec 6, 2015

avodonosov commented Dec 6, 2015

deadtrickster commented Dec 6, 2015

deadtrickster commented Dec 6, 2015

deadtrickster commented Dec 6, 2015

deadtrickster commented Dec 6, 2015

avodonosov commented Dec 6, 2015

avodonosov commented Dec 6, 2015

deadtrickster commented Dec 6, 2015

avodonosov commented Dec 6, 2015

deadtrickster commented Dec 6, 2015

avodonosov commented Dec 6, 2015

avodonosov commented Dec 6, 2015

deadtrickster commented Dec 6, 2015

avodonosov commented Dec 6, 2015

deadtrickster commented Dec 7, 2015

avodonosov commented Dec 7, 2015

avodonosov commented Dec 7, 2015

deadtrickster commented Dec 7, 2015