Skip to content

Commit

Permalink
Better error reporting.
Browse files Browse the repository at this point in the history
- Use ex-info instead of Exception. Return plenty of metadata with each
  exception. Most deliberate exceptions now will contain a reference to
  the clj-libssh2.session.Session being used when it was thrown.
- Improve some error messages to be more accurate and/or less confusing.

Fixes #5
  • Loading branch information
conormcd committed Jan 24, 2016
1 parent 9de5b48 commit 30455ae
Show file tree
Hide file tree
Showing 8 changed files with 59 additions and 42 deletions.
14 changes: 10 additions & 4 deletions src/clj_libssh2/agent.clj
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
"Functions for interacting with an SSH agent. The agent is expected to be
available on the UNIX domain socket referred to by the SSH_AUTH_SOCK
environment variable."
(:require [clj-libssh2.error :refer [handle-errors with-timeout]]
(:require [clj-libssh2.error :as error :refer [handle-errors with-timeout]]
[clj-libssh2.libssh2 :as libssh2]
[clj-libssh2.libssh2.agent :as libssh2-agent])
(:import [com.sun.jna.ptr PointerByReference]))

Expand Down Expand Up @@ -32,7 +33,10 @@
(case ret
0 (.getValue id)
1 nil
(throw (Exception. "An unknown error occurred")))))
(throw (ex-info "libssh2_agent_get_identity returned a bad value."
{:function "libssh2_agent_get_identity"
:return ret
:session session})))))

(defn authenticate
"Attempt to authenticate a session using the agent.
Expand All @@ -50,7 +54,7 @@
[session username]
(let [ssh-agent (libssh2-agent/init (:session session))]
(when (nil? ssh-agent)
(throw (Exception. "Failed to initialize agent.")))
(error/maybe-throw-error session libssh2/ERROR_ALLOC))
(try
(handle-errors session
(with-timeout :agent
Expand All @@ -65,7 +69,9 @@
(libssh2-agent/userauth ssh-agent username id)))
id)
false)))
(throw (Exception. "Failed to authenticate with the agent.")))
(throw (ex-info "Failed to authenticate using the SSH agent."
{:username username
:session session})))
true
(finally
(handle-errors session
Expand Down
13 changes: 7 additions & 6 deletions src/clj_libssh2/authentication.clj
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
[clj-libssh2.agent :as agent]
[clj-libssh2.error :refer [handle-errors with-timeout]]
[clj-libssh2.libssh2.userauth :as libssh2-userauth])
(:import clojure.lang.PersistentArrayMap))
(:import [java.io FileNotFoundException]
[clojure.lang PersistentArrayMap]))

(defprotocol Credentials
"A datatype to represent a way of authentication and the necessary data to
Expand All @@ -21,9 +22,7 @@
(valid? [credentials] (and (some? username)
(some? passphrase)
(some? private-key)
(some? public-key)
(.exists (file private-key))
(.exists (file public-key)))))
(some? public-key))))

(defrecord PasswordCredentials [username password]
Credentials
Expand Down Expand Up @@ -54,7 +53,7 @@
[session credentials]
(doseq [keyfile (map #(% credentials) [:private-key :public-key])]
(when-not (.exists (file keyfile))
(throw (Exception. (format "%s does not exist." keyfile)))))
(throw (FileNotFoundException. keyfile))))
(handle-errors session
(with-timeout :auth
(libssh2-userauth/publickey-fromfile (:session session)
Expand All @@ -81,4 +80,6 @@
(authenticate session creds)
(if (< 1 (count m))
(recur (rest m))
(throw (Exception. "Invalid credentials")))))))
(throw (ex-info "Failed to determine credentials type."
{:items (keys credentials)
:session session})))))))
11 changes: 7 additions & 4 deletions src/clj_libssh2/channel.clj
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,8 @@
:else (str v)))
fail-if-forbidden (fn [ret]
(if (= libssh2/ERROR_CHANNEL_REQUEST_DENIED ret)
(throw (Exception. "Setting environment variables is not permitted."))
(throw (ex-info "Setting environment variables is not permitted."
{:session session}))
ret))]
(doseq [[k v] env]
(block session
Expand Down Expand Up @@ -322,9 +323,11 @@
(when (and (= pump-fn pull)
(= :eagain new-status)
(< (-> session :options :read-timeout) (- now last-read-time)))
(throw (Exception. (format "Read timeout for %s stream %d"
(-> stream :direction name)
(-> stream :id)))))
(throw (ex-info "Read timeout on a channel."
{:direction (-> stream :direction name)
:id (-> stream :id)
:timeout (-> session :options :read-timeout)
:session session})))
(assoc stream :status new-status :last-read-time now))
stream))

Expand Down
14 changes: 9 additions & 5 deletions src/clj_libssh2/error.clj
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,9 @@
which may be useful to debug the error handling itself:
:error The error message from error-messages, if any.
:session-error The error message from session-error-message, if any.
:error-code The numeric return value."
:error-code The numeric return value.
:session The clj-libssh2.session.Session object, if any.
:session-error The error message from session-error-message, if any."
[session error-code]
(when (and (some? error-code)
(> 0 error-code)
Expand All @@ -121,8 +122,9 @@
default-message
(format "An unknown error occurred: %d" error-code))
{:error default-message
:session-error session-message
:error-code error-code})))))
:error-code error-code
:session session
:session-error session-message})))))

(defmacro handle-errors
"Run some code that might return a negative number to indicate an error.
Expand Down Expand Up @@ -197,7 +199,9 @@
timeout# (get-timeout ~timeout)]
(loop [timedout# false]
(if timedout#
(throw (Exception. "Timeout!"))
(throw (ex-info "Timeout exceeded."
{:timeout ~timeout
:timeout-length timeout#}))
(let [r# (do ~@body)]
(if (= r# libssh2/ERROR_EAGAIN)
(recur (< timeout# (- (System/currentTimeMillis) start#)))
Expand Down
8 changes: 5 additions & 3 deletions src/clj_libssh2/known_hosts.clj
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"Utilities for checking the fingerprint of a remote machine against a list of
known hosts."
(:require [clojure.java.io :refer [file]]
[clj-libssh2.error :refer [handle-errors with-timeout]]
[clj-libssh2.error :as error :refer [handle-errors with-timeout]]
[clj-libssh2.libssh2 :as libssh2]
[clj-libssh2.libssh2.knownhost :as libssh2-knownhost]
[clj-libssh2.libssh2.session :as libssh2-session])
Expand Down Expand Up @@ -39,7 +39,9 @@
libssh2/ERROR_HOSTKEY_SIGN
0)
libssh2/KNOWNHOST_CHECK_FAILURE libssh2/ERROR_HOSTKEY_SIGN
(throw (Exception. (format "Unknown return code from libssh2-knownhost/checkp: %d" result)))))
(throw (ex-info "Unknown return code from libssh2_knownhost_checkp."
{:function "libssh2_knownhost_checkp"
:return result}))))

(defn- host-fingerprint
"Get the remote host's fingerprint.
Expand Down Expand Up @@ -131,7 +133,7 @@
fail-on-mismatch (-> session-options :fail-unless-known-hosts-matches)
fail-on-missing (-> session-options :fail-if-not-in-known-hosts)]
(when (nil? known-hosts)
(throw (Exception. "Failed to initialize known hosts store.")))
(error/maybe-throw-error session libssh2/ERROR_ALLOC))
(try
(load-known-hosts session known-hosts file)
(check-fingerprint session
Expand Down
16 changes: 8 additions & 8 deletions src/clj_libssh2/session.clj
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
[clj-libssh2.libssh2 :as libssh2]
[clj-libssh2.libssh2.session :as libssh2-session]
[clj-libssh2.authentication :refer [authenticate]]
[clj-libssh2.error :refer [handle-errors with-timeout]]
[clj-libssh2.error :as error :refer [handle-errors with-timeout]]
[clj-libssh2.known-hosts :as known-hosts]
[clj-libssh2.socket :as socket]))

Expand Down Expand Up @@ -37,7 +37,7 @@
[]
(let [session (libssh2-session/init)]
(when-not session
(throw (Exception. "Failed to create a libssh2 session.")))
(error/maybe-throw-error nil libssh2/ERROR_ALLOC))
session))

(defn- create-session-options
Expand Down Expand Up @@ -73,15 +73,15 @@
nil or throws an exception if requested."
([session]
(destroy-session session "Shutting down normally." false))
([{session :session} reason raise]
([session reason raise]
(handle-errors nil
(with-timeout :session
(libssh2-session/disconnect session reason)))
(libssh2-session/disconnect (:session session) reason)))
(handle-errors nil
(with-timeout :session
(libssh2-session/free session)))
(libssh2-session/free (:session session))))
(when raise
(throw (Exception. reason)))))
(throw (ex-info reason {:session session})))))

(defn- handshake
"Perform the startup handshake with the remote host.
Expand Down Expand Up @@ -147,9 +147,9 @@
(authenticate session credentials)
(swap! sessions conj session)
session
(catch Exception e
(catch Throwable t
(close session)
(throw e)))))
(throw t)))))

(defmacro with-session
"A convenience macro for running some code with a particular session.
Expand Down
17 changes: 9 additions & 8 deletions src/clj_libssh2/socket.clj
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,18 @@
port)]
(when (> 0 socket)
;; Magic numbers are from libsimplesocket.h
(throw (Exception. (condp = socket
SIMPLE_SOCKET_BAD_ADDRESS
(format "%s is not a valid IP address" address)
(let [message (condp = socket
SIMPLE_SOCKET_BAD_ADDRESS
(format "%s is not a valid IP address" address)

SIMPLE_SOCKET_SOCKET_FAILED
"Failed to create a TCP socket"
SIMPLE_SOCKET_SOCKET_FAILED
"Failed to create a TCP socket"

SIMPLE_SOCKET_CONNECT_FAILED
(format "Failed to connect to %s:%d" address port)
SIMPLE_SOCKET_CONNECT_FAILED
(format "Failed to connect to %s:%d" address port)

"An unknown error occurred"))))
"simple_socket_connect returned a bad value")]
(throw (ex-info message {:socket socket}))))
socket))

(defn select
Expand Down
8 changes: 4 additions & 4 deletions test/clj_libssh2/test_authentication.clj
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
(session/close session))
(is (= 0 (count @session/sessions))))

; This is more fully tested in clj-libssh2.test-authentication
; This is more fully tested in clj-libssh2.test-agent
(deftest agent-authentication-works
(is (auth (->AgentCredentials (test/ssh-user)))))

Expand Down Expand Up @@ -49,10 +49,10 @@
(is (valid? unauthorized))
(is (thrown? Exception (auth unauthorized))))
(testing "It fails if the private key file doesn't exist"
(is (not (valid? bad-privkey)))
(is (valid? bad-privkey))
(is (thrown? Exception (auth bad-privkey))))
(testing "It fails if the public key file doesn't exist"
(is (not (valid? bad-pubkey)))
(is (valid? bad-pubkey))
(is (thrown? Exception (auth bad-pubkey))))
(testing "It fails if the passphrase is incorrect"
(is (valid? with-wrong-passphrase))
Expand All @@ -78,5 +78,5 @@
(deftest authenticating-with-a-map-fails-if-there's-no-equivalent-record
(is (thrown-with-msg?
Exception
#"Invalid credentials"
#"Failed to determine credentials type"
(auth {:password "foo"}))))

0 comments on commit 30455ae

Please sign in to comment.