-
Notifications
You must be signed in to change notification settings - Fork 32
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
Update section on client authentication #216
Update section on client authentication #216
Conversation
No, it doesn't. An API consumer may get used to tokens with a longer lifetime being accepted, and then one day they are rejected because they are dealing with a different authorisation server.
The CAMARA specification is a separate specification to OIDC, albeit one defined by differences to that standard. Any CAMARA client or authorisation server must meet the CAMARA specification, and not just vanilla OIDC. What you are saying is that any API consumer that does not adopt the CAMARA standard but only the OIDC standard will have interoperability issues. Well, yes they will, which is why we don't say "just use OIDC". There are already changes relative to OIDC in the CAMARA standard that mandate features that are optional in OIDC. This would be another such change - a feature that is optional for OIDC is mandatory for CAMARA.
API definitions are linked to a specific release of the CAMARA working group standards. This would be a change to the next release of the ICM standards, and so existing API definitions are not affected. The client would only need to upgrade to support the next release of the API.
The authorisation server should reject the request as not being CAMARA compliant. However, no "authorisation server police" are going to come around and shutdown the server. But an API consumer used to having tokens accepted without the That is the situation we are trying to avoid by having an "interoperability profile". |
I think we can have the best of both worlds (ensure interoperability + minimize deviation from standards + avoid breaking existing API consumers when possible) if we:
JWTs should be used shortly after they are generated; 300 seconds is a generous interval, so I don't think defaulting to the current time is an issue. @eric-murray What do you think? Some telcos don't have a dedicated Authorization Server exclusively for issuing tokens for CAMARA APIs. This is one of the reasons why making standard JWTs not valid is problematic. |
So I'm fine to keep I'd prefer to keep the statement "JWTs with a longer lifetime SHALL be rejected by the authorisation server" because this addresses @MarkCornall's concern about specifying consistent behaviour across implementations. Now I accept that there will be authorisation server implementations that will anyway accept tokens with a longer lifetime. And that's fine - as I said, no "authorisation server police" are going to come around and shut it down. Specifications such as this one are written primarily for clients so that they understand how to construct their requests, and what responses they can expect. API providers know the game and know what they can get away with at the edges of a standard without breaking it. So the statement "JWTs with a longer lifetime SHALL be rejected by the authorisation server" would be telling the client what will happen if their token's lifetime is too long. Whether that actually happens in all cases is not so important, though we do want to avoid the situation where most authorisation servers accept longer lived tokens and clients get used to that behaviour. |
Agree, thanks Eric. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM considering #216 (comment)
What is the benefit of having Why not just use exp and define that exp-now<300 seconds? Also why mention all the different endpoints? Why not just state that the lifetime of the jwt used in client_credentials flow must be shorter than 300 seconds. The current text seems unnecessarily complicated. |
CIBA Core made the mistake of making JWTs without an I see that OIDC CIBA avoided this mistake for signed authentication requests, so well done there.
From ICM Meeting Minutes:
So the request was to specify error responses for the different endpoints as the relevant standards do not include "token lifetime too long" as an explicit error example. But I agree it may now be better to document this in #220.
If the standard is using the term "lifetime", we need to define what is meant by "lifetime", and I am defining it as the difference between the "client_credentials flow" is not the correct term here |
Removed wording on error responses, as that is now covered by #220 |
How does iat help against creating stockpiled jwts or stockpiled jwts being misused after they were stolen? The API consumer is controlling JWT creation. So, they could create 1000 JWT for future use. some pseudo-code that creates JWTs with lifetime 300 that can run at the API consumer's site at anytime:
Now the API consumer has many JWTs with iat which still can be stolen and misused. I feel I am missing something. Sorry, if I am thick. Which threat scenario is iat protecting against? |
Well, it clearly doesn't eliminate the risk of tokens being stolen and misused, but may reduce the number that are available to be stolen. Let's say an API consumer does not want to create signed JWTs "on the fly", but rather generate them overnight using a batch process, and then use them throughout the next day by pulling one off the stack when required. Even with the new 300 second maximum token lifetime rule, they could still create multiple batches overnight, with But if they included the And maybe the need to generate a new batch every 300 seconds just might persuade the API consumer to generate them on demand, as they should. It's a small point, but a point nonetheless. |
@camaraproject/identity-and-consent-management_maintainers The two options are:
The pros and cons for each solution are:
It's a relatively small point compared to all the other issues being discussed in ICM, so I propose a simple "vote" to resolve it. I favour Option 2. Axel favours Option 1. Others should express their view in a comment. |
Agree with Option 2 as per #216 (comment) |
As there are no further responses, can we close this PR considering option 2? Do you agree with this? If so, can we merge the PR as it stands? I would suggest setting this week as the deadline. |
@AxelNennker Please review this PR to unblock and potentially merge it. The latest feedback from WG is available at https://lf-camaraproject.atlassian.net/wiki/spaces/CAM/pages/56754234/2024-12-18+ICM+Minutes |
@eric-murray @jpengar I am waiting on comments on my suggestions. Please commit or write a comment why not. |
There is only one open suggestion, which already has a comment against it, but I will add another. The current text proposal is the one that the last meeting agreed to proceed with. |
I marked the two uncommented suggestions with "I see no reaction to this suggestion". Why do you think that I am in favor of option 1)?
I want to avoid using different words than the spec used.
I think "required" is better then "mandate", and "age" is better than "lifetime". Which is not ignoring iat, right? @eric-murray you write you see only one suggestion by me. Are we looking at the same page? I see two suggestions here on this page and two on this page https://github.com/camaraproject/IdentityAndConsentManagement/pull/216/files |
I don't see that suggestion. What I see is the (now outdated) text suggestion:
... and this is Option 1 - determine lifetime relative to the arrival time, ignoring
Happy to use "required" instead of "mandated", though they are not exact synonyms. I think "lifetime" is ( So I would prefer to use "lifetime" to emphasise that a token for which
I only see one outdated suggestion not resolved, and do not see the text you quote above. Are the new changes shown as "pending" in your view? Github lets you prepare multiple changes in private before publishing them for others to see. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I never mend that iat is ignored and don't know where this notion comes from.
I think the sentence "measured as the difference between the exp (expires at) claim and the token creation time (the value of the iat claim, whether present in the token or not)" makes no sense, because you cannot compute the difference of two values if one is not present. That is why I proposed this text.
Actually that text has the same flaw. How about this:
|
It does make sense, because the API consumer can always ask themselves "if I did include the |
Following the above comments, I tightened up the language to clarify:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@eric-murray the main point in the WG meeting seemed to be we want to prevent the client assertion producer to do stupid things. Ff client assertions are created in a batch then iat would be a lie if a API consumer created assertions with a lifetime of 300 seconds. start = now Afterward they would have a ton of assertions which are all a lie because iat is a lie. What we write in CAMARA specs cannot prevent servers to create assertions with any values and sign them. The API consumer could also create a ton of assertions in a batch without iat and just increase exp by 300. The OAuth2 security best practices do not mention I think it is a good idea to restrict the lifetime of the assertion to 300 seconds. |
@eric-murray addresses clients and servers in one text. I think we should only address the server and the client requirements logically follow from that.
The server cannot determine the token creation time if iat is missing How about this: The request MUST be rejected if the |
I did update the text compared to that quoted in your last comment, but the point that the authorisation server is implementing two rules, dependent on whether
I think it is still useful to include some "guidance" for clients not to create long lived tokens and think that is OK just because they don't include the |
Good |
OK, updated |
Dismissing my review change request that would make iat mandatory
I removed my review change request and if won't create a new one. Still, how about this:
write:
thus making it clear that this sentence is guidance for the API consumer who creates client assertions. |
@AxelNennker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Formally we can merge now. To make it easier to see that all comments are addressed, I closed all "conversions" which were "outdated" anyway. Also there a no change-requests from reviews. |
What type of PR is this?
What this PR does / why we need it:
The current specification for client authentication does not limit the lifetimes of the signed JWT. This presents a security risk, and further complicates implementations as there is an additional requirement that the authorisation server should reject duplicate JWTs.
This PR proposes that:
iat
claimWhich issue(s) this PR fixes:
Fixes #208
Special notes for reviewers:
None
Changelog input
Additional documentation
See proposed changes for documentation links