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

Afford a twilio SMS Authenticator for keycloak #3

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

bickelj
Copy link
Contributor

@bickelj bickelj commented Feb 23, 2023

This project creates a lib-all.jar that can be placed in keycloak's directory named /providers. When present, the jar affords an SMS authenticator. The Authenticator implementation is based on dasniko's at https://github.com/dasniko/keycloak-2fa-sms-authenticator and also requires his keycloak-requiredaction jar to be in /providers. After the jars are in place, keycloak needs to be configured to use them. See https://www.n-k.de/2020/12/keycloak-2fa-sms-authentication.html and
#2 (comment) for more details.

The twilio settings are loaded via environment variables:

  • TWILIO_PHONE_NUMBER: the "from" phone number set up in twilio.
  • TWILIO_ACCOUNT_SID: the SID or username for twilio API access.
  • TWILIO_AUTH_TOKEN: the token or secret for twilio API access.

Copy link
Contributor Author

@bickelj bickelj left a comment

Choose a reason for hiding this comment

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

This PR is a little hasty in the sense that it is missing a README and some of the comments I just made need to be incorporated as comments in the code. There are also recommendations in the comments by dasniko that should be followed. The reason to push code is it is good to push frequently and to show progress, affording the opportunity for specific feedback.

@@ -0,0 +1,6 @@
distributionBase=GRADLE_USER_HOME
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This properties file, the above binary, and below gradlew scripts are boilerplate/bootstrapping for the gradle build system. Yes, it is customary to commit the gradle-wrapper.jar binary to the repository but no other jars. The reason it is here is to make it easy to run gradle commands, e.g. ../gradlew test just works.

twilio-keycloak-provider/lib/build.gradle.kts Show resolved Hide resolved
twilio-keycloak-provider/lib/build.gradle.kts Outdated Show resolved Hide resolved
twilio-keycloak-provider/lib/build.gradle.kts Show resolved Hide resolved
twilio-keycloak-provider/lib/build.gradle.kts Show resolved Hide resolved
private static final PhoneNumber FROM = new PhoneNumber(System.getenv("TWILIO_PHONE_NUMBER"));

static {
Twilio.init(System.getenv("TWILIO_ACCOUNT_SID"), System.getenv("TWILIO_AUTH_TOKEN"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For better testability these might be converted to Java System Properties rather than env vars. But env vars are simple enough and the code is so little...

Copy link
Contributor

Choose a reason for hiding this comment

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

Could they be ProviderConfigPropertys, like length and ttl?

If we stay with environment variables, can we make them a little more resiliant? System.getEnv(String) will return a null if the variable is not defined. What happens if Keycloak is booted with this provider but without the Twilio configuration? I see from the unit test that Twilio will throw an AuthenticationException, but will it be clear to the Keycloak operator what to do about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Side note on testability, it is weird that the Twilio library requires static init method call at all. Right away I found twilio/twilio-java#650.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the environment variable loading a little more resilient. Do you think it would be much better as ProviderConfigProperty? The environment variables are disconnected from keycloak but are pretty straightforward to configure. The ProviderConfigProperty values have working defaults but I doubt we can make working defaults with Twilio. The benefit of going to ProviderConfigProperty seems to be more seamless integration. I am up for either but inertia kept the env vars.

*/

rootProject.name = "twilio-keycloak-provider"
include("lib")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lib name is kind of awkward.

Copy link
Contributor

@jasonaowen jasonaowen left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @bickelj! This looks very promising.

I don't have a full environment set up yet, and I don't have access to any Twilio credentials, so I haven't been able to test this yet - at this point I'm only looking at the code.

I think it would make sense to squash the four commits down into one - I think this is just one logical change, right?

Sorry for so many comments; they're not all equally important! I hope this is helpful feedback.

twilio-keycloak-provider/lib/build.gradle.kts Outdated Show resolved Hide resolved
private static final PhoneNumber FROM = new PhoneNumber(System.getenv("TWILIO_PHONE_NUMBER"));

static {
Twilio.init(System.getenv("TWILIO_ACCOUNT_SID"), System.getenv("TWILIO_AUTH_TOKEN"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could they be ProviderConfigPropertys, like length and ttl?

If we stay with environment variables, can we make them a little more resiliant? System.getEnv(String) will return a null if the variable is not defined. What happens if Keycloak is booted with this provider but without the Twilio configuration? I see from the unit test that Twilio will throw an AuthenticationException, but will it be clear to the Keycloak operator what to do about that?

@jasonaowen
Copy link
Contributor

In #1 (comment), @bickelj wrote:

Looking back at my objections to this API key extension approach, namely the "internal SPI" and "extending keycloak" objections in PhilanthropyDataCommons/service#96 (comment) the same "internal SPI" warning occurs with PR #3 and it too is an extension to keycloak.

I think a critical difference is that this is extending Keycloak in a way that is not visible to applications that authenticate with Keycloak! This work is more in line with the operational task of "how do we make Keycloak work in our organization with our security requirements", and not part of the development task "how do we make an app that works with Keycloak".

tl;dr: this seems like good and worthwhile work! I have no concerns with this approach.

@bickelj
Copy link
Contributor Author

bickelj commented Feb 27, 2023

I plan to squash the commits once the changes are up to snuff. Does that sound reasonable?

@bickelj bickelj requested a review from jasonaowen February 27, 2023 23:22
@jasonaowen
Copy link
Contributor

I plan to squash the commits once the changes are up to snuff. Does that sound reasonable?

Of course! I thought that might be the case, but explicit is better than implicit. :)

Copy link
Contributor

@jasonaowen jasonaowen left a comment

Choose a reason for hiding this comment

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

This looks good! Thanks for the excellent follow-through, @bickelj.

After talking with @jim-mcgowan today, I believe we decided that my time was not best spent on testing this code - we can verify it works in our testing environment.

This project creates a `lib-all.jar` that can be placed in keycloak's
directory named `/providers`. When present, the jar affords an SMS
authenticator. The Authenticator implementation is based on dasniko's
at https://github.com/dasniko/keycloak-2fa-sms-authenticator and also
requires his keycloak-requiredaction jar to be in `/providers`. After
the jars are in place, keycloak needs to be configured to use them.
See https://www.n-k.de/2020/12/keycloak-2fa-sms-authentication.html
and
#2 (comment)
for more details.

The twilio settings are loaded via environment variables:
 - `TWILIO_PHONE_NUMBER`: the "from" phone number set up in twilio.
 - `TWILIO_ACCOUNT_SID`: the SID or username for twilio API access.
 - `TWILIO_AUTH_TOKEN`: the token or secret for twilio API access.

The LICENSE continues the license of the original work by Niko Köbler.
See
https://github.com/dasniko/keycloak-2fa-sms-authenticator/blob/4205a6/LICENSE

The jar is a shaded fat jar, but avoids including classes found on the
keycloak classpath.

Issue #2 Two-factor SMS authentication with keycloak and twilio

Co-authored-by: Jason Owen <jason@jasonaowen.net>
@bickelj
Copy link
Contributor Author

bickelj commented Feb 28, 2023

I am still torn on all the squashing but that is a discussion for another day. Squashed and it looks like it's working. Merging.

@bickelj bickelj merged commit ae64f8c into main Feb 28, 2023
@bickelj bickelj mentioned this pull request Mar 2, 2023
@bickelj
Copy link
Contributor Author

bickelj commented Mar 2, 2023

@jasonaowen and @kfogel I have a question regarding process. Above I squashed, force pushed, and then presumed to merge before re-review. This was following a pattern established in the service project, but would it be better to have a reviewer actually click merge? Or to at least confirm the exact squashed commit has been re-reviewed prior to merge?

@jasonaowen
Copy link
Contributor

@bickelj and I talked about this a bit offline, but for the record: the way I like to work - and I believe the way @slifty likes to work - is for the author to merge PRs after getting an approving review. I like the way this spreads responsibility and autonomy, and it also gives reviewers the opportunity to leave "yes, and..." reviews with optional suggestions - as I did above - and leave it up to the author to take those suggestions or not as they see fit.

One side note is that we should clean up branches after they're merged; there's a setting to do so automatically for this that I'm following up about with the folks who can make that change. For now, I'll delete this one by hand.

@jasonaowen jasonaowen deleted the 2-initial-keycloak-sms-authenticator branch March 2, 2023 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants