#2165 Global configuration `SecurityOptions` #2170

Fabman08 · 2024-10-11T06:47:50Z

Closes #2165

Issue with IP Blocking and Allowing in global configuration #2165

Proposed Changes

As the issue #2165 reports, the SecurityOptions are missing in GlobalConfiguration.

This pull request adds the ability to configure Allowed and Blocked IPs globally: the global configuration will be applied for every route unless an existing SecurityOptions is present on it

raman-m

Absence of Acceptance Tests❗

What I realized, currently, there are no Security acceptance tests in place, which is undoubtedly a significant concern.

Please develop acceptance tests:

Create "Security" folder in Ocelot.AcceptanceTests project
Add IpSecurityTests class inheriting from Steps
Develop basic config tests

src/Ocelot/Configuration/Creator/SecurityOptionsCreator.cs

raman-m · 2024-10-17T12:28:41Z

Dear Fabrizio,
Why are you pushing commits to the origin when the tests fail? Please verify that the tests pass locally before pushing new commits to the origin. My mailbox is overflowing with failed build reports from your feature branch!

Fabman08 · 2024-10-17T14:32:39Z

Dear Fabrizio, Why are you pushing commits to the origin when the tests fail? Please verify that the tests pass locally before pushing new commits to the origin. My mailbox is overflowing with failed build reports from your feature branch!

@raman-m sorry for the mail bombing, but switching to develop and running all tests the result is the following

Am I missing something to run unit test correctly?

raman-m · 2024-10-18T08:30:37Z

Oh, you love screenshots, my dear friend...

Current builds in develop: Passed
Top commit checks status: ce0227c has passed
All CircleCI builds here: develop builds

raman-m · 2024-10-18T08:39:42Z

but switching to develop and running all tests the result is the following

Regarding the execution of tests in Visual Studio locally, we have certain precise tests that are highly sensitive to CPU resource availability. These tests generally pass when run silently through the terminal. They are designed to pass 99% of the time, but occasionally they may fail. In Visual Studio, additional UI tasks may be running, and if your PC's CPU is at 90-100% capacity, these precise tests might fail. Usually, running the tests again resolves the issue. Ensure that your PC's CPU is not busy, and Visual Studio is not engaged in background compilation or other intensive tasks. If the tests fail, attempt to run them once more.

raman-m · 2024-10-18T09:35:23Z

My proof that we should not panic. Below are the results from my local testing in Visual Studio, which indicate that everything is functioning correctly.

Everything is fine!

ggnaegi · 2024-10-25T11:10:33Z

@raman-m @Fabman08 could you please rebase instead of merge?

ggnaegi

Ok for me, but I would add some changes...

ggnaegi · 2024-10-25T11:26:05Z

src/Ocelot/Configuration/SecurityOptions.cs

        }

        public SecurityOptions(string allowed = null, string blocked = null)
            : this()
        {
            if (!string.IsNullOrEmpty(allowed))
            {
-                IPAllowedList.Add(allowed);
+                IPAllowedList = IPAllowedList.Append(allowed).ToList();


This is in my opinion a bit over-engineered, I would keep it simple, avoid calling this() and simply instantiate the two lists and then add the elements.

Skipped due to raman-m answer

Gui, do you know that this feature and its internal classes can produce 1 million of strings in memory?

@raman-m @Fabman08 I'm probably too dumb, but could you clarify? My point is, as soon as you call SecurityOptions(...): this(), the lists will be recreated... So what's the relation with the 1 million of strings in memory? My understanding of it was, that you could end up with very big lists for reach route.

@ggnaegi The issue lies in the design of the IPSecurityPolicy, which relies on processing string objects, often referred to as a flawed design (aka Chinese design, the author is Chinese developer who developed Security feat in 2017/18). It is my firm belief that IP policy processing should be based on IpAddress objects instead of solely relying on strings.

Another concern of mine is that using the Contains method to compare strings, this is a workable solution but it requires having a list of all IP strings. However, in general, it is incorrect to compare IP strings due to the presence of the . dot character mixed with digits. The appropriate comparator for IP strings should position the segments as follows:

10.20.1.200 should be converted to _10._20.__1.200

10.1.10.100 should be converted to _10.__1._10.100

111.222.123.123 should remain unchanged.

The optimal solution is to convert IP to IpAddress or even to a long value. I believe the internal state of IpAddress is based on long values.

So what's the relation with the 1 million of strings in memory?

This is a problem of the design issue mentioned above, descendant problem. The Creator class logic parses the IpAddress object and converts it into strings.
Let's consider the following test cases:

the range 192.168.1.1-192.168.255.255: it produces 256², a maximum of 65 536 strings in the list

the range 10.1.1.1-10.255.255.255: it generates 256³, a maximum of 16 777 216 string objects in the list, resulting in 16 million items❗ This highlights a significant problem.

However, this design issue is out of the current scope. Let the author to complete this config-feat.

@Fabman08 Do you understand this code review issue?
Would you prefer to keep calling base constructor this() but remove producing new list .ToList()?
Or would you remove this() but wanna keep .ToList()? 😃

P.S. To be fair, I don't understand why did you propose this change? The old code seems pretty optimized and it's valid which was based on this()...
Wanted to fix Visual Studio warning with .Add to upgrade to .Append, right? 🥲

@raman-m Yes, I understand the issue reported, and I choose to keep calling this() and remove .ToList().
Fixed in last commit

I will add my concerns as a new TODO-task for the next refactoring round.

ggnaegi · 2024-10-25T12:05:29Z

src/Ocelot/Security/IPSecurity/IPSecurityPolicy.cs

@@ -18,7 +18,7 @@ public async Task<Response> Security(DownstreamRoute downstreamRoute, HttpContex

            if (securityOptions.IPBlockedList != null)
            {
-                if (securityOptions.IPBlockedList.Exists(f => f == clientIp.ToString()))
+                if (securityOptions.IPBlockedList.Contains(clientIp.ToString()))


ClientIp could be null, it's not handled

Fixed checking clientIp

Why not to develop operators aka equality operator methods for IpAddress and string objects, if their classes have no already implemented ones? 😉
After operators implementation it will be possible to simply write: IPBlockedList.Contains(clientIp) because of IEquatable<T> interface.

See examples of operator implementations, current ones. The awesome possibility of operators as long as other methods is having 2 arguments of different types. But classic operators override the operation for 2 operands of the same type.

Different types of 2 operands:

Ocelot/src/Ocelot/LoadBalancer/Lease.cs

Lines 58 to 62 in 41fc9bd

public static bool operator ==(ServiceHostAndPort h, Lease l) => h == l.HostAndPort;

public static bool operator !=(ServiceHostAndPort h, Lease l) => !(h == l);

public static bool operator ==(Lease l, ServiceHostAndPort h) => l.HostAndPort == h;

public static bool operator !=(Lease l, ServiceHostAndPort h) => !(l == h);

The same types of 2 operands:

Ocelot/src/Ocelot/LoadBalancer/Lease.cs

Lines 55 to 56 in 41fc9bd

public static bool operator ==(Lease x, Lease y) => x.HostAndPort == y.HostAndPort; // ignore -> x.Connections == y.Connections;

public static bool operator !=(Lease x, Lease y) => !(x == y);

and

Ocelot/src/Ocelot/Requester/MessageInvokerPool.cs

Lines 119 to 120 in 41fc9bd

public static bool operator ==(MessageInvokerCacheKey left, MessageInvokerCacheKey right) => left.Equals(right);

public static bool operator !=(MessageInvokerCacheKey left, MessageInvokerCacheKey right) => !(left == right);

Cool, right?

The funny fact is that YARP project developers sometimes implement operators: only 4 types in whole project ! 🤣
But they are Microsoft senior engineers with 150K+ salaries, right @ggnaegi ? 😄
And those operators are pretty elementary, simple equality.

Maybe in a next Feature PR ? 🤣

Yes, next time... 😋

src/Ocelot/Security/IPSecurity/IPSecurityPolicy.cs

src/Ocelot/Configuration/Creator/SecurityOptionsCreator.cs

raman-m · 2024-10-25T17:59:44Z

Ok for me, but I would add some changes...

Gui, do you know that this feature and its internal classes can produce 1 million of strings in memory? 😂

This reverts commit 3e4d4a4.

…ecurityOptions constructr parameters

raman-m · 2024-11-19T13:00:01Z

Could we reduce the number of rebase operations that involve force pushes? A rebase is necessary when conflicts arise, but if are no conflicts, it would be better to simply merge from develop. The reason I bring this up is messages from the team during code reviews seem to be lost after rebasing.
Ultimately, I would prefer not to lose our code review results.

raman-m · 2024-11-19T13:03:03Z

@ggnaegi commented on Oct 25

@raman-m @Fabman08 could you please rebase instead of merge?

I disagree! Read my msg above! There were no conflicts. Let's rebase only in case of merge conflicts...

raman-m

My final suggestions below 👇

src/Ocelot/Configuration/Creator/SecurityOptionsCreator.cs

src/Ocelot/Configuration/File/FileSecurityOptionsExtensions.cs

test/Ocelot.AcceptanceTests/Security/SecurityOptionsTests.cs

test/Ocelot.UnitTests/Configuration/SecurityOptionsCreatorTests.cs

test/Ocelot.UnitTests/Security/IPSecurityPolicyTests.cs

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

raman-m · 2024-11-20T17:08:47Z

@Fabman08 Please stop any development! Don't push anything!.. I'm making final review...

raman-m

Ready for Delivery ✅

raman-m · 2024-11-20T19:22:14Z

@Fabman08 Thank you, Fabrizio, for this contribution! 👍
We completed the feature in 40 days. Not bad! 😄

Fabman08 · 2024-11-20T19:40:01Z

@Fabman08 Thank you, Fabrizio, for this contribution! 👍
We completed the feature in 40 days. Not bad! 😄

@raman-m thank you too!! It's a pleasure for me collaborate for this project! I'm sorry for the misunderstanding, I hope to be more accurate the next collaboration

Fabman08 marked this pull request as ready for review October 11, 2024 06:53

raman-m assigned Fabman08 and raman-m Oct 11, 2024

raman-m added Security Options Ocelot feature: Security Options Configuration Ocelot feature: Configuration labels Oct 11, 2024

raman-m added this to the October'24 milestone Oct 11, 2024

raman-m added the Oct'24 October 2024 release label Oct 11, 2024

raman-m requested review from RaynaldM, raman-m and ggnaegi October 11, 2024 07:55

raman-m changed the title ~~Adds SecurityOptions to Global Configuration~~ Adds SecurityOptions to global configuration Oct 11, 2024

raman-m requested changes Oct 11, 2024

View reviewed changes

RaynaldM approved these changes Oct 14, 2024

View reviewed changes

src/Ocelot/Configuration/Creator/SecurityOptionsCreator.cs Outdated Show resolved Hide resolved

raman-m requested a review from RaynaldM October 14, 2024 08:52

Fabman08 requested a review from raman-m October 15, 2024 10:32

Fabman08 force-pushed the fix/ipBlockAllowGlobalCnf branch from 8b0c83b to 60fa237 Compare October 15, 2024 11:02

raman-m marked this pull request as draft October 17, 2024 12:29

RaynaldM approved these changes Oct 24, 2024

View reviewed changes

ggnaegi reviewed Oct 25, 2024

View reviewed changes

raman-m added Winter'25 Winter 2025 release and removed Oct'24 October 2024 release labels Oct 26, 2024

raman-m modified the milestones: October'24, Autumn'24 Oct 26, 2024

FabrizioMancin added 8 commits November 19, 2024 08:47

Adds acceptance tests

1b0af5f

set SecurityOptions constructor with IList

7e0f066

Revert "set SecurityOptions constructor with IList"

9e54669

This reverts commit 3e4d4a4.

SecurityOptions se constructor parameters as IList<T>

4667bc2

SecurityOptionsCreator edit IP parsing in order to pass string[] as S…

cf362ed

…ecurityOptions constructr parameters

IPSecurityPolicy fix Exists to Contains

eb6036e

avoid null check nesting, add clientIp null check

eb2fa5e

simplify SecurityOptions Create method

4a37e8b

Fabman08 force-pushed the fix/ipBlockAllowGlobalCnf branch from b8d2031 to 4a37e8b Compare November 19, 2024 07:47

SecurityOptions removed .ToList()

bd592a3

raman-m requested changes Nov 20, 2024

View reviewed changes

Fabman08 and others added 4 commits November 20, 2024 15:48

SecurityOptionsCreator set native parameter in private methods

b4dbf32

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

SecurityOptionsCreator edited as suggested

8f51eff

fix traits and BDDfy

2c35ad9

FileSecurityOptions move IsFullFilled inside the class

05914fa

Code review by @raman-m

a06a467

raman-m approved these changes Nov 20, 2024

View reviewed changes

raman-m changed the title ~~Adds SecurityOptions to global configuration~~ Global configuration SecurityOptions Nov 20, 2024

raman-m changed the title ~~Global configuration SecurityOptions~~ #2165 Global configuration SecurityOptions Nov 20, 2024

raman-m merged commit 7a5046f into ThreeMammals:develop Nov 20, 2024
1 check passed

raman-m added Nov'24 November 2024 release and removed Winter'25 Winter 2025 release labels Nov 22, 2024

raman-m modified the milestones: Autumn'24, November'24 Nov 22, 2024

raman-m mentioned this pull request Nov 22, 2024

Release 23.4.1 | Routing patch | November'24 release | +semver: patch #2215

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2165 Global configuration `SecurityOptions` #2170

#2165 Global configuration `SecurityOptions` #2170

Fabman08 commented Oct 11, 2024 •

edited by raman-m

Loading

raman-m left a comment

raman-m commented Oct 17, 2024

Fabman08 commented Oct 17, 2024

raman-m commented Oct 18, 2024

raman-m commented Oct 18, 2024

raman-m commented Oct 18, 2024

ggnaegi commented Oct 25, 2024

ggnaegi left a comment

ggnaegi Oct 25, 2024

Fabman08 Nov 13, 2024

ggnaegi Nov 19, 2024 •

edited

Loading

raman-m Nov 19, 2024 •

edited

Loading

raman-m Nov 19, 2024 •

edited

Loading

Fabman08 Nov 19, 2024

raman-m Nov 20, 2024 •

edited

Loading

ggnaegi Oct 25, 2024

Fabman08 Nov 6, 2024 •

edited

Loading

raman-m Nov 19, 2024 •

edited

Loading

raman-m Nov 19, 2024 •

edited

Loading

Fabman08 Nov 19, 2024

raman-m Nov 20, 2024

raman-m commented Oct 25, 2024 •

edited

Loading

raman-m commented Nov 19, 2024

raman-m commented Nov 19, 2024

raman-m left a comment

raman-m commented Nov 20, 2024

raman-m left a comment

raman-m commented Nov 20, 2024

Fabman08 commented Nov 20, 2024

	public static bool operator ==(ServiceHostAndPort h, Lease l) => h == l.HostAndPort;
	public static bool operator !=(ServiceHostAndPort h, Lease l) => !(h == l);

	public static bool operator ==(Lease l, ServiceHostAndPort h) => l.HostAndPort == h;
	public static bool operator !=(Lease l, ServiceHostAndPort h) => !(l == h);

	public static bool operator ==(Lease x, Lease y) => x.HostAndPort == y.HostAndPort; // ignore -> x.Connections == y.Connections;
	public static bool operator !=(Lease x, Lease y) => !(x == y);

	public static bool operator ==(MessageInvokerCacheKey left, MessageInvokerCacheKey right) => left.Equals(right);
	public static bool operator !=(MessageInvokerCacheKey left, MessageInvokerCacheKey right) => !(left == right);

#2165 Global configuration SecurityOptions #2170

#2165 Global configuration SecurityOptions #2170

Conversation

Fabman08 commented Oct 11, 2024 • edited by raman-m Loading

Closes #2165

Proposed Changes

raman-m left a comment

Choose a reason for hiding this comment

Absence of Acceptance Tests❗

raman-m commented Oct 17, 2024

Fabman08 commented Oct 17, 2024

raman-m commented Oct 18, 2024

raman-m commented Oct 18, 2024

raman-m commented Oct 18, 2024

ggnaegi commented Oct 25, 2024

ggnaegi left a comment

Choose a reason for hiding this comment

ggnaegi Oct 25, 2024

Choose a reason for hiding this comment

Fabman08 Nov 13, 2024

Choose a reason for hiding this comment

ggnaegi Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

raman-m Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

raman-m Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Fabman08 Nov 19, 2024

Choose a reason for hiding this comment

raman-m Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

ggnaegi Oct 25, 2024

Choose a reason for hiding this comment

Fabman08 Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

raman-m Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

raman-m Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Fabman08 Nov 19, 2024

Choose a reason for hiding this comment

raman-m Nov 20, 2024

Choose a reason for hiding this comment

raman-m commented Oct 25, 2024 • edited Loading

raman-m commented Nov 19, 2024

raman-m commented Nov 19, 2024

raman-m left a comment

Choose a reason for hiding this comment

raman-m commented Nov 20, 2024

raman-m left a comment

Choose a reason for hiding this comment

Ready for Delivery ✅

raman-m commented Nov 20, 2024

Fabman08 commented Nov 20, 2024

#2165 Global configuration `SecurityOptions` #2170

#2165 Global configuration `SecurityOptions` #2170

Fabman08 commented Oct 11, 2024 •

edited by raman-m

Loading

ggnaegi Nov 19, 2024 •

edited

Loading

raman-m Nov 19, 2024 •

edited

Loading

raman-m Nov 19, 2024 •

edited

Loading

raman-m Nov 20, 2024 •

edited

Loading

Fabman08 Nov 6, 2024 •

edited

Loading

raman-m Nov 19, 2024 •

edited

Loading

raman-m Nov 19, 2024 •

edited

Loading

raman-m commented Oct 25, 2024 •

edited

Loading