This looks fine , with the caveat of tested to work of course, which you have already stated. Just a couple of minor things, and then a conditional ACK 1. In CMSEngine: this bloc: if (tag.equals("internaldb")) { authType = config.getString("internaldb.ldapauth.authtype", "BasicAuth"); @@ -382,8 +384,35 @@ public class CMSEngine implements ICMSEngine { binddn = config.getString("ca.publish.ldappublish.ldap.ldapauth.bindDN"); } else { - // ignore any others for now - continue; + /* + * This section assumes a generic format of + * <prefix>.ldap.xxx + * where <prefix> is specified under the tag substore + * + * e.g. if tag = "externalLDAP" + * cms.passwordlist=...,externalLDAP + * externalLDAP.prefix=auths.instance.UserDirEnrollment + * + * auths.instance.UserDirEnrollment.ldap.ldapauth.authtype=BasicAuth + * auths.instance.UserDirEnrollment.ldap.ldapauth.bindDN=cn=Corporate Directory Manager + * auths.instance.UserDirEnrollment.ldap.ldapauth.bindPWPrompt=externalLDAP + * auths.instance.UserDirEnrollment.ldap.ldapconn.host=host.example.com + * auths.instance.UserDirEnrollment.ldap.ldapconn.port=389 + * auths.instance.UserDirEnrollment.ldap.ldapconn.secureConn=false + */ + String prefix = config.getString(tag + ".prefix"); + System.out.println("CMSEngine.initializePasswordStore(): prefix=" + prefix); + authType = config.getString(prefix +".ldap.ldapauth.authtype", "BasicAuth"); + System.out.println("CMSEngine.initializePasswordStore(): authType " + authType); + if (!authType.equals("BasicAuth")) + continue; In the else clause could we short circuit processing earlier if we find something we don't like for instance: String prefix = config.getString(tag + ".prefix"); No need to go on if that fails. The same for the rest of the values checked. 2. Can we rename "prefix" to something more friendly to the user like "auths-prefix" to it is clearer to the user what the exact purpose of that setting is. ----- Original Message ----- > From: "Christina Fu" <cfu(a)redhat.com&gt; > To: "pki-devel" <pki-devel(a)redhat.com&gt; > Sent: Wednesday, August 5, 2015 4:43:16 PM > Subject: [Pki-devel] [PATCH] pki-cfu-0090-Ticket-1531-Directory-auth-plugin-requires-LDAP-anon.patch > > This patch is for ticket > https://fedorahosted.org/pki/ticket/1531 Directory auth plugin requires > LDAP anonymous binds > > This patch adds a feature to allow a directory based authentication > plugin > to use bound ldap conneciton instead of anonymous. > Two files need to be edited > 1. <instance>/conf/password.conf > add a "tag" and the password of the binding user dn to the file > e.g. externalLDAP=password123 > 2. <instance>/ca/CS.cfg > add the tag to cms.passwordlist: > e.g. cms.passwordlist=internaldb,replicationdb,externalLDAP > add the prefix of the auths entry for the authentication instance > e.g. externalLDAP.prefix=auths.instance.UserDirEnrollment > add relevant entries to the authenticaiton instance > e.g. auths.instance.UserDirEnrollment.ldap.ldapBoundConn=true > auths.instance.UserDirEnrollment.ldap.ldapauth.authtype=BasicAuth > auths.instance.UserDirEnrollment.ldap.ldapauth.bindDN=uid=rhcs,ou=serviceaccounts,dc=EXAMPLE,dc=com > auths.instance.UserDirEnrollment.ldap.ldapauth.bindPWPrompt=externalLDAP > > The code has been tested to work. > The code (in its plugin form) has also been tested to work successfully > with an ldap server that has its anonymous bind turned off. > > thanks, > Christina > > _______________________________________________ > Pki-devel mailing list > Pki-devel(a)redhat.com > https://www.redhat.com/mailman/listinfo/pki-devel

After the fixes and some further discussion over the connection issue being resolved: ACK ----- Original Message ----- From: "Christina Fu" <cfu(a)redhat.com&gt; To: "pki-devel" <pki-devel(a)redhat.com&gt; Sent: Thursday, August 6, 2015 5:51:03 PM Subject: Re: [Pki-devel] [PATCH] pki-cfu-0090-Ticket-1531-Directory-auth-plugin-requires-LDAP-anon.patch Updated per jack's suggestion. Also, during testing, one issue was discovered where a failed authentication would cause the next one to fail. Investigation shows that a bad connection gets recycled back to the pool and somehow the underlying connection framework does not seem to clear it out. My solution was to just disconnect the bad connection once it's determined that it's botched, before it is returned back to the pool. That seems to reset it and works well now. Since this extra disconnect code needs to go into all authentication plugins that extends the DirBasedAuthentication, I have to modify all four of them to do the disconnect in case of ldap authentication failure. thanks, Christina On 08/05/2015 05:57 PM, John Magne wrote: > This looks fine , with the caveat of tested to work of course, > which you have already stated. > > Just a couple of minor things, and then a conditional ACK > > 1. In CMSEngine: this bloc: > > if (tag.equals("internaldb")) { > authType = config.getString("internaldb.ldapauth.authtype", "BasicAuth"); > @@ -382,8 +384,35 @@ public class CMSEngine implements ICMSEngine { > binddn = config.getString("ca.publish.ldappublish.ldap.ldapauth.bindDN"); > > } else { > - // ignore any others for now > - continue; > + /* > + * This section assumes a generic format of > + * <prefix>.ldap.xxx > + * where <prefix> is specified under the tag substore > + * > + * e.g. if tag = "externalLDAP" > + * cms.passwordlist=...,externalLDAP > + * externalLDAP.prefix=auths.instance.UserDirEnrollment > + * > + * auths.instance.UserDirEnrollment.ldap.ldapauth.authtype=BasicAuth > + * auths.instance.UserDirEnrollment.ldap.ldapauth.bindDN=cn=Corporate Directory Manager > + * auths.instance.UserDirEnrollment.ldap.ldapauth.bindPWPrompt=externalLDAP > + * auths.instance.UserDirEnrollment.ldap.ldapconn.host=host.example.com > + * auths.instance.UserDirEnrollment.ldap.ldapconn.port=389 > + * auths.instance.UserDirEnrollment.ldap.ldapconn.secureConn=false > + */ > + String prefix = config.getString(tag + ".prefix"); > + System.out.println("CMSEngine.initializePasswordStore(): prefix=" + prefix); > + authType = config.getString(prefix +".ldap.ldapauth.authtype", "BasicAuth"); > + System.out.println("CMSEngine.initializePasswordStore(): authType " + authType); > + if (!authType.equals("BasicAuth")) > + continue; > > > In the else clause could we short circuit processing earlier if we find something we don't like for instance: > > String prefix = config.getString(tag + ".prefix"); > > No need to go on if that fails. The same for the rest of the values checked. > > > > 2. Can we rename "prefix" to something more friendly to the user like "auths-prefix" to it is clearer to the user > what the exact purpose of that setting is. > > > > > > ----- Original Message ----- >> From: "Christina Fu" <cfu(a)redhat.com&gt; >> To: "pki-devel" <pki-devel(a)redhat.com&gt; >> Sent: Wednesday, August 5, 2015 4:43:16 PM >> Subject: [Pki-devel] [PATCH] pki-cfu-0090-Ticket-1531-Directory-auth-plugin-requires-LDAP-anon.patch >> >> This patch is for ticket >> https://fedorahosted.org/pki/ticket/1531 Directory auth plugin requires >> LDAP anonymous binds >> >> This patch adds a feature to allow a directory based authentication >> plugin >> to use bound ldap conneciton instead of anonymous. >> Two files need to be edited >> 1. <instance>/conf/password.conf >> add a "tag" and the password of the binding user dn to the file >> e.g. externalLDAP=password123 >> 2. <instance>/ca/CS.cfg >> add the tag to cms.passwordlist: >> e.g. cms.passwordlist=internaldb,replicationdb,externalLDAP >> add the prefix of the auths entry for the authentication instance >> e.g. externalLDAP.prefix=auths.instance.UserDirEnrollment >> add relevant entries to the authenticaiton instance >> e.g. auths.instance.UserDirEnrollment.ldap.ldapBoundConn=true >> auths.instance.UserDirEnrollment.ldap.ldapauth.authtype=BasicAuth >> auths.instance.UserDirEnrollment.ldap.ldapauth.bindDN=uid=rhcs,ou=serviceaccounts,dc=EXAMPLE,dc=com >> auths.instance.UserDirEnrollment.ldap.ldapauth.bindPWPrompt=externalLDAP >> >> The code has been tested to work. >> The code (in its plugin form) has also been tested to work successfully >> with an ldap server that has its anonymous bind turned off. >> >> thanks, >> Christina >> >> _______________________________________________ >> Pki-devel mailing list >> Pki-devel(a)redhat.com >> https://www.redhat.com/mailman/listinfo/pki-devel _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel