Alon Bar-Lev has posted comments on this change. Change subject: aaa: Remove dependency at builtin on Common config ......................................................................
Patch Set 8: (11 comments) Looks good! Some comments/questions. http://gerrit.ovirt.org/#/c/27607/8/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapAuthn.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapAuthn.java: Line 47: Line 48: private void doInit(ExtMap inputMap, ExtMap outputMap) { Line 49: context = inputMap.<ExtMap> get(Base.InvokeKeys.CONTEXT); Line 50: configuration = context.<Properties> get(Base.ContextKeys.CONFIGURATION); Line 51: Utils.putDefaultsIfAbsent(configuration); set defaults will be better name? Line 52: broker = LdapFactory.getInstance(getAuthzName()); Line 53: context.<List<String>> get( Line 54: Base.ContextKeys.CONFIGURATION_SENSITIVE_KEYS Line 55: ).add("config.authn.user.password"); Line 74: Base.ContextKeys.BUILD_INTERFACE_VERSION, Line 75: Base.INTERFACE_VERSION_CURRENT); Line 76: Line 77: try { Line 78: Utils.handleApplicationInit(System.getProperty("application.name")); why in system property and not global context? Line 79: } catch (Exception e) { Line 80: outputMap.mput( Line 81: Base.InvokeKeys.MESSAGE, Line 82: e.getMessage() http://gerrit.ovirt.org/#/c/27607/8/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosManager.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosManager.java: Line 36: if (!krb5File.exists()) { Line 37: String msg = String.format("Failed loading kerberos settings from File %1$s.", krb5File.getAbsolutePath()); Line 38: log.error(msg); Line 39: throw new RuntimeException(msg); Line 40: } else { no need for else at positive flow Line 41: System.setProperty("java.security.krb5.conf", krb5File.getAbsolutePath()); Line 42: } Line 43: System.setProperty("sun.security.krb5.msinterop.kstring", "true"); Line 44: } http://gerrit.ovirt.org/#/c/27607/8/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/Utils.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/Utils.java: Line 21: public static void putDefaultsIfAbsent(Properties conf) { Line 22: { Line 23: Properties defaults = new Properties(); Line 24: try { Line 25: defaults.load(Utils.class.getResourceAsStream("/builtin/defaults.properties")); shouldn't this be fqdn of our name space? Line 26: for (Map.Entry<Object, Object> entry : defaults.entrySet()) { Line 27: putIfAbsent(conf, (String) entry.getKey(), (String) entry.getValue()); Line 28: } Line 29: Line 39: } Line 40: } Line 41: Line 42: public static void handleApplicationInit(String applicationName) throws Exception { Line 43: if (firstCall.compareAndSet(true, false)) { shouldn't something here should be synchronized? Line 44: if (applicationName.equals(Base.ApplicationNames.OVIRT_ENGINE_AAA_EXTENSION_TOOL)) { Line 45: if (System.getProperty("java.security.auth.login.config") == null) { Line 46: InputStream stream = Utils.class.getResourceAsStream("/extensions-tester/jaas.conf"); Line 47: File file = File.createTempFile("jaas", "conf"); Line 42: public static void handleApplicationInit(String applicationName) throws Exception { Line 43: if (firstCall.compareAndSet(true, false)) { Line 44: if (applicationName.equals(Base.ApplicationNames.OVIRT_ENGINE_AAA_EXTENSION_TOOL)) { Line 45: if (System.getProperty("java.security.auth.login.config") == null) { Line 46: InputStream stream = Utils.class.getResourceAsStream("/extensions-tester/jaas.conf"); same Line 47: File file = File.createTempFile("jaas", "conf"); Line 48: Files.copy(stream, file.toPath(), StandardCopyOption.REPLACE_EXISTING); Line 49: System.setProperty("java.security.auth.login.config", file.getAbsolutePath()); Line 50: } Line 43: if (firstCall.compareAndSet(true, false)) { Line 44: if (applicationName.equals(Base.ApplicationNames.OVIRT_ENGINE_AAA_EXTENSION_TOOL)) { Line 45: if (System.getProperty("java.security.auth.login.config") == null) { Line 46: InputStream stream = Utils.class.getResourceAsStream("/extensions-tester/jaas.conf"); Line 47: File file = File.createTempFile("jaas", "conf"); when do you delete this? Line 48: Files.copy(stream, file.toPath(), StandardCopyOption.REPLACE_EXISTING); Line 49: System.setProperty("java.security.auth.login.config", file.getAbsolutePath()); Line 50: } Line 51: Configuration.getConfiguration().refresh(); Line 60: if (!krb5File.exists()) { Line 61: throw new Exception(String.format("Failed loading kerberos settings from File %1$s.", Line 62: krb5File.getAbsolutePath())); Line 63: } else { Line 64: System.setProperty("java.security.krb5.conf", krb5File.getAbsolutePath()); I think we should move this directive to engine's config file... why should we set it at runtime? then move this into the tool section, just like the jaas. Line 65: } Line 66: Configuration.getConfiguration().refresh(); Line 67: DirectoryManager.setObjectFactoryBuilder(new DirectoryContextFactoryBuilder()); Line 68: } http://gerrit.ovirt.org/#/c/27607/8/backend/manager/modules/builtin-extensions/src/main/resources/builtin/defaults.properties File backend/manager/modules/builtin-extensions/src/main/resources/builtin/defaults.properties: Line 1: config.LdapServers= Line 2: config.LDAPServerPort=389 Line 3: config.AdUserName= Line 4: config.AdUserPassword= Line 5: config.LDAPSecurityAuthentication=GSSAPI can you please add the jaas context name per[1]? [1] http://gerrit.ovirt.org/#/c/27811/ Line 6: config.AuthenticationMethod=LDAP Line 7: config.LDAPProviderTypes= Line 8: config.LDAPQueryTimeout=30 Line 9: config.DomainName= http://gerrit.ovirt.org/#/c/27607/8/backend/manager/modules/builtin-extensions/src/main/resources/extensions-tester/jaas.conf File backend/manager/modules/builtin-extensions/src/main/resources/extensions-tester/jaas.conf: Line 1: EngineKerberosAuth { Line 2: com.sun.security.auth.module.Krb5LoginModule required client=TRUE; Line 3: }; Line 4: you probably do not need this file http://gerrit.ovirt.org/#/c/27607/8/backend/manager/tools/src/main/resources/extensions-tester/jaas.conf File backend/manager/tools/src/main/resources/extensions-tester/jaas.conf: Line 1: EngineKerberosAuth { Line 2: com.sun.security.auth.module.Krb5LoginModule required client=TRUE; Line 3: }; so you don't need it now, right? -- To view, visit http://gerrit.ovirt.org/27607 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1384a99f73ab605b61bce8dcdfd63e222b0001fa Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
