Restore `TRUSTED_PROXIES` behavior for Rails 5 #1690

micahlee · 2020-07-15T13:13:06Z

What does this PR do?

What's changed? Why were these changes made?

This PR re-implements the TRUSTED_PROXIES Rack IP filter for Rails 5. This replaces the implementation previously provided by the conjur-rack gem for Rails 4.

This PR also adds rspec tests that verify the expected request.ip with the given scenarios for REMOTE_ADDR, TRUSTED_PROXIES, and X-Forwarded-For.

How should the reviewer approach this PR, especially if manual tests are required?
Are there relevant screenshots you can add to the PR description?

What ticket does this PR close?

Connected to #1689

Checklists

Change log

The CHANGELOG has been updated, or
This PR does not include user-facing changes and doesn't require a CHANGELOG update

Test coverage

This PR includes new unit and integration tests to go with the code changes, or
The changes in this PR do not require tests

Documentation

Docs (e.g. READMEs) were updated in this PR, and/or there is a follow-on issue to update docs, or
This PR does not require updating any documentation

TODO:

Cleanup fix code
Add tests for the request.ip behavior
Create issue/PR to remove monkey patch in conjur-rack for trusted_proxy?
Issue: Remove Rack:Request#trusted_proxy? monkey patch conjur-rack#21
Update CHANGELOG

spec/request/request_ip_spec.rb

jonahx · 2020-07-16T22:09:31Z

lib/conjur/trusted_proxy_filter.rb

+
+    def env_trusted_proxies
+      @env['TRUSTED_PROXIES']
+    end


I think I'd just inline this.

Yeah... I wish there was a better way to configure code climate for this. Inlining was my default, but code climate didn't like that "@env['TRUSTED_PROXIES'] is called twice."

I'll inline again and add the exception comment.

jonahx

This looks good to me although I'd still like to move unit testing the object directly for the matrix tests, per our previous convo. Were you planning to do that in a different PR?

It's not a blocker though since these tests are still readable and straightforward. Though I really don't like the rspec convention whereby a function call with 3 parameters becomes 3 let statements followed by an include_examples to "call the function". May not be worth the effort of changing. Wdyt?

micahlee · 2020-07-17T13:34:04Z

@jonahx

I'd still like to move unit testing the object directly for the matrix tests, per our previous convo. Were you planning to do that in a different PR?

I'm torn on this. On one hand, I agree that the layers of unit tests would be the more proper way to do this. On the other hand, I do still have concerns about the fidelity of the tests, considering how mutable the ActiveDispatch::Request is as it passes through the Rack/Rails pipeline. I really do want the subject of these tests to be that request pipeline, rather than only the TrustedProxiesFilter. Or to assume too much about what the final state of ActiveDispatch::Request will be after the pipeline executes.

I am willing to explore that refactor, but I do think that's better suited as a follow up PR. The ROI at this moment of that refactor seems low compared to the ROI of finishing the client IP sourcing document (which depends on this regression fix) on these points:

A) These tests do clearly communicate what the expected outcomes are for the inputs we care about.
B) These tests run fast now, so a refactor wouldn't greatly improve test cycle times.
C) We do get a high degree of assurance from these tests regarding the request.ip outcome, given the full Rack request pipeline is exercised.

I created the issue here: #1696. If there is an epic for the overall spec refactor you discussed during our team meeting Wednesday. I think this would be very appropriate in that epic.

Though I really don't like the rspec convention whereby a function call with 3 parameters becomes 3 let statements followed by an include_examples to "call the function"

I agree with you, and I'd like to hear your suggestions for how to do it better.

I considered inlining everything with something like:

request_env = { 'HTTP_AUTHORIZATION' => token_auth_header, 'REMOTE_ADDR' => '4.4.4.4' }
headers = { 'X-Forwarded-For' => '3.3.3.3,5.5.5.2,5.5.5.3'
ENV['TRUSTED_PROXIES'] = '4.4.4.4,5.5.5.0/24'
get '/request_ip', env: request_env, headers: headers
expect(body_hash['ip']).to eq('3.3.3.3')

But this felt overly cumbersome, not very DRY, and made it hard to read the values that mattered.

I considered writing my own helper method in vanilla Ruby to produce the result. Something akin to:
```
expect(request_ip(
  remote_addr: '4.4.4.4', 
  x_forwarded_for: '3.3.3.3,5.5.5.2,5.5.5.3',
  trusted_proxies: '4.4.4.4,5.5.5.0/24'
  )
).to eq('3.3.3.3')
```
But this didn't feel remarkably different in outcome (or LOC) from the lets and include_examples, or in the spirit of rspec. I personally find this easier to read though.
Some of the lets are unnecessary since they are inherited from the containing context, so they could be removed. But I thought it was easier to read with each test's context clearly stated.

What do you think? Do any of those stand out as a clear improvement over the current code? Do you have any other suggestions I haven't considered yet?

alexkalish · 2020-07-17T14:07:12Z

@micahlee: Are you planning on covering the use TRUSTED_PROXIES (including evoke, etc) in depth in your IP related dap-wiki PR? If not, can you file a separate issue to do so? I want ensure that we document this feature well in dap-wiki so that it can get into the docs for 11.7. Thanks!

micahlee · 2020-07-17T14:16:48Z

@alexkalish

Are you planning on covering the use TRUSTED_PROXIES (including evoke, etc) in depth in your IP related dap-wiki PR?

I'm not sure if this PR is where you want this discussion, rather than on the dap-wiki PR, but the existing draft includes this as a reference discussion. Jason's PR includes this in a tutorial/guide format (although I now see he's removed Conjur OSS from his doc).

It would be helpful to know more specifically what gaps there are that need to be filled for allowing this to be officially documented.

You can see my current "to do" list on the PR description here. If Jason does not intend to cover OSS configuration of TRUSTED_PROXIES in his tutorial, I can create an issue for that.

alexkalish · 2020-07-17T14:17:06Z

@micahlee: Actually, I forgot that @jvanderhoof was already working on this PR: https://github.com/cyberark/dap-wiki/pull/87. Maybe that could serve to document this feature. Could you take a look?

jonahx · 2020-07-17T15:11:30Z

@micahlee

Your plan re: the refactor or lack thereof is ok with me. It's good food for thought for future tests either way and eventually it would be nice to get standards out of it, but now is not the time for that.
I prefer both of your alternatives to the let versions. And I think "inherited" lets are especially problematic, and would always take repetition over that.
I like the version with your bespoke helper most. I think it not being "rspec"-ish is a non-issue. In this case it seems to be a badge of honor.

micahlee · 2020-07-20T17:41:16Z

@jonahx , this is still ready for your re-review when you're able. I added one more fixup commit today to re-used a shared method to create the test access token that I added in #1698

I've left the fixups as separate commits to make the updates easier to review. Once you've had a chance to look at them, I'll squash them back down.

Thanks!

spec/request/request_ip_spec.rb

jonahx

Let's add a note at the top of the test file to the effect of your comments here:

https://conjurhq.slack.com/archives/CGTRB6G8G/p1595352318278600?thread_ts=1595346959.269300&cid=CGTRB6G8G

for Rails 5. This replaces the implementation previously provided by the `conjur-rack` gem. This commit also adds rspec tests that verify the expected `request.ip` with the given scenarios for `REMOTE_ADDR`, `TRUSTED_PROXIES`, and `X-Forwarded-For`.

codeclimate · 2020-07-21T21:24:05Z

Code Climate has analyzed commit e115374 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 86.2% (0.1% change).

View more on Code Climate.

micahlee force-pushed the 1689-trusted-proxies-fix branch 3 times, most recently from e5977d3 to 4ab00bf Compare July 16, 2020 01:32

micahlee marked this pull request as ready for review July 16, 2020 01:32

micahlee requested a review from a team as a code owner July 16, 2020 01:32

micahlee changed the title ~~WIP: Restore TRUSTED_PROXIES behavior for Rails 5~~ Restore TRUSTED_PROXIES behavior for Rails 5 Jul 16, 2020

Tovli reviewed Jul 16, 2020

View reviewed changes

spec/request/request_ip_spec.rb Outdated Show resolved Hide resolved

Tovli reviewed Jul 16, 2020

View reviewed changes

spec/request/request_ip_spec.rb Show resolved Hide resolved

micahlee force-pushed the 1689-trusted-proxies-fix branch 2 times, most recently from b513144 to 8fe3269 Compare July 16, 2020 14:40

jonahx reviewed Jul 16, 2020

View reviewed changes

jonahx reviewed Jul 17, 2020

View reviewed changes

micahlee mentioned this pull request Jul 17, 2020

Consider refactoring request IP tests to smaller units under test #1696

Open

micahlee requested a review from jonahx July 17, 2020 16:28

micahlee force-pushed the 1689-trusted-proxies-fix branch 2 times, most recently from d617e05 to 9a932e9 Compare July 20, 2020 17:38

micahlee mentioned this pull request Jul 20, 2020

Remove Rack:Request#trusted_proxy? monkey patch cyberark/conjur-rack#21

Open

jonahx reviewed Jul 21, 2020

View reviewed changes

spec/request/request_ip_spec.rb Show resolved Hide resolved

jonahx reviewed Jul 21, 2020

View reviewed changes

spec/request/request_ip_spec.rb Show resolved Hide resolved

jonahx reviewed Jul 21, 2020

View reviewed changes

spec/request/request_ip_spec.rb Show resolved Hide resolved

jonahx reviewed Jul 21, 2020

View reviewed changes

micahlee force-pushed the 1689-trusted-proxies-fix branch from 6ba43db to ecf75b9 Compare July 21, 2020 17:54

micahlee requested a review from jonahx July 21, 2020 18:17

micahlee force-pushed the 1689-trusted-proxies-fix branch from ecf75b9 to e115374 Compare July 21, 2020 19:17

jonahx approved these changes Jul 21, 2020

View reviewed changes

micahlee merged commit 48f9f2b into master Jul 21, 2020

micahlee deleted the 1689-trusted-proxies-fix branch July 21, 2020 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore `TRUSTED_PROXIES` behavior for Rails 5 #1690

Restore `TRUSTED_PROXIES` behavior for Rails 5 #1690

micahlee commented Jul 15, 2020 •

edited

Loading

jonahx Jul 16, 2020

micahlee Jul 17, 2020

jonahx left a comment

micahlee commented Jul 17, 2020 •

edited

Loading

alexkalish commented Jul 17, 2020

micahlee commented Jul 17, 2020 •

edited

Loading

alexkalish commented Jul 17, 2020

jonahx commented Jul 17, 2020

micahlee commented Jul 20, 2020

jonahx left a comment

codeclimate bot commented Jul 21, 2020

Restore TRUSTED_PROXIES behavior for Rails 5 #1690

Restore TRUSTED_PROXIES behavior for Rails 5 #1690

Conversation

micahlee commented Jul 15, 2020 • edited Loading

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

TODO:

jonahx Jul 16, 2020

Choose a reason for hiding this comment

micahlee Jul 17, 2020

Choose a reason for hiding this comment

jonahx left a comment

Choose a reason for hiding this comment

micahlee commented Jul 17, 2020 • edited Loading

alexkalish commented Jul 17, 2020

micahlee commented Jul 17, 2020 • edited Loading

alexkalish commented Jul 17, 2020

jonahx commented Jul 17, 2020

micahlee commented Jul 20, 2020

jonahx left a comment

Choose a reason for hiding this comment

codeclimate bot commented Jul 21, 2020

Restore `TRUSTED_PROXIES` behavior for Rails 5 #1690

Restore `TRUSTED_PROXIES` behavior for Rails 5 #1690

micahlee commented Jul 15, 2020 •

edited

Loading

micahlee commented Jul 17, 2020 •

edited

Loading

micahlee commented Jul 17, 2020 •

edited

Loading