You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directory.apache.org by Emmanuel Lecharny <el...@apache.org> on 2007/06/03 17:26:29 UTC
Server does not allow anymore simple password
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".
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.
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.
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.
As a result, I have reverted the server.xml to its original form, and
reinjected the missing methods in the StartupConfiguration class.
Emmanuel
Re: Server does not allow anymore simple password
Posted by Emmanuel Lecharny <el...@apache.org>.
Enrique Rodriguez a écrit :
<snip/>
>
>
> This is a new "best practice" for me and I will strive to use it in
> the future. Your comments last month on my bad debugging practices
> are legitimate and I will begin to review code this month to improve
> this situation for existing commits and to make sure any new commits
> do not threaten the performance of the server.
Ok, thanks. Listen, it was just the last thing I saw while I was trying
to understand what happened with the server. It is not exactly the most
important point, but it was the last which hit my face. I understand you
forgot to check this, as you committed a lot of code.
My job is certainly not to teach you how to code, I'm not good enough
for that. I don't think you have bad debugging practices, and on the
opposite I think that your code is above the average. I was just a
little bit upset...
Emmanuel
Re: Server does not allow anymore simple password
Posted by Enrique Rodriguez <en...@gmail.com>.
On 6/3/07, Emmanuel Lecharny <el...@apache.org> wrote:
> ...
> 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".
You have several good points about the new PasswordPolicyService. I
am sorry I committed the server.xml with the interceptors enabled and
you are correct to disable them.
> ...
> void check( String username, String password ) throws NamingException
> {
> int passwordLength = 6;
> int categoryCount = 2;
> int tokenSize = 3;
I was unsure how to pass configuration to interceptors but I believe I
now understand how to do so. I will update this in the
PasswordPolicyService interceptor.
> ...
> 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.
This is a new "best practice" for me and I will strive to use it in
the future. Your comments last month on my bad debugging practices
are legitimate and I will begin to review code this month to improve
this situation for existing commits and to make sure any new commits
do not threaten the performance of the server.
FWIW, the PasswordPolicyService has an integration test in server-unit
called 'PasswordPolicyServiceITest'.
Enrique
Re: Server does not allow anymore simple password
Posted by Emmanuel Lecharny <el...@apache.org>.
Enrique Rodriguez a écrit :
> I have to split up this thread to respond to it, since you covered a
> couple points. Comments below.
>
> On 6/3/07, Emmanuel Lecharny <el...@apache.org> wrote:
>
>> ...
>> 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.
>
>
> The two parameters you felt were missing are, in fact, only ever used
> by the SearchHandler.
Do I have to remind you that LDAP is all about searching ?
> Therefore, in the split of core configuration
> to LdapConfiguration I moved them to LdapConfiguration. These
> parameters are in the new config doco at:
>
> http://cwiki.apache.org/confluence/display/DIRxSBOX/LDAP+Protocol+Configuration
>
>
> I apologize if you think this change wasn't warranted but it does
> reflect how the server is designed.
I just tried to launch the server, and it immediatly failed. That's why
I wrote this mail. It's not a design question, it's all about user
expecting a stable behaviour. We have had this discussion back in march
about this configuration modification :
Emmanuel Lécharny a écrit :
>>Enrique Rodriguez wrote :
>>> On 3/15/07, Alex Karasulu <ak...@apache.org> wrote:
>>> Yeah I agree with Emmanuel. Enrique let's leave the configuration
as it is
>>> for now.
>>>
>>> You are after all just trying to add the SASL stuff and getting
bogged down
>>> with configuration.
>> What just happened to LdapConfiguration? It seemed like an easy part
>> of the SASL initiative. Emmanuel brought up good points, and I'm fine
>> with putting the LdapConfiguration bean under
>> ServerStartupConfiguration.
>
> This is the perfect place right now. We will reconsider this later, I
think, because as you said, the actual config does not help > end users.
But anyway, the urgent things is SASL, not configuration refactoring.
I don't think we agreed on modifying the configuration, so far. It was
clearly the opposite, for a simple reason : let's go on step by step.
And the result is that I get trapped one month and a half after with a
problem we anticipated.
> The fact that there is no unit
> test is bad but there wasn't one before this move. I think these
> parameters belong on LdapConfiguration and that your change should be
> reverted. The parameters you added back to StartupConfiguration only
> satisfy your attempt to configure server.xml and they are unused
> during runtime.
I just reverted to a configuration which should have never been changed,
as we stated it was not urgent. You committed it anyway. I hope you
simply forgot about what we agreed on.
> I know you are busy and that this change came at a bad time, but in
> Eclipse a quick "right-click | References | Workspace" will show that
> the LdapConfiguration#getMaxSizeLimit and
> LdapConfiguration#getMaxTimeLimit are being used by the SearchHandler.
This is not the point. We are all busy, but we can deal with it, unless
we have unexpected modifications which were done without agreement. The
LdapConfiguration modification has been postponed, at least this is what
I remembered, so I kept the previous configuration as it was, except
your SASL specific configuration.
Ok, now, one very important thing : this new proposed configuration has
been evaluated, and we think it's a good thing. We agreed on that.
Changing it straight without notification, in within a bunch of SASL
code, was a bad move, because we focused on SASL (which is working well,
btw), and totally forgot about LdapConfiguration modification. That plus
the other problems I faced at the same time were a little bit to much
for me to handle, so my mail.
I really want you to be *cautious* when adding new code, just to be sure
that this does not happen again. We could perfectly have commit those
LdapConfiguration changes a week later, smoothly, without all those
problems. I would have been warned, and I would have changed my
server.xml without wondering where the problem is coming from.
Let's just work together, interact more, and doing baby steps, for the
good of community. The way it worked for SASL was just perfect. We have
had many interactions, we also have discussions about the chain pattern
which was quite interesting, I really appreciated it. And it was
*really* good to have SASL into the server, you did a good job on this,
Enrique. I wish we can work the same way for every part of the server,
that's all we need.
Thanks,
Emmanuel
Re: Server does not allow anymore simple password
Posted by Enrique Rodriguez <en...@gmail.com>.
I have to split up this thread to respond to it, since you covered a
couple points. Comments below.
On 6/3/07, Emmanuel Lecharny <el...@apache.org> wrote:
> ...
> 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.
The two parameters you felt were missing are, in fact, only ever used
by the SearchHandler. Therefore, in the split of core configuration
to LdapConfiguration I moved them to LdapConfiguration. These
parameters are in the new config doco at:
http://cwiki.apache.org/confluence/display/DIRxSBOX/LDAP+Protocol+Configuration
I apologize if you think this change wasn't warranted but it does
reflect how the server is designed. The fact that there is no unit
test is bad but there wasn't one before this move. I think these
parameters belong on LdapConfiguration and that your change should be
reverted. The parameters you added back to StartupConfiguration only
satisfy your attempt to configure server.xml and they are unused
during runtime.
> ...
> 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.
>
> As a result, I have reverted the server.xml to its original form, and
> reinjected the missing methods in the StartupConfiguration class.
I know you are busy and that this change came at a bad time, but in
Eclipse a quick "right-click | References | Workspace" will show that
the LdapConfiguration#getMaxSizeLimit and
LdapConfiguration#getMaxTimeLimit are being used by the SearchHandler.
Enrique
Re: Server does not allow anymore simple password
Posted by Enrique Rodriguez <en...@gmail.com>.
On 6/3/07, Alex Karasulu <ak...@apache.org> wrote:
> ...
> 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.
Regarding the config, I discussed the changes in my response to
Emmanuel's original email. I feel this was a legitimate part of the
changes to make SASL configurable by creating a config bean specific
to the LDAP protocol.
FWIW, password policy was not part of the SASL branch, but rather the
branch for enhancements to Kerberos encryption. My thinking was that
strong encryption is useless without password policy, but I agree that
it was a change that should not have been part of the merge to trunk.
At least, the interceptor should not have been inserted in the
server-main server.xml. Luckily, it is an interceptor and Emmanuel
was right to remove its xml stanza.
For future reference, we have a JIRA issue for password policy, still
targeting 1.5.2, at:
https://issues.apache.org/jira/browse/DIRSERVER-899
Enrique
Re: Server does not allow anymore simple password
Posted by Alex Karasulu <ak...@apache.org>.
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 <ak...@apache.org> wrote:
>
> Comments in-line ...
>
> On 6/3/07, Emmanuel Lecharny <el...@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
>
>
>
Re: Server does not allow anymore simple password
Posted by Alex Karasulu <ak...@apache.org>.
Comments in-line ...
On 6/3/07, Emmanuel Lecharny <el...@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