New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIFI-5442 Get X-ProxyContextPath value from request attributes rather… #2908

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@alopresto
Contributor

alopresto commented Jul 18, 2018

… than directly from headers.

Thank you for submitting a contribution to Apache NiFi.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced
    in the commit message?

  • Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

  • Has your PR been rebased against the latest commit within the target branch (typically master)?

  • Is your initial contribution a single, squashed commit?

For code changes:

  • Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
  • Have you written or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
  • If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
  • If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

@alopresto

This comment has been minimized.

Contributor

alopresto commented Jul 18, 2018

Verified that the curl request in the ticket is no longer successful.

@danfike

This comment has been minimized.

danfike commented Jul 18, 2018

In addition to verifying the curl in the original ticket is no longer successful, did you also verify that a curl request which should be successful indeed still is? I ask because I thought the attribute contextPath was not actually assigned to anything at this point.

@alopresto

This comment has been minimized.

Contributor

alopresto commented Jul 18, 2018

@danfike I don't have an instance running right now which is set up with a proxy. I can run a remote debugger and verify an artificial proxy path in the response. What is an example of what you would expect the outcome to be?

@alopresto

This comment has been minimized.

Contributor

alopresto commented Jul 19, 2018

I can observe that passing a valid (whitelisted in nifi.properties: nifi.web.proxy.context.path=some/path) but unused X-ProxyContextPath is handled fine, while passing a malicious one X-ProxyContextPath: /nifi/assets/reset.css/reset.css\" type=\"text/css\" /><script type=\"text/javascript\">alert(\"omg\");</script><link rel=\"stylesheet\" href=\" results in the expected error and is logged to the nifi-app.log file.

@alopresto

This comment has been minimized.

Contributor

alopresto commented Jul 19, 2018

"Safe" (whitelisted) but inaccurate context path:
screen shot 2018-07-18 at 5 13 17 pm

Malicious context path:
screen shot 2018-07-18 at 5 13 37 pm

@danfike

This comment has been minimized.

danfike commented Jul 19, 2018

I actually also don't have an instance set up with a proxy either. I've just been directly poking a simple unsecured NiFi instance (without a proxy) using curl

I guess I'd expect it to work like index.jsp. Consider my simple unsecured NiFi instance with no proxy set up. If I hit http://hostname/ (without a /nifi path), I get a simple page advising I probably meant to add /nifi to the end (index.jsp). You can see this easily enough with curl http://hostname/. Note the stylesheet paths all start with /nifi/....

If I add --header "X-ProxyContextPath: foo" to my curl (note that I'm still hitting NiFi directly; there is no actual proxy in place), you'll notice there is no change to the stylesheet paths unless I "whitelist" the path foo in nifi.properties at nifi.web.proxy.context.path. In that case, we'd see the request response references stylesheet paths starting with /foo/nifi/... This is what I'd expect given https://nifi.apache.org/docs/nifi-docs/html/administration-guide.html#proxy_configuration

I'd expect requests that use message-page.jsp to do something similar. I've been using curl to GET /nifi-api/access/oidc/callback on my unsecured cluster with no proxy in front. It is just an easy way to land at the Message Page and get it to render to test this behavior. I'd expect it to behave as above.

(I'd embed the full shell text illustrating this but I don't have that available at this moment).

I poked around this issue a bit earlier today. I thought I tried exactly your fix (on top of 1.6.0 stable) and discovered that all message page renders were failing because of setAttribute("contextPath") never being called in this case. So no matter whether the header was absent, present and safe, present and whitelisted, or present and unsafe, the requests all failed.

It looked to me like we'd need to add something to

that is similar to
String contextPath = getSanitizedContextPath(request);
request.setAttribute("contextPath", contextPath);

But I'm not sure. And perhaps there are other references to the message page that this wouldn't address.

@alopresto

This comment has been minimized.

Contributor

alopresto commented Jul 19, 2018

Ok, I think I understand. When I do a remote debug, the CatchAllFilter is executing, but I believe because all of the requests I was trying would end in failure or an expected outcome, the message-page.jsp wasn't getting rendered. In that case, yes, I believe we need a similar interceptor for that response page. I'll push another commit tomorrow.

NIFI-5442 Populate request contextPath attribute during AccessResourc…
…e before displaying on message-page.jsp.

Refactored shared code from CatchAllFilter to WebUtils.
@alopresto

This comment has been minimized.

Contributor

alopresto commented Jul 24, 2018

Hi @danfike ; I was away from my computer for a few days but should have the fix for what you described. Please verify that the new commit does what you expect. Thanks.

@danfike

This comment has been minimized.

danfike commented Jul 25, 2018

@alopresto I'm not able to validate right now, but @patwhitey2007 has agreed to take a look for me.

@alopresto

This comment has been minimized.

Contributor

alopresto commented Jul 25, 2018

I believe there are some other locations in the code that forward requests to /message rather than .../message-page.jsp, so I am performing a refactor where either the CatchAllFilter or a service/utility method for the sanitization will be moved to nifi-web-utils and then both nifi-web-ui and nifi-web-error will consume this and register (the single filter or a filter per URL) in their respective web.xml files.

This PR is not ready to be merged.

@alopresto

This comment has been minimized.

Contributor

alopresto commented Jul 26, 2018

This PR is now ready for final review.

@danfike

This comment has been minimized.

danfike commented Jul 27, 2018

Trying again to @mention Pat, whose ID I got wrong earlier, @mcg30005

@mcg30005

This comment has been minimized.

mcg30005 commented Jul 27, 2018

Thanks very much @alopresto and @danfike, and apologies Dan, my bad with the id info.

Andy, thanks again for looking at this, much appreciated. i will try to get a review in today.

@mcg30005

This comment has been minimized.

mcg30005 commented Jul 30, 2018

+1
Hi Andy, PR looks really good, i asked Shawna to take a look as well since she has way more experience in this area. One question that we had was what page render will do with empty init str, we're going to patch and try this.

@mcg30005

This comment has been minimized.

mcg30005 commented Jul 31, 2018

+1
do see travis-ci build passing

@mcgilman

This comment has been minimized.

Contributor

mcgilman commented Jul 31, 2018

Thanks for the PR! I'm happy to help get this merged in. I tried out the proposed patch locally and it does address the issue in the case identified. However, I tried to run in some of the other context and the filter was not running as expected. Adding a dispatcher type of FORWARD on the SanitizeContextPathFilter resolved the issue locally. With this update I'll be a +1.

@alopresto

This comment has been minimized.

Contributor

alopresto commented Jul 31, 2018

Thanks Matt. Made the change locally and verifying before pushing. This seems to be a change in how the filters were invoked from the 2.3 spec, so thanks for identifying it.

NIFI-5442 Added explicit dispatchers to nifi-web-ui web.xml and remov…
…ed unnecessary code from AccessResource.
@mcgilman

This comment has been minimized.

Contributor

mcgilman commented Jul 31, 2018

@alopresto I'm a +1 with the addition of the most recent commit. I'll wait until tomorrow to merge it in just in case anyone else engaged here [@danfike @mcg30005] wants to check it out.

@asfgit asfgit closed this in e62aa02 Aug 1, 2018

@mcgilman

This comment has been minimized.

Contributor

mcgilman commented Aug 1, 2018

Thanks again for the PR and reviews! This has been merged to master.

@mcg30005

This comment has been minimized.

mcg30005 commented Aug 1, 2018

Thanks a lot folks, much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment