Juan Hernandez has posted comments on this change.
Change subject: [WIP] Add LDAP directory
......................................................................
Patch Set 16:
(7 comments)
....................................................
File
backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/kerberos/KerberosPasswordAuthenticatorFactory.java
Line 37: KerberosConfigurationFile conf =
KerberosConfigurationFile.getInstance();
Line 38:
Line 39: // Populate the libdefaults section of the Kerberos
configuration:
Line 40: KerberosConfigurationFile.Section defaultsSection =
conf.getSection(DEFAULTS_SECTION);
Line 41: // TODO - fix this, it breaks an RFE of ovirt 3.2 that
requested control if to use the dns or not
It does't break it because the user can decide to use its own Kerberos
configuration file instead of letting the system generate it. For example:
kerberos.file=/etc/mykrb5.conf
If this parameter is given then the authenticator will use that file and ignore
completely what is provided here.
Also there is the user can override these parameters to put whatever is
required in the generated Kerberos configuration files:
kerberos.defaults.dns_lookup_realm=true
Line 42: defaultsSection.setRelation("dns_lookup_realm", "false");
Line 43: defaultsSection.setRelation("dns_lookup_kdc", "false");
Line 44: defaultsSection.setRelation("ticket_lifetime", "24h");
Line 45: defaultsSection.setRelation("renew_lifetime", "7d");
....................................................
File
backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/ldap/LdapContextFactory.java
Line 83:
Line 84: /**
Line 85: * Responsible for performing a JAAS login
Line 86: */
Line 87: private class LoginContextBuilder {
This class is already very long. Can you move this outside, to its own file?
Line 88:
Line 89: private volatile LoginContext ctx;
Line 90:
Line 91: public LoginContext getLoginContext() throws LoginException {
Line 121: ctx.login();
Line 122: }
Line 123: }
Line 124:
Line 125: private class UriBuilder {
Same for this, can you move it to its own class?
Line 126: private volatile List<String> uris = null;
Line 127:
Line 128: public List<String> getUrisList() {
Line 129: if (uris == null) {
Line 420: // that it represents:
Line 421: // TODO - remove this before merge
Line 422: System.setProperty("ENGINE_ETC",
"/home/yzaslavs/ovirt-engine/etc/ovirt-engine");
Line 423: // TODO - change back to System.getenv
Line 424: String engineEtc = System.getProperty("ENGINE_ETC");
I tried to avoid any dependencies on the "engine" from the authentication
mechanism, even if the dependency is just the "ENGINE" word in the name. Try to
use the KerberosConfiguationFile class.
Line 425: if (engineEtc == null) {
Line 426: engineEtc = "/etc/ovirt-engine";
Line 427: }
Line 428: File krb5File = new File(engineEtc, "krb5.conf");
Line 433: System.setProperty("java.security.krb5.conf",
Line 434: krb5File.getAbsolutePath());
Line 435: } else {
Line 436: log.error("Failed loading kerberos setting. File " +
krb5File + " not found.");
Line 437: }
Here you can use the KerberosConfigurationFile class, in a similiar way to how
it is used in KerberosPasswordAuthenticatorFactory.
Line 438: System.setProperty("sun.security.krb5.msinterop.kstring",
"true");
Line 439: Object result = Subject.doAs(
Line 440: getKerberosTicket(),
Line 441: new PrivilegedAction<Object>() {
Line 434: krb5File.getAbsolutePath());
Line 435: } else {
Line 436: log.error("Failed loading kerberos setting. File " +
krb5File + " not found.");
Line 437: }
Line 438: System.setProperty("sun.security.krb5.msinterop.kstring",
"true");
Why is this property needed? What if the Kerberos server isn't Windows?
Line 439: Object result = Subject.doAs(
Line 440: getKerberosTicket(),
Line 441: new PrivilegedAction<Object>() {
Line 442: @Override
....................................................
File
backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/ldap/LdapDirectory.java
Line 131: public DirectoryUser findUser(ExternalId id) {
Line 132: LdapContext ctx = null;
Line 133: try {
Line 134: // Create the LDAP context:
Line 135: // TODO: improve creation speed - maybe hold the context
alive and not close it?
One of the things I wanted to do is to use a pool of contexts, that is why the
context factory is a separate class.
Line 136: // TODO: if holding the context alive - there should be a
check in case of SASL+GSSAPI if the kerberos
Line 137: // ticket
Line 138: // is still alive
Line 139: ctx = contextFactory.create();
--
To view, visit http://gerrit.ovirt.org/22386
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8d1883145a86a4f338cd17f2d36c150e67ab653
Gerrit-PatchSet: 16
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
