MINA SSHD
  1. MINA SSHD
  2. SSHD-102

Add error logging to org.apache.sshd.server.jaas.JaasPasswordAuthenticator.authenticate()

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.4.0, 0.5.0
    • Fix Version/s: 0.6.0
    • Labels:
      None
    • Environment:
      sshd JAAS used with LDAP based authentication.

      Description

      The JaasPasswordAuthenticator.authenticate() method currently silently ignores any exceptions being raised and simply returns false in case of any authentication erorrs.
      In environments where sshd is used in other frameworks like OSGi, it becomes very difficult to trouble shoot the reasons for the authentication failure.
      Rather than simply returning false, I propose to print a logging statement at the least.

      catch (Exception e)

      { log.error("Authentication failed with error: " + e.getMessage() + ", cause: " + e.getCause() ); return false; }

        Activity

        Hide
        Guillaume Nodet added a comment -

        I've slightly changed the log to log the root cause exception, as if we have to log something, i'd rather log the whole cause and not miss any details.

        Committing to https://svn.apache.org/repos/asf/mina/sshd/trunk ...
        M sshd-core/src/main/java/org/apache/sshd/server/jaas/JaasPasswordAuthenticator.java
        Committed r1050323

        Show
        Guillaume Nodet added a comment - I've slightly changed the log to log the root cause exception, as if we have to log something, i'd rather log the whole cause and not miss any details. Committing to https://svn.apache.org/repos/asf/mina/sshd/trunk ... M sshd-core/src/main/java/org/apache/sshd/server/jaas/JaasPasswordAuthenticator.java Committed r1050323
        Hide
        Guillaume Nodet added a comment -

        Ok, as long as the credentials can never appear in the log, i'm fine.

        Show
        Guillaume Nodet added a comment - Ok, as long as the credentials can never appear in the log, i'm fine.
        Hide
        Torsten Mielke added a comment -

        One could argue that the exception object itself should not carry the password if it is considered sensitive data, as it is never clear what class is going to catch the exception.
        Did a quick check here using an LDAP LoginModule and the password itself is not part of the exception.

        Using the attached patch this is the output:

        JaasPasswordAuthenticator [29] - Authentication failed with error: LDAP Error, cause: javax.security.auth.login.FailedLoginException

        Show
        Torsten Mielke added a comment - One could argue that the exception object itself should not carry the password if it is considered sensitive data, as it is never clear what class is going to catch the exception. Did a quick check here using an LDAP LoginModule and the password itself is not part of the exception. Using the attached patch this is the output: JaasPasswordAuthenticator [29] - Authentication failed with error: LDAP Error, cause: javax.security.auth.login.FailedLoginException
        Hide
        Guillaume Nodet added a comment -

        What I fear is that some sensitive informations such as passwords could end up in the log and that would be a big security issue.

        Show
        Guillaume Nodet added a comment - What I fear is that some sensitive informations such as passwords could end up in the log and that would be a big security issue.
        Hide
        Torsten Mielke added a comment -

        I specifically added e.getCause() to the logging statement as in my case I was hitting an error:
        java.lang.NumberFormatException: For input string: "10389""

        when specifying an LDAP server uri escaped in double quotes.
        This real cause was only accessible when calling getCause() on the exception.

        Show
        Torsten Mielke added a comment - I specifically added e.getCause() to the logging statement as in my case I was hitting an error: java.lang.NumberFormatException: For input string: "10389"" when specifying an LDAP server uri escaped in double quotes. This real cause was only accessible when calling getCause() on the exception.
        Hide
        Torsten Mielke added a comment -

        Proposed patch added.

        Show
        Torsten Mielke added a comment - Proposed patch added.

          People

          • Assignee:
            Guillaume Nodet
            Reporter:
            Torsten Mielke
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development