Hi,
On Thursday, 8. September 2011, Graham Barr wrote:
> On Sep 7, 2011, at 09:26 , Peter Marschall wrote:
> > * 0.43 has a serious regression: commit 041d540 broke start_tls
> > completely
> >
> > and led to warnings being generated at every encrypted connection with
> > sslverify='none'.
> > This is fixed by commit a3c4f7f "un-break certificate verification"
> >
> > BTW: this commit does The Right Thing(tm) and obsoletes commit 4dc845e
> > "Verify hostnames in TLS connections" in the next branch.
>
> Chris, as that commit is yours. Do you agree with this comment ?
Here are additional arguments:
* setting SSL_verifycn_scheme => 'ldap' (as it was done unconditionally in
0.43) automatically does a hostname verification
* commit a3c4f7f makes sure that this does not happen when sslverify => 'none'
avoiding that a warning shows up on every LDAPS connection
* for the sslverify => { 'require' | 'optional' } cases commit a3c4f7f
makes sure the host name is set, hence unbreaking start_tls()
* IMHO there's already an option to configure hostname verification: 'sslverify'
Setting it to 'none' will disable hostname verficiation, in the other cases
host names are checked automatically.
There's no need for another option doing the same thing
* IMHO not v erifying the host names in the 'optional' and 'require' cases
was a bug and a security risk. We should fix it instead of adding options
to continue keeping the users at risk.
Anyone wanting the old behaviour back, can set sslverify => 'none'
NOTE:
1) if someone switched to 0.43 they are bitten already
even in the sslverify => 'none' case
2) the only ones affected by this issue are those connecting to a
server where the hostname in the certificate does not match
the host name of the server they connect to.
In the non-matching cases, other clients that adhere to the standard
were complaining already.
So the number of these cases should be fairly low.
(if the host names match, then the new behaviour is identical to
the old one: everything works without a problem)
* IMHO the option name 'check' is too generic.
* if I understand the connection / start_tls code correctly,
* in the LDAPS case there will be no connection if host name verification
fails. Hence there's no ned to disconnect.
* in the start_tls() case, it's a bit different
Here dissconnecting after a failed IO::socket::SSL->start_SSL
might be an option.
In my checks, any succeeding communication in the same session failed,
but closing the connection explicitly might make it clearer for the users.
* IMHO disconnect() is too generic too.
In the remaining case above, it should be changed so something like:
_drop_conn($self, LDAP_USER_CANCELED, "start_tls() failed");
Best regards
Peter
--
Peter Marschall
pe...@adpm.de
