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

Issue with IP Blocking and Allowing in global configuration #2165

Closed
CavidH opened this issue Oct 7, 2024 · 7 comments · Fixed by #2170
Closed

Issue with IP Blocking and Allowing in global configuration #2165

CavidH opened this issue Oct 7, 2024 · 7 comments · Fixed by #2170
Assignees
Labels
Configuration Ocelot feature: Configuration merged Issue has been merged to dev and is waiting for the next release Nov'24 November 2024 release proposal Proposal for a new functionality in Ocelot Security Options Ocelot feature: Security Options
Milestone

Comments

@CavidH
Copy link

CavidH commented Oct 7, 2024

I configured IP blocking and allowing in Ocelot using SecurityOptions, but it's not working.

{
  "GlobalConfiguration": {
    "BaseUrl": "http://localhost:5000",
    "SecurityOptions": {
      "IPBlockedList": ["192.168.0.23"]
    }
  }
}


The IP blocking configuration is not working as expected.

@raman-m raman-m added Security Options Ocelot feature: Security Options question Initially seen a question could become a new feature or bug or closed ;) Configuration Ocelot feature: Configuration labels Oct 8, 2024
@raman-m
Copy link
Member

raman-m commented Oct 8, 2024

Hello, Cavid!
It seems we lack support for global settings.
The potential solutions could be:

  1. Solely using ocelot.json. Define the options for each route individually.
  2. Utilizing C# coding. Replace the ISecurityOptionsCreator service in the DI container by redeveloping the SecurityOptionsCreator class to consider only the global settings.
    Services.TryAddSingleton<ISecurityOptionsCreator, SecurityOptionsCreator>();

Which solution would be more convenient for you?

@raman-m
Copy link
Member

raman-m commented Oct 8, 2024

Hello @Fabman08,
The absence of global settings support is a significant issue. Here's the current usage of SecurityOptionsCreator:

var securityOptions = _securityOptionsCreator.Create(fileRoute.SecurityOptions);

Consequently, the method should accept two arguments, including global settings:

public SecurityOptions Create(FileSecurityOptions securityOptions)

Would you be able to allocate some time to address this?

@raman-m raman-m changed the title Issue with IP Blocking and Allowing in Ocelot Configuration Issue with IP Blocking and Allowing in global configuration Oct 8, 2024
@Fabman08
Copy link
Contributor

Fabman08 commented Oct 8, 2024

Hi @raman-m!
Sure, I'll be able to fix the issue this or next week. ☺️

@CavidH thank you for reporting this issue! 👍

@raman-m raman-m added proposal Proposal for a new functionality in Ocelot and removed question Initially seen a question could become a new feature or bug or closed ;) labels Oct 8, 2024
@CavidH
Copy link
Author

CavidH commented Oct 8, 2024

Thank you very much for your help. We will eagerly await the new version.

@CavidH
Copy link
Author

CavidH commented Oct 8, 2024

Hello, Cavid! It seems we lack support for global settings. The potential solutions could be:

  1. Solely using ocelot.json. Define the options for each route individually.
  2. Utilizing C# coding. Replace the ISecurityOptionsCreator service in the DI container by redeveloping the SecurityOptionsCreator class to consider only the global settings.
    Services.TryAddSingleton<ISecurityOptionsCreator, SecurityOptionsCreator>();

Which solution would be more convenient for you?

I had thought of the first option, but since I have too many services, it will be difficult to control this

@Fabman08
Copy link
Contributor

@raman-m just a little question about Global Settings behaviours.

I've found three ways to fix the issue:

  1. Every properties in Route FileSecurityOptions overrides their Global Settings values
    eg.
  • Global:
    Allowed: B,C
    Blocked: E
    ExcludeAllowedFromBlocked: true
  • Route:
    Allowed: A,B,C
    Blocked: D,E,F
    ExcludeAllowedFromBlocked: false
  • Final:
    Allowed: A,B,C
    Blocked: D,E,F
    ExcludeAllowedFromBlocked: false
  1. Only the existing properties in Route FileSecurityOptions overrides their Global Settings values
    eg.
  • Global:
    Allowed: B,C
    Blocked: E
    ExcludeAllowedFromBlocked: true
  • Route:
    Allowed: A,B,C
    Blocked: null
    ExcludeAllowedFromBlocked: null
  • Final:
    Allowed: A,B,C
    Blocked: E
    ExcludeAllowedFromBlocked: true
  1. The Route FileSecurityOptions values will be merged to Global Settings values
    eg.
  • Global:
    Allowed: A,B,C
    Blocked: D,E
    ExcludeAllowedFromBlocked: true
  • Route:
    Allowed: F,G,H
    Blocked: I,K
    ExcludeAllowedFromBlocked: false
  • Final:
    Allowed: A,B,C,F,G,H
    Blocked: D,E,I,K
    ExcludeAllowedFromBlocked: false

Which one is the best?

@raman-m
Copy link
Member

raman-m commented Oct 10, 2024

@Fabman08 Your research is too complicated!

Which one is the best?

It is best to prioritize the route-specific FileSecurityOptions object over the global settings. Therefore, if route settings are defined, all global settings should be disregarded. In practice, if the route object is null, then the global settings should be utilized.

Every properties in Route FileSecurityOptions overrides their Global Settings values

Yes, if route options are defined, all global ones should be ignored. We will highlight this in the documentation. There should be no merging whatsoever. This approach is the simplest and most correct solution, as merging properties would be erroneous due to potential conflicts within the algorithm. Therefore, merging is identified as a source of bugs.

@raman-m raman-m added the Oct'24 October 2024 release label Oct 11, 2024
@raman-m raman-m added this to the October'24 milestone Oct 11, 2024
@raman-m raman-m added Dec'24 December 2024 release and removed Oct'24 October 2024 release labels Oct 26, 2024
@raman-m raman-m modified the milestones: October'24, Autumn'24 Oct 26, 2024
raman-m added a commit that referenced this issue Nov 20, 2024
* Adds SecurityOptions to Global Configuration

* Adds acceptance tests

* set SecurityOptions constructor with IList

* Revert "set SecurityOptions constructor with IList"
This reverts commit 3e4d4a4.

* SecurityOptions se constructor parameters as IList<T>

* SecurityOptionsCreator edit IP parsing in order to pass string[] as SecurityOptions constructr parameters

* IPSecurityPolicy fix Exists to Contains

* avoid null check nesting, add clientIp null check

* simplify SecurityOptions Create method

* SecurityOptions removed .ToList()

* SecurityOptionsCreator set native parameter in private methods

Co-authored-by: Raman Maksimchuk <[email protected]>

* SecurityOptionsCreator edited as suggested

* fix traits and BDDfy

* FileSecurityOptions move IsFullFilled inside the class

* Code review by @raman-m

---------

Co-authored-by: Fabrizio Mancin <[email protected]>
Co-authored-by: Raman Maksimchuk <[email protected]>
@raman-m raman-m added merged Issue has been merged to dev and is waiting for the next release Nov'24 November 2024 release and removed Dec'24 December 2024 release labels Nov 20, 2024
@raman-m raman-m modified the milestones: Autumn'24, November'24 Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Configuration Ocelot feature: Configuration merged Issue has been merged to dev and is waiting for the next release Nov'24 November 2024 release proposal Proposal for a new functionality in Ocelot Security Options Ocelot feature: Security Options
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants