Whitelist

[Voloshkevych]

develop

GitHub Board

Issue link

#11 Issue name

Story link

#11 Story

Code reviewers

• @github_username

Summary of issue

• Summary of issue
• Summary of issue

Summary of change

• Summary of change
• Summary of change

[handicraftsman]

The lack of reverse proxy support is a blocker here.

The other note, related to the way of storing and handling IP addresses, is not critical, but is a good thing to be done.

[handicraftsman]

No. Just no.

Your service might be behind a reverse proxy (especially on Heroku), and then this statement will just return the same set of IP for all users. Adding your IP in the list will then make you ban yourself from accessing this app.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For

https://devcenter.heroku.com/articles/http-routing#heroku-headers

[handicraftsman]

The getRemoteAddr() should only be checked if other means of getting the IP did not work.

[handicraftsman]

Actually, as a TLDR, on Heroku getRemoteAddr() will return IP addresses representing Heroku servers, not the end users.

[handicraftsman]

Forgot to add: make sure you can disable the X-Forwarded-For check, and use request.getRemoteAddr() as the primary source of truth. But only as an application.yaml config value!

The reason for this is when the app is NOT behind a reverse proxy, any value can be put in X-Forwarded-For instead of the real IP.

[handicraftsman]

I don't know, this just looks like it should not be here for long. Or it should be handled differently.

First of all, this: https://ma.ttias.be/theres-more-than-one-way-to-write-an-ip-address/ .
Of course you don't have to support all of the ways to write the IP address. Maybe, depends on what you expect from the user.

Then, in addition to the previous note - IPv4 addresses are 32-bit, making them regular ints (except for they are unsigned, and that's not available natively in Java).

There are also tons of IPv6 addresses. I personally know people who own whole ranges of them. These have even more ways to be written. These are 128-bit numbers though.

Also, while it seems to not be planned in the first milestone, handling subranges also involves working with numbers. And by that I mean bitwise operations, because they are used to define subnetworks.

What I recommend:

1. Sanitize the IP addresses before saving. Format them so that they are in a canonical shape, that is, no matter what is sent by the user, it will always be stored in the same format as other addresses. No need to save them as numbers, will explain below. I am not sure if Java Class Library has tools for that. If not, just use a popular library, such as Apache Commons (by the way it has tools to work with IP addresses).
2. Think about IPv6, some machines even have no IPv4 but have IPv6. That is a rare case for now, but amount of these machines is growing.
3. When you get to implementing subnetworks, it might get to the next shape:
4. Fetch the whole whitelist for the user (not really performant, but doing that on the database side requires deep knowledge of SQL and Postgres extensions to SQL, probably deeper than done in that trigger-based repeatable migration). It's not a real issue because there's a limitation on that list's size.
5. Convert that list into a list of objects, where IP is stored as an int, and subnet is converted to a bit mask.
6. Parse current user's IP into a similar int, and then, using the bitmask, try to match it with the whitelist-side IP pattern. If at least one pattern from the whitelist matches the user's IP, user is allowed.
7. If you decide to use a library for handling IPs, like mentioned above, you might want to use its features instead of doing the bit crunching manually.