Alon Bar-Lev has posted comments on this change.
Change subject: core: WIP: Add suppoort for SSL in LDAP.
......................................................................
Patch Set 2: (7 inline comments)
Hi,
This is complex change indeed, this is why it is so important to do this
properly... so I am sorry for the comments, just know that I have same comments
for the current code.
What we need to do is to SEPARATE entirely between the channel encryption and
authentication, and have the configuration of channel/authentication be
available for every domain we add.
It means that there must be a significant change in how code is implemented.
The extra abstraction of the code is a PROBLEM not a SOLUTION, best is to
eliminate it completely, based on the example I sent you.
Pseudo code, I am copying from my example:
Hashtable<String, String> env = new Hashtable<String, String>()
profile = getDomainProfile(name)
boolean doTLS = url.startsWith("ldaps:");
env.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory");
env.put(Context.PROVIDER_URL, profile.url);
if (doTLS) {
env.put(Context.SECURITY_PROTOCOL, profile.securityProtocol);
}
_ctx = new InitialLdapContext(env, null);
if (!doTLS && profile.doStartTLS) {
_tls = (StartTlsResponse)_ctx.extendedOperation(
new StartTlsRequest()
);
}
...
if (_user != null) {
if (profile.doSASL_digestMD5) {
_ctx.addToEnvironment(Context.SECURITY_AUTHENTICATION, "DIGEST-MD5");
}
if (profile.doGSSAPI) {
...
}
_ctx.addToEnvironment(Context.SECURITY_PRINCIPAL, _user);
_ctx.addToEnvironment(Context.SECURITY_CREDENTIALS, _password);
}
...
At the end you have a valid ldap context hand to operative classes.
If you really like you can abstract the entire thing, but every abstraction
should get the ldap environment and ldap context and modify it, that's it.
The result code will be much simpler than what we currently have.
Thank you.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/LDAPSecurityAuthentication.java
Line 1: package org.ovirt.engine.core.bll.adbroker;
Line 2:
Line 3: public enum LDAPSecurityAuthentication {
Line 4: SIMPLE, SIMPLE_SSL, GSSAPI;
No need for SIMPLE_SSL.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/LDAPSecurityProtocol.java
Line 1: package org.ovirt.engine.core.bll.adbroker;
Line 2:
Line 3: public enum LDAPSecurityProtocol {
Line 4: SSL;
LDAPProtocol not security.
Should be plain, ssl, startTLS
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/LDAPTemplateWrapperFactory.java
Line 19: private static Log log =
LogFactory.getLog(LDAPTemplateWrapperFactory.class);
Line 20:
Line 21: static {
Line 22: registerClass(LDAPSecurityAuthentication.GSSAPI,
GSSAPILdapTemplateWrapper.class);
Line 23: if ("SSL".equals(LDAPSecurityProtocol.SSL)) {
Not sure I understand why not to register only the selected
channel/authentication based on configuration.
This is static context, so it means that whatever you are doing here effects
all domains, which is not something that should be.
Also I think that "SSL" will always equals to LDAPSecurityProtocol.SSL or I
don't understand the condition above.
Line 24: registerClass(LDAPSecurityAuthentication.SIMPLE,
SslSimpleLdapTemplateWrapper.class);
Line 25: } else {
Line 26: registerClass(LDAPSecurityAuthentication.SIMPLE,
SimpleLdapTemplateWrapper.class);
Line 27: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/SslSimpleDirContextAuthenticationStrategy.java
Line 11: private static final String SSL_PROTOCOL = "ssl";
Line 12:
Line 13: @Override
Line 14: public void setupEnvironment(Hashtable env, String userDn, String
password) {
Line 15: super.setupEnvironment(env, userDn, password);
Again, I think that we need to separate between the channel and the
authentication.
Line 16: env.put(Context.SECURITY_PROTOCOL, SSL_PROTOCOL);
Line 17:
Line 18: }
Line 19:
Line 12:
Line 13: @Override
Line 14: public void setupEnvironment(Hashtable env, String userDn, String
password) {
Line 15: super.setupEnvironment(env, userDn, password);
Line 16: env.put(Context.SECURITY_PROTOCOL, SSL_PROTOCOL);
We need to support startTLS as well, and certificate validation.
Line 17:
Line 18: }
Line 19:
Line 20:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/SslSimpleLdapTemplateWrapper.java
Line 20:
Line 21: @Override
Line 22: public void search(String baseDN, String filter, String
displayFilter, SearchControls searchControls, NameClassPairCallbackHandler
handler) {
Line 23: pagedSearch(baseDN,filter, displayFilter, searchControls,
handler);
Line 24: }
This should not be different than standard LDAP.
Line 25:
Line 26: @Override
Line 27: protected DirContextAuthenticationStrategy
buildContextAuthenticationStategy() {
Line 28: return new SslSimpleDirContextAuthenticationStrategy();
Line 32: protected void setCredentialsOnContext() {
Line 33: contextSource.setUserDn(userName);
Line 34: contextSource.setPassword(password);
Line 35:
Line 36: }
Same.
Line 37:
Line 38: @Override
Line 39: public void adjustUserName(LdapProviderType ldapProviderType) {
Line 40: // No manipulation on user name is required,
--
To view, visit http://gerrit.ovirt.org/10898
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I31acf3142ca8cffe8f9174545ee8421ec243644a
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Sharad Mishra <[email protected]>
Gerrit-Reviewer: Adam Litke <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Oved Ourfali <[email protected]>
Gerrit-Reviewer: Sharad Mishra <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
