Alon Bar-Lev has posted comments on this change. Change subject: aaa: Add support for rest api Basic auth ......................................................................
Patch Set 1: (15 comments) http://gerrit.ovirt.org/#/c/37786/1/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SSORestApiLoginFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SSORestApiLoginFilter.java: Line 50: } Line 51: ticketDecoder = new TicketDecoder(ks, EngineLocalConfig.getInstance().getSsoStoreEku()); Line 52: } catch (Exception e) { Line 53: throw new RuntimeException("Unable to instantiate TicketDecoder", e); Line 54: } this code should go into aaa utilities or something... just return instance of ticket utils. maybe in engine messy design, even hold static ticket decoder within the utils class... but this is really ugly! Line 55: } Line 56: Line 57: @Override Line 58: public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, Line 76: String trustManagerAlgorithm = TrustManagerFactory.getDefaultAlgorithm(); Line 77: String trustStore = EngineLocalConfig.getInstance().getProperty("ENGINE_PKI_TRUST_STORE"); Line 78: String trustStoreType = KeyStore.getDefaultType(); Line 79: String trustStorePassword = EngineLocalConfig.getInstance().getPKITrustStorePassword(); Line 80: Integer readTimeout = 0; you can really avoid these temp variables... it does not add readability. Line 81: return new HttpURLConnectionBuilder(url).setHttpsProtocol(httpsProtocol) Line 82: .setReadTimeout(readTimeout) Line 83: .setTrustManagerAlgorithm(trustManagerAlgorithm) Line 84: .setTrustStore(trustStore) Line 103: private void authenticateWithSSO(HttpServletRequest req) throws ServletException { Line 104: String headerAuthorization = req.getHeader(FiltersHelper.Constants.HEADER_AUTHORIZATION); Line 105: HttpURLConnection connection = null; Line 106: try { Line 107: connection = create(new URL("https://127.0.0.1/ovirt-engine/sso/login-restapi";)); problem.... 1. using ssl will force trusting apache certificate... we do not trust it correctly. 2. not using ssl in localhost will enable local sniffers to see password, but local sniffers requires root anyway... so not that important. maybe the solution will be to open a new port, directly to jboss using https, bypassing apache, as this issue is similar to the serial console issue as well. for now, I suggest to use localhost and plain http, although we can enable https by configuration, and when we do, the trust store should probably be something else than .truststore. is there any way to do this within jboss? something like forward between context? Line 108: connection.setDoInput(true); Line 109: connection.setDoOutput(true); Line 110: connection.setRequestMethod("POST"); Line 111: connection.setRequestProperty(FiltersHelper.Constants.HEADER_AUTHORIZATION, headerAuthorization); Line 109: connection.setDoOutput(true); Line 110: connection.setRequestMethod("POST"); Line 111: connection.setRequestProperty(FiltersHelper.Constants.HEADER_AUTHORIZATION, headerAuthorization); Line 112: connection.setRequestProperty("Content-Type", "application/x-www-form-urlencoded"); Line 113: connection.setRequestProperty("Content-Length", "" + Integer.toString(headerAuthorization.getBytes().length)); not sure why content length is considering header. Line 114: connection.setRequestProperty("Content-Language", "en-US"); Line 115: ByteArrayOutputStream os = new ByteArrayOutputStream(); Line 116: copy(connection.getInputStream(), os); Line 117: connection.connect(); Line 111: connection.setRequestProperty(FiltersHelper.Constants.HEADER_AUTHORIZATION, headerAuthorization); Line 112: connection.setRequestProperty("Content-Type", "application/x-www-form-urlencoded"); Line 113: connection.setRequestProperty("Content-Length", "" + Integer.toString(headerAuthorization.getBytes().length)); Line 114: connection.setRequestProperty("Content-Language", "en-US"); Line 115: ByteArrayOutputStream os = new ByteArrayOutputStream(); try ()? Line 116: copy(connection.getInputStream(), os); Line 117: connection.connect(); Line 118: createUserSession(req, new String(os.toByteArray(), "UTF-8")); Line 119: } catch (Exception e) { Line 112: connection.setRequestProperty("Content-Type", "application/x-www-form-urlencoded"); Line 113: connection.setRequestProperty("Content-Length", "" + Integer.toString(headerAuthorization.getBytes().length)); Line 114: connection.setRequestProperty("Content-Language", "en-US"); Line 115: ByteArrayOutputStream os = new ByteArrayOutputStream(); Line 116: copy(connection.getInputStream(), os); try()? Line 117: connection.connect(); Line 118: createUserSession(req, new String(os.toByteArray(), "UTF-8")); Line 119: } catch (Exception e) { Line 120: log.error("Exception obtaining ticket from sso", e.getMessage()); http://gerrit.ovirt.org/#/c/37786/1/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/RestApiLoginServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/RestApiLoginServlet.java: Line 9: import javax.servlet.http.HttpServletRequest; Line 10: import javax.servlet.http.HttpServletResponse; Line 11: import java.io.IOException; Line 12: Line 13: public class RestApiLoginServlet extends HttpServlet { why restapi? it should be generic to all cases. Line 14: Line 15: private static final long serialVersionUID = 4414518331391974859L; Line 16: Line 17: @Override Line 16: Line 17: @Override Line 18: protected void service(HttpServletRequest request, HttpServletResponse response) Line 19: throws ServletException, IOException { Line 20: Credentials credentials = SSOUtils.getUserCredentialsFromHeader(request); you can move this statement into the try block. Line 21: try { Line 22: if (credentials != null) { Line 23: AuthenticationUtils.handleCredentials( Line 24: request.getSession(true), Line 25: credentials.getUsername(), Line 26: credentials.getPassword(), Line 27: credentials.getProfile()); Line 28: response.getOutputStream().write(SSOUtils.issueTicket(request.getSession(true), request).getBytes("UTF-8")); Line 29: response.getOutputStream().flush(); why do you need flush? won't it better to: try (OutputStream os = response.getOutputStream()) { } Line 30: } else { Line 31: response.sendError(HttpServletResponse.SC_UNAUTHORIZED); Line 32: } Line 33: } catch (Exception e) { Line 27: credentials.getProfile()); Line 28: response.getOutputStream().write(SSOUtils.issueTicket(request.getSession(true), request).getBytes("UTF-8")); Line 29: response.getOutputStream().flush(); Line 30: } else { Line 31: response.sendError(HttpServletResponse.SC_UNAUTHORIZED); please always put trivial path first in conditionals. Line 32: } Line 33: } catch (Exception e) { Line 34: response.sendError(HttpServletResponse.SC_UNAUTHORIZED); Line 35: } Line 29: response.getOutputStream().flush(); Line 30: } else { Line 31: response.sendError(HttpServletResponse.SC_UNAUTHORIZED); Line 32: } Line 33: } catch (Exception e) { please log exception (debug trace, error message) Line 34: response.sendError(HttpServletResponse.SC_UNAUTHORIZED); Line 35: } Line 36: } Line 37: http://gerrit.ovirt.org/#/c/37786/1/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOContextListener.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOContextListener.java: Line 64: } Line 65: KeyStore.PrivateKeyEntry entry = (KeyStore.PrivateKeyEntry) ks.getEntry( Line 66: getStringInitParam(ctx, localConfig, SSO_STORE_ALIAS), Line 67: new KeyStore.PasswordProtection(passwd.toCharArray())); Line 68: return new TicketEncoder(entry.getCertificate(), entry.getPrivateKey(), 60); why 60? this ticket should be valid for seconds. anyway, once you add these constants, please delegate into configuration, this belongs to parent patches. Line 69: } Line 70: Line 71: private String getStringInitParam(ServletContext ctx, SSOLocalConfig localConfig, String name) { Line 72: String r = ctx.getInitParameter(name); http://gerrit.ovirt.org/#/c/37786/1/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUtils.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUtils.java: Line 68: throw new RuntimeException(ex); Line 69: } Line 70: } Line 71: Line 72: public static String issueTicket(HttpSession session, you can move this split into parent patches, ot make this one smaller. Line 73: HttpServletRequest request) throws SQLException, IOException, GeneralSecurityException { Line 74: SSOConfig config = (SSOConfig) session.getServletContext().getAttribute(SSO_CONFIG); Line 75: String authzName = (String) session.getAttribute(SSOUtils.SSO_AUTHZ_ATTR_NAME); Line 76: ExtMap principalRecord = (ExtMap) session.getAttribute(SSOUtils.SSO_PRINCIPAL_RECORD_ATTR_NAME); http://gerrit.ovirt.org/#/c/37786/1/backend/manager/modules/enginesso/src/main/webapp/WEB-INF/web.xml File backend/manager/modules/enginesso/src/main/webapp/WEB-INF/web.xml: Line 195: Line 196: <servlet-mapping> Line 197: <servlet-name>RestApiLoginServlet</servlet-name> Line 198: <url-pattern>/login-restapi</url-pattern> Line 199: </servlet-mapping> login-credentials or something like that, no reason to make it restapi specific at sso side. Line 200: Line 201: <servlet> Line 202: <servlet-name>LogoutServlet</servlet-name> Line 203: <servlet-class>org.ovirt.engine.core.sso.servlets.LogoutServlet</servlet-class> http://gerrit.ovirt.org/#/c/37786/1/backend/manager/modules/restapi/webapp/src/main/webapp/WEB-INF/jboss-deployment-structure.xml File backend/manager/modules/restapi/webapp/src/main/webapp/WEB-INF/jboss-deployment-structure.xml: Line 34: <module name="org.ovirt.engine.api.restapi-definition" annotations="true" services="import"/> Line 35: <module name="org.ovirt.engine.api.restapi-jaxrs" annotations="true" services="import"/> Line 36: <module name="org.ovirt.engine.api.restapi-types" annotations="true" services="import"/> Line 37: <module name="org.ovirt.engine.core.aaa"/> Line 38: <module name="org.ovirt.engine.api.ovirt-engine-extensions-api"/> I am unsure why this is required and was not required before. Line 39: Line 40: </dependencies> Line 41: </deployment> Line 42: -- To view, visit http://gerrit.ovirt.org/37786 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib5f6975f2d306a4dc2d81b795ab4905e5d3281a1 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@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
