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

Validate incoming JWT tokens from the bot framework #11129

Merged

Conversation

losterloh
Copy link
Contributor

@losterloh losterloh commented May 18, 2022

Proposed changes:
This PR enables JSON Web Token validation for the JWTs coming in from the Azure botframework channel. This is achieved by periodically updating the keys that Microsoft uses to sign the tokens, saving those in memory, and then validating the tokens and verifying their signature for incoming requests from the bot framework.
For details on how the botframework does authentication, see here.

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

self._update_cached_jwk_keys()

def _update_cached_jwk_keys(self) -> None:
if datetime.datetime.now() - self.jwt_update_time < datetime.timedelta(days=1):
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 now, I'd say a daily update of the JWT keys is plenty, and also currently no need to make this configurable.

@losterloh losterloh force-pushed the ATO-99-validate-incoming-jwt-token-for-bot-framework branch from cafe4e9 to f0cee1c Compare May 25, 2022 09:15
@losterloh losterloh marked this pull request as ready for review May 25, 2022 09:18
@losterloh losterloh requested a review from a team as a code owner May 25, 2022 09:18
@losterloh losterloh requested review from ancalita and removed request for a team May 25, 2022 09:18
@losterloh losterloh force-pushed the ATO-99-validate-incoming-jwt-token-for-bot-framework branch 2 times, most recently from 268f3b2 to 83c9eb1 Compare May 25, 2022 14:30
@losterloh losterloh changed the base branch from main to 3.1.x May 25, 2022 14:32
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

Looks good 😎
Made some style suggestions, and if possible, some unit tests covering different scenarios for peace of mind 🙏🏼

rasa/core/channels/botframework.py Outdated Show resolved Hide resolved
rasa/core/channels/botframework.py Outdated Show resolved Hide resolved
@losterloh losterloh force-pushed the ATO-99-validate-incoming-jwt-token-for-bot-framework branch from 83c9eb1 to c01d966 Compare June 2, 2022 13:22
@losterloh losterloh requested a review from a team June 2, 2022 13:22
@losterloh losterloh requested a review from a team as a code owner June 2, 2022 13:22
@losterloh losterloh requested review from kedz, RASADSA and ancalita and removed request for a team and kedz June 2, 2022 13:22
@losterloh losterloh force-pushed the ATO-99-validate-incoming-jwt-token-for-bot-framework branch from c01d966 to f92eddb Compare June 2, 2022 13:25
@losterloh
Copy link
Contributor Author

@ancalita I think I found a good way to test the auth stuff, thanks for pointing that out. Also, made me learn a lot about JWT in the process 😅

Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

Great work 💯
Left some existential questions about the type of status to be returned in some cases 😄 .
Also please look at the additional comments in the unit tests to strengthen the existing assertions 💪🏼

tests/core/channels/test_botframework.py Outdated Show resolved Hide resolved
tests/core/channels/test_botframework.py Show resolved Hide resolved
tests/core/channels/test_botframework.py Show resolved Hide resolved
tests/core/channels/test_botframework.py Show resolved Hide resolved
tests/core/channels/test_botframework.py Show resolved Hide resolved
tests/core/channels/test_botframework.py Outdated Show resolved Hide resolved
rasa/core/channels/botframework.py Outdated Show resolved Hide resolved
rasa/core/channels/botframework.py Show resolved Hide resolved
@losterloh losterloh force-pushed the ATO-99-validate-incoming-jwt-token-for-bot-framework branch from f92eddb to b349ec5 Compare June 8, 2022 14:05
@losterloh losterloh requested a review from ancalita June 8, 2022 14:05
@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2022

🚀 A preview of the docs have been deployed at the following URL: https://11129--rasahq-docs-rasa-v2.netlify.app/docs/rasa

Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

🎉 Thank you for addressing those extra assertions 💯

@losterloh losterloh enabled auto-merge June 8, 2022 14:47
@losterloh losterloh merged commit fbbdcf1 into 3.1.x Jun 8, 2022
@losterloh losterloh deleted the ATO-99-validate-incoming-jwt-token-for-bot-framework branch June 8, 2022 15:06
@Maxinho96
Copy link
Contributor

Hello, thank you for implementing this.
Before this change I addressed this issue myself using a custom channel component. I leveraged the jwt.PyJWKClient (ref), which should already handle caching (ref). The important thing is to keep an instance of the PyJWKClient in the channel object for its whole lifecycle, like this:

@cached_property # This ensures the jwks_client is instantiated only at the first call
def jwks_client(self) -> PyJWKClient:
    try:
        openid_metadata = requests.get(
            "https://login.botframework.com/v1/.well-known/openidconfiguration"
        ).json()
        jwks_uri = openid_metadata["jwks_uri"]
        jwks_client = PyJWKClient(jwks_uri)
    except Exception as e:
        logger.exception(e)
        raise sanic.exceptions.Unauthorized(
            "Cannot retrieve the Botframework JWKS URI."
        )

    return jwks_client

def _check_jwt(self, request: Request) -> None:
    try:
        key = self.jwks_client.get_signing_key_from_jwt(request.token).key
    except Exception as e:
        logger.exception(e)
        raise sanic.exceptions.Unauthorized(
            "Cannot retrieve the Botframework public key."
        )

    try:
        jwt.decode(
            request.token, key, audience=self.app_id, algorithms=["RS256"]
        )
    except jwt.exceptions.ExpiredSignatureError as e:
        logger.exception(e)
        raise sanic.exceptions.SanicException(
            status_code=440,
            message="The JWT has expired.",
            quiet=True,
        )
    except jwt.InvalidSignatureError as e:
        logger.exception(e)
        raise sanic.exceptions.Unauthorized("Invalid JWT signature.")
    except Exception as e:
        logger.exception(e)
        raise sanic.exceptions.Unauthorized("Could not verify JWT.")

You could consider switching to this class instead of handling cache by your own. Hope you find this useful.

@losterloh
Copy link
Contributor Author

@Maxinho96 thank you for pointing thejwt.PyJWKClient out, did not know about that one before and looks great! We'll consider refactoring this again to use that.

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.

None yet

3 participants