From 8a54568d3e7d70f23cfeda292aff21b8c2203f49 Mon Sep 17 00:00:00 2001 From: "F. Jason Park" Date: Thu, 7 Sep 2023 06:08:38 -0700 Subject: [PATCH 0/4] *** NOT A PATCH *** *** BLURB HERE *** F. Jason Park (4): Don't hard code server ports in SOCKS tests Improve SOCKS error handling and add support for 4a [POC] Simplify network-stream openers in socks.el [POC] Integrate the socks and url libraries lisp/net/socks.el | 139 +++++++++++++++++++++++++++++------ lisp/url/url-gw.el | 8 +- lisp/url/url-http.el | 16 ++-- lisp/url/url-proxy.el | 18 ++++- lisp/url/url-vars.el | 11 ++- test/lisp/net/socks-tests.el | 67 +++++++++++++---- 6 files changed, 206 insertions(+), 53 deletions(-) Interdiff: diff --git a/lisp/net/socks.el b/lisp/net/socks.el index f8881ba3990..1bb78113d52 100644 --- a/lisp/net/socks.el +++ b/lisp/net/socks.el @@ -335,17 +335,14 @@ socks-override-functions (when socks-override-functions (advice-add 'open-network-stream :around #'socks--open-network-stream)) -(defcustom socks-connect-function 'open-network-stream +(defvar socks-connect-function #'open-network-stream "Function to open a network connection to a SOCKS provider. -Called with arguments suitable for `open-network-stream'." - :version "30.1" - :type '(choice (function-item :value open-network-stream) - (function :tag "User-provided function"))) +Called with arguments suitable for `open-network-stream'.") (defun socks-open-connection (server-info &rest stream-params) "Create and initialize a SOCKS process. -Perform authentication if needed. Expect SERVER-INFO to take the -form of `socks-server' and STREAM-PARAMS to be keyword params +Perform authentication if needed. Expect SERVER-INFO to resemble +`socks-server' and STREAM-PARAMS to be keyword parameters accepted by `open-network-stream'." (save-excursion (let ((proc @@ -473,7 +470,9 @@ socks-send-command (let ((err (process-get proc 'socks-reply))) (if (eql version 5) (nth (or err 1) socks-errors) - (nth (- (if (and err (<= 90 err 93)) err 91) 90) + ;; The defined error codes for v4 range from + ;; 90-93, but we store them in a simple list. + (nth (pcase err (90 0) (92 2) (93 3) (_ 1)) socks--errors-4))))) proc)) @@ -536,8 +535,12 @@ socks-find-services-entry (if udp socks-udp-services socks-tcp-services))) (defun socks--open-network-stream (orig-fun name buffer host service &rest params) - (let ((socks-override-functions orig-fun)) - (apply #'socks-open-network-stream name buffer host service params))) + "Call `socks-open-network-stream', falling back to ORIG-FUN. +Expect NAME, BUFFER, HOST, SERVICE, and PARAMS to be compatible +with `open-network-stream'." + (let ((socks-connect-function orig-fun)) + (apply (if socks-override-functions #'socks-open-network-stream orig-fun) + name buffer host service params))) (defcustom socks-proxied-tls-services '(443 6697) "Ports whose connections should use TLS. @@ -563,11 +566,9 @@ socks-open-network-stream though `socks-connect-function' has the final say). For TLS with proxied connections, see the option `socks-proxied-tls-services'. -Before connecting, check the host against `socks-noproxy' and, on -rejection, fall back to non-SOCKS. Similarly, when -`socks-override-functions' is a function, call it directly and -trust that it's not interested in options defined in this -library, such as `socks-server'. +Before connecting, check the HOST against `socks-noproxy'. On +rejection, fall back to a non-SOCKS connection determined by +the variable `socks-connect-function'. But, before doing anything, check if `url-using-proxy' is bound to a `url' struct object, as defined in `url-parse'. If so, @@ -589,9 +590,8 @@ socks-open-network-stream socks-username)) (socks-password (or (and url (url-password url)) socks-password))) - (if-let* (((booleanp socks-override-functions)) - (route (socks-find-route host service)) - (proc (apply #'socks-open-connection route params))) + (if-let ((route (socks-find-route host service)) + (proc (apply #'socks-open-connection route params))) (let ((port (if (numberp service) service (process-contact proc :service))) @@ -604,13 +604,10 @@ socks-open-network-stream (progn (gnutls-negotiate :process proc :hostname host :keylist (and certs (list certs))) + ;; FIXME skip when TLD is .onion. (nsm-verify-connection proc host port)) proc)) - (let ((fn (if (functionp socks-override-functions) - socks-override-functions ; `socks-noproxy' not consulted - #'open-network-stream)) ; `socks-noproxy' is non-nil - socks-override-functions) - (apply fn name buffer host service params))))) + (apply socks-connect-function name buffer host service params)))) (defun socks--initiate-command-connect (proc buffer host service) (progn ; preserve indentation level for git blame / code review diff --git a/lisp/url/url-vars.el b/lisp/url/url-vars.el index 73585275a71..d96890db04a 100644 --- a/lisp/url/url-vars.el +++ b/lisp/url/url-vars.el @@ -191,12 +191,12 @@ url-mail-command (defcustom url-proxy-services nil "An alist of schemes and proxy servers that gateway them. -Looks like ((\"http\" . \"hostname:portnumber\") ...). This is -set up from the ACCESS_proxy environment variables. Depending on -the gateway type, values may instead be expected to look like -\"proxyscheme://hostname:portnumber\" where \"proxyscheme\" is -something like \"socks5\". As of Emacs 30.1, this only applies -to SOCKS servers." +Looks like ((\"http\" . \"hostname:portnumber\") ...). This is set up +from the ACCESS_proxy environment variables. Depending on the +gateway type, Emacs may expect certain server values to specfiy a +\"scheme\", for example, \"proxyscheme://hostname:portnumber\", +in which \"proxyscheme\" is something like \"socks5\". As of +Emacs 30.1, this only applies to SOCKS servers." :type '(repeat (cons :format "%v" (string :tag "Protocol") (string :tag "Proxy"))) diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index 7c7f68eafa2..df69fb2f5cf 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -165,9 +165,9 @@ socks-tests--hello-world-http-request-pattern "Content-Length: 13\r\n\r\n" "Hello World!\n"))) -(defun socks-tests-perform-hello-world-http-request () +(defun socks-tests-perform-hello-world-http-request (&optional method) "Start canned server, validate hello-world response, and finalize." - (let* ((url-gateway-method 'socks) + (let* ((url-gateway-method (or method 'socks)) (url (url-generic-parse-url "http://example.com")) (server (socks-tests-canned-server-create)) ;; @@ -281,7 +281,7 @@ socks-tests-v5-auth-user-pass-blank ;; against curl 7.71 with the following options: ;; $ curl --verbose --proxy socks5h://127.0.0.1:10082 example.com -(ert-deftest socks-tests-v5-auth-none () +(defun socks-tests-v5-auth-none (method) "Verify correct handling of SOCKS5 when auth method 0 requested." (let ((socks-server '("server" "127.0.0.1" t 5)) (socks-authentication-methods (append socks-authentication-methods @@ -295,7 +295,27 @@ socks-tests-v5-auth-none (socks-unregister-authentication-method 2) (should-not (assq 2 socks-authentication-methods)) (ert-info ("Make HTTP request over SOCKS5 with no auth method") - (socks-tests-perform-hello-world-http-request))) + (socks-tests-perform-hello-world-http-request method))) (should (assq 2 socks-authentication-methods))) +(ert-deftest socks-tests-v5-auth-none () + (socks-tests-v5-auth-none 'socks)) + +;; This simulates the top-level advice around `open-network-stream' +;; that's applied when loading the library with a non-nil +;; `socks-override-functions'. +(ert-deftest socks-override-functions () + (should-not socks-override-functions) + (should-not (advice-member-p #'socks--open-network-stream + 'open-network-stream)) + (advice-add 'open-network-stream :around #'socks--open-network-stream) + (should (advice-member-p #'socks--open-network-stream 'open-network-stream)) + + (unwind-protect (let ((socks-override-functions t)) + (socks-tests-v5-auth-none 'native)) + (advice-remove 'open-network-stream #'socks--open-network-stream)) + + (should-not (advice-member-p #'socks--open-network-stream + 'open-network-stream))) + ;;; socks-tests.el ends here -- 2.41.0