search

Brightspace / D2L.Security.OAuth2

Brightspace OAuth 2.0 for C#
Apache License 2.0
7 stars 16 forks source link

Allow TokenSigner to sign tokens with complex claims #113

Closed khorwood closed 5 years ago

khorwood commented 5 years ago

Adds support for signing JWT messages with complex claims such as objects or arrays for use by integrations such as LTI v1.3 launch workflow.

khorwood commented 5 years ago

Further updates expected in additional PRs:

omsmith commented 5 years ago

I believe the only user of ITokenSigner is AccessTokenProvider. So UnsignedToken could just be updated, instead of providing two variations.

omsmith commented 5 years ago

Support arbitrary string kid from partner supplied jwks URIs

Because OIDC?

omsmith commented 5 years ago

@j3parker If we're doing to support inter-operating with other folks JWKS, then dc85894ba1c8e60076c85c7c4358037888b2e4c8 becomes invalid.

omsmith commented 5 years ago

(And fetching /jwk/:id in the first place becomes confusing)

omsmith commented 5 years ago

@khorwood We have some d2l-isms in this library. I wonder if

Support arbitrary string kid from partner supplied jwks URIs

is better left to using a "standard" OIDC implementation to validate incoming tokens, and then hand it off into our stack?

khorwood commented 5 years ago

I believe the only user of ITokenSigner is AccessTokenProvider. So UnsignedToken could just be updated, instead of providing two variations.

Could do, would need to add an extra Provision method to support the funky LTI claims needed which aren't compatible with the Claims class. Otherwise we could leverage the ITokenSigner directly from the LMS launch flow.

edit: On second reading I don't actually want this, since we're minting an LTI message with a JWS. I don't need an access token.

khorwood commented 5 years ago

@khorwood We have some d2l-isms in this library. I wonder if

Support arbitrary string kid from partner supplied jwks URIs

Is better left to using a "standard" OIDC implementation to validate incoming tokens, and then hand it off into our stack?

I'd like to, but we'll also be supporting a Service-to-Service flow where the Tool (registered as an app in AuthService) will be sending signed assertions which we'll need to verify before issuing an Auth Service token to them so they can make API calls against the LMS.

omsmith commented 5 years ago

I'd like to, but we'll also be supporting a Service-to-Service flow where the Tool (registered as an app in AuthService) will be sending signed assertions which we'll need to verify before issuing an Auth Service token to them.

Right, which is where I was suggesting the standard OIDC implementation would validate that incoming assertion, and after validating, then giving one of our tokens. i.e. send them to a different endpoint?

khorwood commented 5 years ago

I'd like to, but we'll also be supporting a Service-to-Service flow where the Tool (registered as an app in AuthService) will be sending signed assertions which we'll need to verify before issuing an Auth Service token to them.

Right, which is where I was suggesting the standard OIDC implementation would validate that incoming assertion, and after validating, then giving one of our tokens. i.e. send them to a different endpoint?

The LTI launch is inspired by OIDC, and doesn't quite follow the spec, so we'll be implementing specific routes in the LMS to handle the launch communication. This specific PR is just about giving us the ability to sign the message using the LMS's PK.

j3parker commented 5 years ago

Sorry for not getting to this sooner 😞

What did you think about @omsmith 's idea about modifying UnsignedToken vs a second type?

These changes are required for LTI message right? So like, only in the LMS? I wonder it might be easier/faster to avoid using D2L.Security.OAuth2 to sign/handle the tokens, and just provide our credentials to the underlying libraries. Just a thought - I don't want to distract you too much.

khorwood commented 5 years ago

Sorry for not getting to this sooner 😞

What did you think about @omsmith 's idea about modifying UnsignedToken vs a second type?

These changes are required for LTI message right? So like, only in the LMS? I wonder it might be easier/faster to avoid using D2L.Security.OAuth2 to sign/handle the tokens, and just provide our credentials to the underlying libraries. Just a thought - I don't want to distract you too much.

Yeah, we only need the funky claims for the LTI message to the tool.

I'm open to condensing the class, I'm just not sure of the optimal approach. Which would be better in your eyes?

I'm not sure if exposing the private credentials would be any better as I'd still need to recreate most of the TokenSigner to achieve the goal of signing a JWT anyway.

omsmith commented 5 years ago

Shouldn't need to worry about any backwards compatibility if you also update AccessTokenProvider

khorwood commented 5 years ago

Revised as per your suggestions. I noticed a lot of whitespace changes. Is there an .editorconfig I should be using for this repo?

omsmith commented 5 years ago

Not presently. I can add one though

omsmith commented 5 years ago

@khorwood I added an .editorconfig. I don't know that it will perfectly represent the current state, but I did my best.