Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0
    • Fix Version/s: 2.0
    • Labels:
      None

      Description

      If an exception occurs accessing a FileObject on a FileSystem that is addressed with an URL containing user and password the thrown exception contains the password as part of the error message:

      org.apache.commons.vfs.FileSystemException: Could not connect to SFTP server at "sftp://user:password@apache.org/".

      In such a case the URL should be printed as "sftp://user:***@apache.org/". Same applied to log messages - at least for INFO and higher.

      This is a security risk, since in big companies exceptions and logs are normally collected and archived in monitoring systems and may reveal the password to persons that have normally no authorization to the target system.

      1. vfs-pwd.patch
        4 kB
        Frank van der Kleij

        Activity

        Hide
        Frank van der Kleij added a comment - - edited

        I think too that the password should never ever be revealed once entered; not in exceptions, nor in other debug messages.

        Probably adapting the AbstractFileName.toString to not print the password would fix most of this. Then the method getFriendlyURI can be deprecated as well.

        I have one remark though. I really dislike the format 'scheme://user:***@host' where the password is hidden with *** ; it is kind of ugly and I don't think it serves any purpose to show the ***.

        I would rather prefer 'scheme://user@host' in all cases; it is shorter and more to the point.

        I hope you fix this soon.

        In my opinion, when the credentials of the user were not in the URI but have been given using the Authentication in the FileSystemOptions the username should also be in the FileName.toString(), which is currently not the case.

        Show
        Frank van der Kleij added a comment - - edited I think too that the password should never ever be revealed once entered; not in exceptions, nor in other debug messages. Probably adapting the AbstractFileName.toString to not print the password would fix most of this. Then the method getFriendlyURI can be deprecated as well. I have one remark though. I really dislike the format 'scheme://user:***@host' where the password is hidden with *** ; it is kind of ugly and I don't think it serves any purpose to show the ***. I would rather prefer 'scheme://user@host' in all cases; it is shorter and more to the point. I hope you fix this soon. In my opinion, when the credentials of the user were not in the URI but have been given using the Authentication in the FileSystemOptions the username should also be in the FileName.toString(), which is currently not the case.
        Hide
        James Carman added a comment -

        "I hope you fix this soon"

        That's the beauty of open source software; you can help fix this! Attaching a patch (including test cases) would be a good way to help get this thing fixed. It seems like you've already got a good idea about how it should be done.

        Show
        James Carman added a comment - "I hope you fix this soon" That's the beauty of open source software; you can help fix this! Attaching a patch (including test cases) would be a good way to help get this thing fixed. It seems like you've already got a good idea about how it should be done.
        Hide
        Frank van der Kleij added a comment -

        This patch is a very local fix, that only changes the toString() method. I would prefer that the password would not be returned on methods like getURI either, but this has big implications on the whole URI parsing. The TODO in DefaultFileSystemManager.resolveName(FileName,String,NameScope) first has to be implemented to allow the URI parser to work on relative paths.

        Show
        Frank van der Kleij added a comment - This patch is a very local fix, that only changes the toString() method. I would prefer that the password would not be returned on methods like getURI either, but this has big implications on the whole URI parsing. The TODO in DefaultFileSystemManager.resolveName(FileName,String,NameScope) first has to be implemented to allow the URI parser to work on relative paths.
        Hide
        Joerg Schaible added a comment -

        My original concern have been only the URLs in the FileSystemException. That's what I've committed now - the info passed to the exception will be examined for URLs with password and passwords will be masked automatically. However, Frank's approach and patch is also reasonable - it depends on what we want. Main concern with his solution is the overridden toString method of the GenericFileName, because it changes silently the API. This method is normally inherited from AbstractFileName and the Javadoc states explicitly that toString is a call to getURI. Now, since we seem to head for 2.0 with the next release anyway, we might consider such a change. But in this case I would rather change AbstractFileName's toString method and call getFriendlyURI there as default.

        Please comment, if we decide to take the friendly URI approach, we will reopen this issue.

        Show
        Joerg Schaible added a comment - My original concern have been only the URLs in the FileSystemException. That's what I've committed now - the info passed to the exception will be examined for URLs with password and passwords will be masked automatically. However, Frank's approach and patch is also reasonable - it depends on what we want. Main concern with his solution is the overridden toString method of the GenericFileName, because it changes silently the API. This method is normally inherited from AbstractFileName and the Javadoc states explicitly that toString is a call to getURI. Now, since we seem to head for 2.0 with the next release anyway, we might consider such a change. But in this case I would rather change AbstractFileName's toString method and call getFriendlyURI there as default. Please comment, if we decide to take the friendly URI approach, we will reopen this issue.
        Hide
        Sergey Vladimirov added a comment -

        Joerg,

        Seems we have duplication of functionality now:

        • GenericFileName has getSafeURI() method
        • FileName & AbstractFileName have getFriendlyURI() method

        As for me, only getFriendlyURI() was okay (may be using *** to replace password should be added), and this (and only this) method should be used for toString() method.

        Also AbstractFileObject should be changed to use "safe" URI in toString()

        Show
        Sergey Vladimirov added a comment - Joerg, Seems we have duplication of functionality now: GenericFileName has getSafeURI() method FileName & AbstractFileName have getFriendlyURI() method As for me, only getFriendlyURI() was okay (may be using *** to replace password should be added), and this (and only this) method should be used for toString() method. Also AbstractFileObject should be changed to use "safe" URI in toString()
        Hide
        Joerg Schaible added a comment -

        Seems we have duplication of functionality now

        ?? As I said, I did not apply Frank's patch, I've chosen a complete different approach.

        Also AbstractFileObject should be changed to use "safe" URI in toString()

        This is the question. While it seems reasonable, we change silently the API. I'd be definitely -1 for a next 1.x release, but since we're heading to 2.x, it might be a different case.

        Show
        Joerg Schaible added a comment - Seems we have duplication of functionality now ?? As I said, I did not apply Frank's patch, I've chosen a complete different approach. Also AbstractFileObject should be changed to use "safe" URI in toString() This is the question. While it seems reasonable, we change silently the API. I'd be definitely -1 for a next 1.x release, but since we're heading to 2.x, it might be a different case.
        Hide
        Sergey Vladimirov added a comment -

        Sorry, Joerg, didn't check SVN, only patch file. Let me review it

        Show
        Sergey Vladimirov added a comment - Sorry, Joerg, didn't check SVN, only patch file. Let me review it
        Hide
        Sergey Vladimirov added a comment -

        Joerg,

        1) Personally, i completly unlike masking password in exception. It's not the place where it should be done. Looks like a hack for me.

        There is a lot of other places where passowrd inside of URI should be masked. For example - debug output of my own program (FileName.toString() or FileObject.toString() calls). They also should be masked.

        2) What about API? We already renamed (in SVN) version number to 2.0-SNAPSHOT, and, AFAIK, we can't change it back (some commercial projects already using snapshots under 2.x numbering)

        Anyway, if the question is only saving FileObject clean of additional methods, for 1.x version we can introduce getFriendlyURI() method in AbstractFileName and not to copy it to FileObject (do it only in 2.x).

        Show
        Sergey Vladimirov added a comment - Joerg, 1) Personally, i completly unlike masking password in exception. It's not the place where it should be done. Looks like a hack for me. There is a lot of other places where passowrd inside of URI should be masked. For example - debug output of my own program (FileName.toString() or FileObject.toString() calls). They also should be masked. 2) What about API? We already renamed (in SVN) version number to 2.0-SNAPSHOT, and, AFAIK, we can't change it back (some commercial projects already using snapshots under 2.x numbering) Anyway, if the question is only saving FileObject clean of additional methods, for 1.x version we can introduce getFriendlyURI() method in AbstractFileName and not to copy it to FileObject (do it only in 2.x).

          People

          • Assignee:
            Joerg Schaible
            Reporter:
            Joerg Schaible
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development