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

#1400 Manage multiple patterns for allowed/blocked IPs via Security Options config section #1399

Merged
merged 29 commits into from
Sep 30, 2023

Conversation

Fabman08
Copy link
Contributor

@Fabman08 Fabman08 commented Dec 23, 2020

Closes #1400

Proposed Changes

  • Added IpAddressRange package ( MPL-2.0 License )
  • Manage multiple pattern in order to allow or block IP access (take a look to the Example section of IpAddressRange)
  • Allow allowed Ips to be removed from blocked list via ExcludeAllowedFromBlocked configuration propery in SecurityOptions node
  • Backward compatibility with current SecurityOptions configuration section
  • Added more unit tests about new feature

Description

This feature is designed to allow greater IP management in order to include or exclude a wide IP range via CIDR notation or IP range.
The current patterns managed are the following:

  • single IP: "192.168.1.1"
  • IP Range: "192.168.1.1-192.168.1.250"
  • IP Short Range: "192.168.1.1-250"
  • IP Range with subnet: "192.168.1.0/255.255.255.0"
  • CIDR: "192.168.1.0/24"
  • CIDR for IPv6: "fe80::/10"

The allowed and block list are evaluated on configuration loaded.
The ExcludeAllowedFromBlocked is meant to give the possibility to specify a wide range of blocked IP and allow a sub range of IPs
Default value: false
Missing property in SecurityOptions allowed, it assume default value.

e.g.

"SecurityOptions": {
  "IPBlockedList": [ "192.168.0.0/23" ],
  "IPAllowedList": [ "192.168.0.15", "192.168.1.15" ],
  "ExcludeAllowedFromBlocked": true
}

src/Ocelot/Configuration/SecurityOptions.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/SecurityOptions.cs Outdated Show resolved Hide resolved
@raman-m
Copy link
Member

raman-m commented May 16, 2023

Hi Fabrizio!
What is the issue this PR is related to? Number?

@raman-m raman-m added feature A new feature proposal Proposal for a new functionality in Ocelot waiting Waiting for answer to question or feedback from issue raiser labels May 16, 2023
@raman-m
Copy link
Member

raman-m commented May 16, 2023

@Fabman08
The build 724 is red.
So, you have to fix CI pipeline!

And don't forget to pull latest top commits from base:develop please!

@raman-m raman-m marked this pull request as draft May 16, 2023 08:58
@Fabman08
Copy link
Contributor Author

Hi Fabrizio! What is the issue this PR is related to? Number?

Hi @raman-m
this is the related issue
#1400

@Fabman08 Fabman08 force-pushed the feat/allowedBlockIpPattern branch from 0e90108 to 681952b Compare May 16, 2023 12:51
@Fabman08
Copy link
Contributor Author

@raman-m I've solve the issue, but now the error raised is not about my code...

Scenario: should reload config on change
	Given there is a configuration Ocelot.Configuration.File.FileConfiguration             [Passed] 
	  And given ocelot is running reloading config True                                    [Passed] 
	  And given there is a configuration Ocelot.Configuration.File.FileConfiguration       [Passed] 
	  And given I wait 5000                                                                [Passed] 
	  And then config should be Ocelot.Configuration.File.FileConfiguration                [Failed] [internalConfig.Data.RequestId should be "someOtherKey" but was "initialKey" difference Difference 

Exceptions:
  1. internalConfig.Data.RequestId should be "someOtherKey" but was "initialKey" difference Difference
  at Ocelot.AcceptanceTests.Steps.ThenConfigShouldBe(FileConfiguration fileConfig) in /root/project/test/Ocelot.AcceptanceTests/Steps.cs:line 97

[xUnit.net 00:00:20.99]     Ocelot.AcceptanceTests.ConfigurationReloadTests.should_reload_config_on_change [FAIL]
  Failed Ocelot.AcceptanceTests.ConfigurationReloadTests.should_reload_config_on_change [5 s]
  Error Message:
   Shouldly.ShouldAssertException : internalConfig.Data.RequestId
    should be
"someOtherKey"
    but was
"initialKey"
    difference

https://app.circleci.com/pipelines/github/ThreeMammals/Ocelot/425/workflows/6eb6eaa1-f002-4eb7-aed5-595972907346/jobs/728?invite=true#step-102-5851

Can anyone check the error?

@raman-m
Copy link
Member

raman-m commented May 16, 2023

Can anyone check the error?

OK. Probably this test "should_reload_config_on_change" is unstable. I've seen this strange behavior multiple times in another CI builds...
Could you check the test locally while running and debugging in your Visual Studio please?
Later I will run this test for current commits in the develop branch...
But just FYI in the top commit push code coverage for main and develop branches we have green CI-CD pipelines! The build: 690

@raman-m
Copy link
Member

raman-m commented May 16, 2023

Hi Fabrizio! What is the issue this PR is related to? Number?

Hi @raman-m this is the related issue #1400

Fabrizio,
Could we move description of the feature to the issue #1400 please?
PR description is not a good place to write requirements of a feature!

@Fabman08
Copy link
Contributor Author

Can anyone check the error?

OK. Probably this test "should_reload_config_on_change" is unstable. I've seen this strange behavior multiple times in another CI builds... Could you check the test locally while running and debugging in your Visual Studio please? Later I will run this test for current commits in the develop branch... But just FYI in the top commit push code coverage for main and develop branches we have green CI-CD pipelines! The build: 690

Local unit tests are successfully passed

image

also "should_reload_config_on_change"

image

@raman-m raman-m marked this pull request as ready for review May 19, 2023 13:16
@raman-m
Copy link
Member

raman-m commented May 19, 2023

@Fabman08
Congrats! The build 755 is green.
So, the PR is accepted for review!

@raman-m raman-m added accepted Bug or feature would be accepted as a PR or is being worked on and removed waiting Waiting for answer to question or feedback from issue raiser labels May 19, 2023
@raman-m raman-m self-requested a review May 19, 2023 13:27
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, fix the issues!

src/Ocelot/Configuration/SecurityOptions.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/SecurityOptions.cs Outdated Show resolved Hide resolved
@raman-m raman-m changed the title Manage multiple pattern for allowed/blocked IP #1400 Manage multiple patterns for allowed/blocked IPs via Security Options config section May 22, 2023
@raman-m raman-m self-requested a review May 22, 2023 18:30
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor issues to fix, plz!

docs/features/routing.rst Outdated Show resolved Hide resolved
src/Ocelot/Configuration/SecurityOptions.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/SecurityOptions.cs Outdated Show resolved Hide resolved
@raman-m raman-m requested review from nikita-petko and thiagoloureiro and removed request for nikita-petko May 22, 2023 18:48
@Fabman08
Copy link
Contributor Author

Same previous build error

@raman-m raman-m added accepted Bug or feature would be accepted as a PR or is being worked on and removed needs feedback Issue is waiting on feedback before acceptance labels Sep 30, 2023
@raman-m raman-m merged commit 5dbbbef into ThreeMammals:develop Sep 30, 2023
@raman-m raman-m deleted the feat/allowedBlockIpPattern branch September 30, 2023 14:26
@raman-m
Copy link
Member

raman-m commented Sep 30, 2023

@Fabman08 Thank you for your contribution! 👍
Hope, you 'll return to us with new PRs for current issues in backlog 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Bug or feature would be accepted as a PR or is being worked on feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Manage multiple patterns for allowed/blocked IPs via Security Options config section
5 participants