Re: [Pki-devel] [PATCH] pki-cfu-0090-Ticket-1531-Directory-auth-plugin-requires-LDAP-anon.patch

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