Alon Bar-Lev has posted comments on this change. Change subject: aaa: Add Bearer and Negotiate auth filters ......................................................................
Patch Set 3: (10 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 hole for security issues. The relative should be based on the context path of application and probably engine fqdn or localhost. This means that you should not pass request all over in this code. But any reason to actually use relative paths? we can always have configuration with fully qualified such as localhost/... or ${ENGINE_FQDN}/... Line 163 Line 164 Line 165 Line 166 Line 167 please accept protocol from parameters or vdc options. Line 171 Line 172 Line 173 Line 174 Line 175 we provably need these two as parameters as well. oh! VerifyHost *MUST* be true, unless this is localhost! because of that it is best to have all as parameters. 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 established. Please go to the current nego filter and see that the nego can interact with the remote. 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. 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: {}" 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 it using base64. 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 filter, they are quite similar and simple. 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: {}" 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. only after auth is established can you go and interact with sso. 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
