Skip to content
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

DS-4074: Only use X-Forwarded-For value from known proxies #2207

Merged
merged 4 commits into from Nov 1, 2018

Conversation

Projects
None yet
3 participants
@tomdesair
Copy link
Contributor

commented Sep 18, 2018

This PR fixes the following problems:

  1. It concentrates all the logic to determine the client IP address into a single class. There was a lot of duplicate code in many different places.
  2. When using useProxies = true, DSpace always trusted the X-Forwarded-For header regardless of the source. This would allow clients that manually set a X-Forwarded-For header value to spoof an IP address. This PR introduces a list of trusted known proxies. DSpace will only use the X-Forwarded-For value if the request originates from a trusted proxy (range).
  3. When Angular developers run the Angular Universal server on their local machine and use an external REST API (e.g. the public DSpace 7 REST API), this currently leads to issues when doing a browser refresh while authenticated:
    1. When you are on the DSpace 7 homepage and you authenticate in the UI, your browser sends a request directly to the external REST API. The REST API then sees the public IP address of your browser and uses this IP address as part of the authentication process to generate a token. This means that the provided token is only valid when coming from your external IP address.
    2. If you do a refresh while authenticated, your browser connects with the NodeJS server running on your local machine. The browser sends the authentication token obtained in step 1 as part of the request. The Angular Universal code sees that your request is coming from localhost. Therefor it puts value 127.0.0.1 in the X-Forwarded-For header when querying the external REST API on your behalf with the provided token.
    3. The REST API receives the authentication token and reads the value from the X-Forwarded-For (the trusted proxy code is not in place). The REST API refuses your token as it seems to be coming from an IP address (127.0.0.1) not matching with the original (external) IP address that requested the token.
    4. When the page has been rendered by Angular Universal and presented in the browser, you seem to be logged out.

Since DSpace 7 will always have a Angular Universal proxy server, I've changed the default value of useProxies to true. I've also preconfigured the list of trusted proxies to be 127.0.0.1 since most instance will run on a single machine. I've added comments explaining what needs to be changed when running the Angular UI on a different server.

@tdonohue

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

@tomdesair : when you get a chance, could you link this up to the ticket(s) it relates to? It looks like interesting work, but I think I'm missing or overlooking the context for these changes. What is this seeking to fix? And, is it work in progress cause you have more improvements to implement, or cause it's not working as expected?

@tomdesair tomdesair force-pushed the tomdesair:Authentication_X-Forwarded-For branch from 7750e43 to bf82b5f Sep 26, 2018

@tomdesair

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

@tdonohue and @abollini I've updated the description above. If Travis is happy, this PR is ready for review.

@tomdesair tomdesair force-pushed the tomdesair:Authentication_X-Forwarded-For branch from bf82b5f to bd464e0 Sep 26, 2018

@tomdesair

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

Travis is happy. Synk had a failure but not related to my changes.

@tdonohue
Copy link
Member

left a comment

The code here is looking great to me. I like the introduction of the new ClientInfoService to provide a centralized place to manage this proxy code. However, I'd like to see two things to move this forward:

  1. @tomdesair , could you add in Unit Tests for the new ClientInfoService class?
  2. I think @artlowel was going to get someone to test the Angular UI against this PR to see if this PR helps to resolve the issues noted here: DSpace/dspace-angular#292 So, ideally, we find a quick tester of this work as well.

Overall though, I really like this direction.

@artlowel

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

@tdonohue I'm sorry, that slipped my mind.

We've tested it now, and it works 👍

@tdonohue tdonohue added this to the 7.0 milestone Oct 10, 2018

@tdonohue

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

@tomdesair : If you could add some Unit/Integration Tests for the new ClientInfoService class, then I think this would be ready to merge! The code is looking good, and we have a test that it works. Just waiting on tests.

@tdonohue

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

@tomdesair : just a reminder that this PR is looking good, but is waiting on Tests. Will you have any time in the near future to create some tests, or should we look for help from others?

tomdesair and others added some commits Oct 29, 2018

@tdonohue

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

@tomdesair : Thanks for adding the requested tests. I just resolved a minor merge conflict in this PR.

Once Travis approves, we should be ready to merge this!

@tdonohue tdonohue changed the title Only use X-Forwarded-For value from known proxies DS-4074: Only use X-Forwarded-For value from known proxies Nov 1, 2018

@tdonohue

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

I realized a ticket didn't yet exist for this change, so I've created one: https://jira.duraspace.org/browse/DS-4074

@tdonohue
Copy link
Member

left a comment

👍 Code looks good to me now. Thanks @tomdesair

@tdonohue

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

I'm merging this as it was tested by the Angular UI team (see @artlowel's comment above) and reviewed by me. So, it's essentially at +2.

@tdonohue tdonohue merged commit 218fdcb into DSpace:master Nov 1, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 29.07%
Details
@tomdesair

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2018

Thanks for helping @tdonohue! I've had a few busy weeks so I was not able to spent much time on DSpace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.