Spring Security
  1. Spring Security
  2. SEC-981

BindAuthenticator swaps read-write and read-only context operations

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Invalid
    • Affects Version/s: 2.0.3
    • Fix Version/s: 3.0.0 M1
    • Component/s: None
    • Labels:
      None
    • Environment:
      all

      Description

      BindAuthenticator's BindWithSpecificDnContextSource class contains these methods:

      public DirContext getReadOnlyContext() throws DataAccessException

      { return ctxFactory.getReadWriteContext(userDn.toString(), password); }

      public DirContext getReadWriteContext() throws DataAccessException

      { return getReadOnlyContext(); }

      The implementation of getReadOnlyContext() should get a read-only context from the context factory, not a read-write context. Vice versa for method getReadWriteContext().

        Activity

        Hide
        Luke Taylor added a comment -

        Both methods use the same implementation to obtain a context with the user's credentials. This is intentional, given that the intention is to bind as that user, and the class is privat to BindAuthenticator so has no impact elsewhere.

        Please explain why you think this is an issue at all, let alone a critical bug.

        Show
        Luke Taylor added a comment - Both methods use the same implementation to obtain a context with the user's credentials. This is intentional, given that the intention is to bind as that user, and the class is privat to BindAuthenticator so has no impact elsewhere. Please explain why you think this is an issue at all, let alone a critical bug.
        Hide
        Jürgen Failenschmid added a comment -

        If a read-only context is requested, then any modify operation with that context should fail. If read-only cannot be supported, then the method should raise an exception.

        Show
        Jürgen Failenschmid added a comment - If a read-only context is requested, then any modify operation with that context should fail. If read-only cannot be supported, then the method should raise an exception.
        Hide
        Luke Taylor added a comment -

        There is nothing anywhere that specifies that this must be the case and in any case it is dependent on how the directory is configured. If you look at Spring LDAP's TransactionAwareContextSourceProxy, the implementation is:

        public DirContext getReadOnlyContext() throws NamingException

        { return getReadWriteContext(); }

        and in AbstractContextSource, both methods have the same behaviour by default. Again, please explain what difference this makes to you, given that it is an internal class which is only used by BindAuthenticator.

        Show
        Luke Taylor added a comment - There is nothing anywhere that specifies that this must be the case and in any case it is dependent on how the directory is configured. If you look at Spring LDAP's TransactionAwareContextSourceProxy, the implementation is: public DirContext getReadOnlyContext() throws NamingException { return getReadWriteContext(); } and in AbstractContextSource, both methods have the same behaviour by default. Again, please explain what difference this makes to you, given that it is an internal class which is only used by BindAuthenticator.
        Hide
        Jürgen Failenschmid added a comment -

        I'm only pointing out improvements. I've configured a context source that checks for read-only access and read-write access (for auditing purposes) and I have to give the Spring Security stuff write access, although it really shouldn't need it.

        Just because other classes are implemented sloppyly as well, doesn't make it right: don't excuse bad (method) behavior by pointing to other bad (method) behavior. I'll deal with the other Spring packages with similar issues separately.

        Having set all of that, I really appreciate the hard work that went into Spring Security!

        Show
        Jürgen Failenschmid added a comment - I'm only pointing out improvements. I've configured a context source that checks for read-only access and read-write access (for auditing purposes) and I have to give the Spring Security stuff write access, although it really shouldn't need it. Just because other classes are implemented sloppyly as well, doesn't make it right: don't excuse bad (method) behavior by pointing to other bad (method) behavior. I'll deal with the other Spring packages with similar issues separately. Having set all of that, I really appreciate the hard work that went into Spring Security!
        Hide
        Luke Taylor added a comment -

        It would not be an improvement. The whole point is that authentication as the user takes place, regardless of whether the operation is read-only or otherwise. In fact it would open a severe security hole if it were changed to call ctxFactory.getReadOnlyContext() - the context would then be authenticated as the manager user (or anonymously).

        The definition of what is a read-only or read-write context is subjective, just as it is for database connections. You don't have to make a special case for read-only operations but you have the option if the framework allows it. I would avoid references to "sloppy" implementations or "bad behaviour" if you are raising the issue again with the Spring LDAP guys.

        Show
        Luke Taylor added a comment - It would not be an improvement. The whole point is that authentication as the user takes place, regardless of whether the operation is read-only or otherwise. In fact it would open a severe security hole if it were changed to call ctxFactory.getReadOnlyContext() - the context would then be authenticated as the manager user (or anonymously). The definition of what is a read-only or read-write context is subjective, just as it is for database connections. You don't have to make a special case for read-only operations but you have the option if the framework allows it. I would avoid references to "sloppy" implementations or "bad behaviour" if you are raising the issue again with the Spring LDAP guys.

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Jürgen Failenschmid
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: