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

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

Closed
wants to merge 3 commits into from
Closed

Conversation

joshhoey
Copy link

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

src/D2L.Security.OAuth2/Keys/UnsignedToken.cs Outdated Show resolved Hide resolved
src/D2L.Security.OAuth2/Keys/Default/TokenSigner.cs Outdated Show resolved Hide resolved
@omsmith
Copy link
Contributor

omsmith commented Nov 21, 2023

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

@j3parker
Copy link
Member

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
Copy link
Author

joshhoey commented Nov 21, 2023

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
Copy link
Contributor

omsmith commented Nov 21, 2023

(probably just nbf?)

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

@joshhoey could you rebase to get the CI changes.

@j3parker
Copy link
Member

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.

Copy link
Member

@j3parker j3parker left a comment

Choose a reason for hiding this comment

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

We don't need more claims

@omsmith
Copy link
Contributor

omsmith commented Nov 21, 2023

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
Copy link
Member

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
Copy link
Member

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.)

@omsmith omsmith closed this Jan 16, 2024
@omsmith omsmith deleted the jhoey/add-iat branch January 16, 2024 22:35
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