directory-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Karasulu" <akaras...@apache.org>
Subject Re: Server does not allow anymore simple password
Date Sun, 03 Jun 2007 16:10:19 GMT
Enrique,

Also note that you/we could have avoided this issue if we made sure that
each feature branch
only has changes relating to one feature.  I think one of the reasons why I
missed this change
is because the branch was riddled with many different features.  I was
focusing on the SASL
feature specifically and missed this password policy change.

Overall I think we all need to adhere to a policy where if we need a review
to take place on a
feature branch that all must collaborate on, then we must be strict with
ourselves by keeping
changes in that branch specific to the purpose of that branch.

So if we have a SASL branch we only do SASL changes.  If ancilliary changes
are needed to accomplish this task then we can make those changes in another
branch or in the trunk keeping the view of changes in the SASL branch
specific.  Thinking about the config and password policy changes
specifically.

Alex

On 6/3/07, Alex Karasulu <akarasulu@apache.org> wrote:
>
> Comments in-line ...
>
> On 6/3/07, Emmanuel Lecharny <elecharny@apache.org> wrote:
> >
> > Hi,
> >
> > since the last addition of the PasswordPolicyService interceptor in
> > trunk, the server does not accept anymore passwords which are supposly
> > violating a policy : "Password violates policy:  insufficient character
> > mix".
>
>
> This should not be the default behavior and furthermore any change that
> effects default
> behavior out of the box must be discussed beforehand on this list.  Just
> think about the
> issues we had with the small fix to the BasicAttributes(true) change a
> little while ago.
>
> I was very surprised when I first got this message, and I had to dig
> > into the code to find the rationals.
> >
> > What I found in the code didn't pleased me, and I think this code should
> > not have quit a branch to be injected to the trunk.
> >
> > This was directly related to last Enrique merge.
> >
> > Here are some samples of what I have found :
> >
> >     void check( String username, String password ) throws
> > NamingException
> >     {
> >         int passwordLength = 6;
> >         int categoryCount = 2;
> >         int tokenSize = 3;
> >
> >         if ( !isValid( username, password, passwordLength,
> > categoryCount, tokenSize ) )
> >         {
> >             String explanation = buildErrorMessage( username, password,
> > passwordLength, categoryCount, tokenSize );
> >             log.error( explanation );
> >
> >             throw new NamingException( explanation );
> >         }
> >     }
> >
> > Such constants deep in the code is a sure way to get stuck when using
> > the server. I'm pretty sure that it was just intended for testing
> > purpose, but even then, it should not have been written this way.
> >
> >             else if ( attr instanceof byte[] )
> >             {
> >                 String string = StringTools.utf8ToString( ( byte[] )
> > attr
> > );
> >
> >                 StringBuffer sb = new StringBuffer();
> >                  sb.append( "'" + string + "' ( " );
> >                 sb.append( StringTools.dumpBytes( ( byte[] ) attr
> > ).trim() );
> >                 log.debug( "Adding Attribute id : 'userPassword',
> > Values : [ " + sb.toString() + " ) ]" );
> >
> >                 userPassword = string;
> >             }
> >
> > This is another example of what should not be done : there is no reason
> > for debug specific code to be found in the normal flow. Moreoever, I
> > have told Enrique the day before to not using such a pattern.
> >
> > Now, beside those pieces of code, there is something much more important
> > : it changes the way the server works, and nobody has been warned about
> > it. Not only it has been committed as is, but the configuration has been
> > changed so that this code is now active.
>
>
> Yes this is a very serious issue.  As I said above we cannot change the
> default behavior of
> the server without discussing the impact of such changes on this mailing
> list.
>
> I have checked the commit logs, and I found another explaination for a
> > problem I encoutered lately, but as I didn't had time to dig it, I just
> > commented some lines in the server.xml file :
> >
> >     <!-- limits searches by non-admin users to a max time of
> > 15000          -->
> >     <!-- milliseconds and has a default value of
> > 10000                      -->
> >     <property name="maxTimeLimit" value="0" />
> >
> >     <!-- limits searches to max size of 1000 entries: default value is
> > 100  -->
> >     <property name="maxSizeLimit" value="100000" />
> >
> > Those two values was simply not accepted because the related members
> > have been removed from the StartupConfiguration class. I can't imagine
> > that it has been unnoticed before committing the code, or I guess that
> > *no* integration tests has been done, or that the server has never been
> > launch, because launching the server with such a configuration leads to
> > a direct crash in one second.
>
>
> Yes this is probably due to inadequate testing or a presumption that the
> configuration
> works in one environment without testing other environments.
>
> This is also a direct result of the merge.
> >
> > This is simply not the way we should work. I have spent time to
> > understand what was happening, and time to write this mail with all the
> > necessary checks to be sure I was not misunderstanding what has been
> > done. I would have liked to spend this time on something else more
> > usefull.
>
>
> I have kept quiet while dealing with merge issues in the trunk.  I know
> the changes
> introduced were complex and involved.  Nothing ever goes smoothly on big
> change
> merges however they could have been smoother than it was with this
> particular merge.
>
> There were several gross presumptions that were made which cost a number
> of us
> several hours while trying to decipher what is happening while having
> tests fail with
> these JVM issues with the crypto introductions.   These problems could
> have easily
> been avoided by following some simple protocol.
>
> I said nothing because I did not want to upset you Enrique but you have to
> make
> some effort to consider the fallout of these kinds of changes.  You need
> to lookout
> for your fellow committers by:
>
> (1) thoroughly testing your changes
> (2) not presuming everyone has the same environment and testing different
> stock JVMs
> (3) reporting and discussing each behavior altering change on this ML
> before merging it
>
> The trunk is a serious place where we have exposure.  This is why we work
> on feature
> branches where we need to shield others from inconsistent states during
> the course of
> introducing a feature.  Once you decide to merge though you need to make
> sure all your
> i's are dotted and your t's are crossed.
>
> I know I missed these changes while reviewing your branch however I did
> not think
> you would take it upon yourself to enable these features by default in the
> server.  Many people
> depend on certain behaviors and before we change them we need to discuss
> the changes
> and give users a heads up.
>
> Please consider others and have some doubt your ability to make final
> decision to
> introduce such default functionality.  This doubt should not make you stop
> but should
> make you ask some questions or open up some conversations.  To give you
> credit
> I already see you doing this and I commend you on it (internal vs.
> external user convo).
>
> Please keep it up and try to understand our point of view for avoiding
> these kinds of changes
> in the future.
>
> Enrique, do you see where we are coming from with our concerns?
>
> Alex
>
>
>

Mime
View raw message