search

Brightspace / D2L.Security.OAuth2

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

Adding 'iat' (issued at) claim to tokens #342

Closed joshhoey closed 8 months ago

joshhoey commented 10 months ago

Adding iat (Issued At) as a claim (same as nbf for now, but it means something different)

omsmith commented 10 months ago

Just need to get the CI setup on this repo updated. Will get that done before lunch.

j3parker commented 10 months ago

Hm, I thought we had iat but we have nbf and exp. I wonder if we could just get by with those? (probably just nbf?)

joshhoey commented 10 months ago

Hm, I thought we had iat but we have nbf and exp. I wonder if we could just get by with those? (probably just nbf?)

We could get by with nbf (it would have made my life a bit easier) but nbf doesn't strictly need to match iat, we're just making it "now" by convention, so at this point we figured we'd keep going and add it. 🤷‍♀️

My core work on Lms is already testing well against nbf.

omsmith commented 10 months ago

(probably just nbf?)

Given the semantic difference, I preferred adding the proper claim.

@joshhoey could you rebase to get the CI changes.

j3parker commented 10 months ago

Given the semantic difference, I preferred adding the proper claim.

We can also just use one if it exists and if not the other. iat isn't a required claim.

omsmith commented 10 months ago

I don't think it being required or not is a particularly compelling reason not to use it. nbf and exp are also optional claims, but we of course use them due to the semantics of what they imply (and require of client libraries).

We do not currently have any usage of nbf being set to a future date, and I don't think it's terribly likely that we would end up wanting to either. However I'm not sure how to assert that we never do so such that the general-we would catch the change down the line.

I was definitely iffy about adding extra bytes to the tokens, but the semantic of iat is exactly what we're looking for, so suggested it as being more appropriate despite that.

Can you think of a way to future-proof ourselves against future mistakes re nbf being adjusted away from time-of-issue, and thus failing to revoke those tokens?

j3parker commented 10 months ago

Can you think of a way to future-proof ourselves against future mistakes re nbf being adjusted away from time-of-issue, and thus failing to revoke those tokens?

var threshold = token.IssuedAt ?? token.NotBefore;

is what I meant by "we can also just use one if it exists and if not the other". Like we don't have iat right now because it would mostly be redundant, so I think it's fine to just do that.

j3parker commented 10 months ago

I would also not be opposed to min(nbf, iat) to be conservative. I don't think there are any rules about which of those come earlier, and using the earlier of the two would always revoke a greater number of tokens which I think is what we'd want (assuming we're putting reasonable values into iat/nbf and not like "0" -- JWT claims like this have barely any semantics/its left to implementations to decide this stuff.)