Skip to content

Conversation

@Nikos410
Copy link
Contributor

@Nikos410 Nikos410 commented Jan 13, 2023

Implements #16450

This authenticator accepts a list of ip addresses or subnets and matches if the client ip is in one of the specified subnets. It also allows for excluding the specified addresses, meaning that it matches if the client IP is NOT part of any of the specified subnets.

I added a dependency on com.github.seancfoley:ipaddress for parsing the ip address / subnet notation, because I did not find a utility for doing so that is already part of the project. I evaluated the apache commons-net library, but that one only supports IPv4, while the dependency by seancfoley supports both versions and also supports CIDR notation as well as netmask notation. Examples: a:b:c:d::/64, a.b.c.d/255.255.0.0

I did not include any test cases for this, because I'm not too familiar with the internals of this project and did not find a way to mock the client ip address in an automated test case. Maybe you have some pointers for me?

Example Screenshots of the new conditional authenticator:

Screenshot 2023-01-13 at 14-25-05 Keycloak Administration UI

Screenshot 2023-03-27 at 15-47-55 Keycloak Administration UI

@sschu
Copy link
Contributor

sschu commented Jan 23, 2023

You could probably write an integration test and send the request with an X-Forwarded-For header containing the client ip? I guess a lot of Keycloaks are deployed behind a reverse proxy anyways.

@kreativmonkey
Copy link

kreativmonkey commented Jan 23, 2023

Filtering IP ranges is also possible with the conditional http request header: #14605 it's more flexible but also more complicated.

@raphisuter
Copy link
Contributor

raphisuter commented Jan 27, 2023

We use something similar of the same kind which only supports IPV4. It would be awesome to have this ConditinalAuthenticator built in to Keycloak directly instead of a separate provider. Would't it be better to use MULTIVALUED_STRING_TYPE instead of comma separated IPs? For the user it's self explaining and easier to read the IPs in the configuration view (see screenshot). At the current build there is an error which prevents the use of MULTIVALUED_STRING_TYPE properties #17851 but as soon as it is fixed it would be a better choice in my opinion.
We also implemented a negate switch to easily test the condition, maybe this would be a good addition as well?
image

@Cyben
Copy link

Cyben commented Jan 27, 2023

Actually I have also implemented something like that with some other devs.
From my experience the X-Forwarded-For could have multiple ip's (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For).
So you should check if it works on your implemention, although I can see you are using a build in function for recieving the ip.

@Nikos410
Copy link
Contributor Author

Hello everyone,

thanks for you feedback and your suggestions :)
I'm travelling next week, I will follow up on this PR after that.

Have a nice weekend!
Nikos

Copy link
Contributor

@stianst stianst left a comment

Choose a reason for hiding this comment

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

It's not really clear to me what the use-case for this one is. Could you please elaborate how this is intended to be used?

@Nikos410
Copy link
Contributor Author

It's not really clear to me what the use-case for this one is. Could you please elaborate how this is intended to be used?

@stianst Sure! Our use-case is this:
We have some users that are working from within our / our customer's corporate networks. For these users, a simple login using username / password is sufficient. But some users are connecting from the internet, for example because they are travelling. For these users we want to require additional 2FA authentication. This is also an explicit requirement of our insurance.

Is there an easier way to achieve this?

Best
Nikos

@matrium0
Copy link

matrium0 commented Mar 2, 2023

We would love to have this feature as well. Many companies have security policies like this:
a) if you are within the company network you can login without 2FA
b) if you are in another network you have to use 2FA

@Nikos410 Nikos410 requested a review from a team as a code owner March 27, 2023 10:12
@Nikos410 Nikos410 force-pushed the main branch 2 times, most recently from 180f10c to 035dacf Compare March 28, 2023 12:37
@Nikos410 Nikos410 requested review from a team as code owners March 28, 2023 12:41
@Nikos410 Nikos410 requested a review from a team March 28, 2023 12:41
@Nikos410
Copy link
Contributor Author

Nikos410 commented Mar 28, 2023

Hey everyone, sorry for not getting back to you earlier.

I've modified my implementation to use the multivalue string type and to prefer the IP in the X-Forwarded-For header if it is present. i've updated the screenshot in the PR description.

I've also added some tests, inspired by #14605 . Selenium does not seem to support setting request headers, so the test relies on the actual IP address during the test being the local loopback address.

Could you take a look again? @raphisuter @sschu @Cyben @stianst

Best
Nikos

@Nikos410 Nikos410 force-pushed the main branch 6 times, most recently from 1f4d81a to f2d9206 Compare March 31, 2023 10:52
@Nikos410
Copy link
Contributor Author

@stianst Is there anything I should change about this implementation? Or is this a feature that you don't want to include in Keycloak? In that case I would try to package it as a custom jar extension.

@pedroigor
Copy link
Contributor

My 2cs ...

The capability herein proposed makes sense and I see it being helpful for others.

However, the code we accept needs to be maintained, which most of the time relies on us. Sometimes competing with other priorities.

This situation raises some issues for the project:

  • The frustration from contributors (and believe me, from ourselves too) because their PR takes too much time to review (like this one).
  • It blocks (or significantly reduces) us from evolving the project and delivering new capabilities to the community even though they only reach out to 20% of use cases.
  • It blocks us from baking new capabilities and potentially supporting them OOTB depending on their popularity/usage/ applicability.
  • Impact on extensibility
  • Developer experience is also somehow impacted as we don't provide good tools to find/develop/test/publish extensions.

Only a few came to my mind ...

The issues above are basically addressed by Keycloak Extensions, an initiative we did not yet finish discussing.

I hope that after Keycloak 22 we can, together with the community, discuss an extensibility model that changes how people find/develop/test/publish extensions.

@BaraCoding
Copy link

Hi,
is there a way to get this as an jar extension? I looking for an ip based flow for my mfa solution and your work seems to be the way. Or is there now (Keycloak 22 is released) an easier way to get it in my Keycloak-Environment?

Best
Torsten

@Nikos410
Copy link
Contributor Author

Hey @BaraCoding I've moved this extension to a standalone plugin: https://github.com/evosec/keycloak-ipaddress-authenticator

Just grab the keycloak-ipaddress-authenticator-21.1.1_1-jar-with-dependencies.jar from the latest release and drop it in your Keycloak plugins directory :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants