Ravi Nori has posted comments on this change. Change subject: aaa: Add Bearer and Negotiate auth filters ......................................................................
Patch Set 3: (9 comments) https://gerrit.ovirt.org/#/c/42292/3/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/SSOOAuthServiceUtils.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/SSOOAuthServiceUtils.java: Line 150 Line 151 Line 152 Line 153 Line 154 > I do not think it is good to have relative based on request, it opens a hol using SSO_URL=${ENGINE_URI}/sso removed support for relative path in code Line 171 Line 172 Line 173 Line 174 Line 175 > we provably need these two as parameters as well. added two new parameters to ovirt-engine.conf.in SSO_CONNECTION_PROTOCOL=TLSv1 SSO_CONNECTION_VERIFY_HOST=true https://gerrit.ovirt.org/#/c/42292/3/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SSORestApiBaseFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SSORestApiBaseFilter.java: Line 39: public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, Line 40: ServletException { Line 41: HttpServletRequest req = (HttpServletRequest) request; Line 42: if (!FiltersHelper.isAuthenticated(req) || !FiltersHelper.isSessionValid((HttpServletRequest) request, (HttpServletResponse) response)) { Line 43: authenticateWithSSO(req, (HttpServletResponse) response); > I do not understand how this can be called over and over until nego is esta It is conditionally called after checking the headers and only called if not already authenticated. Removing BaseFilter and adding SSOUtils to make it clearer. Line 44: } Line 45: chain.doFilter(request, response); Line 46: } Line 47: Line 41: } Line 42: Line 43: protected abstract void authenticateWithSSO(HttpServletRequest req, HttpServletResponse res) throws ServletException; Line 44: Line 45: protected void createUserSession(HttpServletRequest req, Map<String, Object> jsonResponse) { > instead of using inheritance, you can use this function as a utility. Done Line 46: if (!FiltersHelper.isStatusOk(jsonResponse)) { Line 47: throw new RuntimeException((String) jsonResponse.get("MESSAGE")); Line 48: } Line 49: InitialContext ctx = null; https://gerrit.ovirt.org/#/c/42292/3/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SSORestApiBasicAuthFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SSORestApiBasicAuthFilter.java: Line 21: Map<String, Object> response = SSOOAuthServiceUtils.authenticate(req, res, scope); Line 22: FiltersHelper.isStatusOk(response); Line 23: createUserSession(req, FiltersHelper.getPayloadForToken(req, res, (String) response.get("access_token"))); Line 24: } catch (Exception e) { Line 25: log.error(e.getMessage()); > "Cannot authenticate using Basic headers: {}" Done Line 26: log.debug("Authentication with SSO failed", e); Line 27: } Line 28: } Line 29: } https://gerrit.ovirt.org/#/c/42292/3/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SSORestApiBearerAuthFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SSORestApiBearerAuthFilter.java: Line 17: String headerValue = req.getHeader(FiltersHelper.Constants.HEADER_AUTHORIZATION); Line 18: if (headerValue != null && headerValue.startsWith("Bearer ")) { Line 19: try { Line 20: createUserSession(req, FiltersHelper.getPayloadForToken(req, res, Line 21: new String(Base64.decodeBase64(headerValue.substring("Bearer".length())), Charset.forName("UTF-8")) > I still do not understand why you decode the token or expect user to encode removed decode Line 22: )); Line 23: } catch (Exception e) { Line 24: log.error(e.getMessage()); Line 25: log.debug("Bearer Authentication with SSO failed", e); Line 18: if (headerValue != null && headerValue.startsWith("Bearer ")) { Line 19: try { Line 20: createUserSession(req, FiltersHelper.getPayloadForToken(req, res, Line 21: new String(Base64.decodeBase64(headerValue.substring("Bearer".length())), Charset.forName("UTF-8")) Line 22: )); > I think that these two methods Basic and Bearer can leave within the same f Done Line 23: } catch (Exception e) { Line 24: log.error(e.getMessage()); Line 25: log.debug("Bearer Authentication with SSO failed", e); Line 26: } Line 20: createUserSession(req, FiltersHelper.getPayloadForToken(req, res, Line 21: new String(Base64.decodeBase64(headerValue.substring("Bearer".length())), Charset.forName("UTF-8")) Line 22: )); Line 23: } catch (Exception e) { Line 24: log.error(e.getMessage()); > "Cannot authenticate using Bearer token: {}" Done Line 25: log.debug("Bearer Authentication with SSO failed", e); Line 26: } Line 27: } Line 28: } https://gerrit.ovirt.org/#/c/42292/3/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SSORestApiNegotiationFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SSORestApiNegotiationFilter.java: Line 81: ); Line 82: } Line 83: Line 84: @Override Line 85: protected void authenticateWithSSO(HttpServletRequest req, HttpServletResponse resp) throws ServletException { > please refer to current nego filter and see how it interact with remote. on I do not understand your comments here. this filter first authenticates using authn. Gets username@profile and sends it to sso for loginOnBehalf. Line 86: try { Line 87: req.setAttribute(FiltersHelper.Constants.REQUEST_SCHEMES_KEY, schemes); Line 88: HttpSession session = req.getSession(false); Line 89: Deque<AuthenticationProfile> stack = null; -- To view, visit https://gerrit.ovirt.org/42292 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idee5137430cefa7ca99c047cfd2d550222e5809a Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
