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

fix: issuance PAR req jti #54

Merged
merged 6 commits into from
Sep 1, 2023
Merged

fix: issuance PAR req jti #54

merged 6 commits into from
Sep 1, 2023

Conversation

peppelinux
Copy link
Member

@peppelinux peppelinux commented Jul 6, 2023

the PAR request is a JWS which its reuse (replay attack) must be prevented

This PR adds jti in the PAR request JWS

@asharif1990
Copy link
Collaborator

asharif1990 commented Jul 6, 2023

I forgot to add the JWS claims in the PAR request. However, the Nonce will provide the exact reply detection mechanism to avoid the PAR replay. In any case, we can have the "jti" as well as it is a good practice for the JWS. Probably, we can add the following missing headers of the JWS: "iss", "aud", "exp", and "iat".

@fmarino-ipzs
Copy link
Collaborator

nonce and jti parameters have different purposes. Typically the jti is chosen by the authentication server, while nonce is given by the client. Given this, we probably should add the nonce parameter instead of jti, am I missing something?

@asharif1990
Copy link
Collaborator

nonce and jti parameters have different purposes. Typically the jti is chosen by the authentication server, while nonce is given by the client. Given this, we probably should add the nonce parameter instead of jti, am I missing something?

So, as the PAR request is created and signed by the Wallet Instance, in this case, the Wallet Instance itself can add the jti parameter in the payload as well. The scenario that you are referring to is for the case of releasing ID Token and Access Token that are released by the server, so, in that case, the jti value will be set by the server.

@peppelinux
Copy link
Member Author

@asharif1990 are you suggesting to use nonce instead of jti?

@fmarino-ipzs
Copy link
Collaborator

Sorry I was wrong about that, I meant to say use jti instead of nonce. The purpose of the nonce is to create a binding to a token as pointed out by @asharif1990 in the case of Access Token or ID Token. I'm not sure we really need the nonce here. The Access Token in our case is bound to the c_nonce parameter. The jti is for a replay attack. The only concern about this is that in this case it is difficult to have a collision resistant jti since it is given by the client. What do you think?

@asharif1990
Copy link
Collaborator

asharif1990 commented Jul 7, 2023

@asharif1990 are you suggesting to use nonce instead of jti?

I think nonce may provide the same countermeasure because it is transaction specific. However, the more standard way of doing it for the JWS is jti. my recommendation is to keep both.

@asharif1990
Copy link
Collaborator

Sorry I was wrong about that, I meant to say use jti instead of nonce. The purpose of the nonce is to create a binding to a token as pointed out by @asharif1990 in the case of Access Token or ID Token. I'm not sure we really need the nonce here. The Access Token in our case is bound to the c_nonce parameter. The jti is for a replay attack. The only concern about this is that in this case it is difficult to have a collision resistant jti since it is given by the client. What do you think?

I am totally okay with jti as I mentioned in the above comment. I think it shouldn't be a problem. If you are not using a problematic algorithm to create this random string, shouldn't be any problem.

@peppelinux
Copy link
Member Author

following a chat with @fmarino-ipzs we agreed that we should investigate better what would be the impacts of a reply of that request

in this pr we have proposed jti as a mitigation following the BCP, but we didn't have any analysis about the implication or the impact of a reply attack in that endpoint.

there's the perception that we don't have any relevant risk due to a reply of that request, further analysis are required

@peppelinux
Copy link
Member Author

Here my considerations

  • jti Is not collision resistant and must be identified uniquely together with its issuer

  • the nonce binds a request to a response

  • an adversary should stay in the middle to intercept the request and hijack the victim to a different request_uri, in that case It simply replay the nonce since It Is well known during the auditing

This makes to me that jti aside with its issuer Is collision resistant and prevents that the request might be replayed

While the nonce is required when a request or any Response May be replayed, in particular when the transation is async and over different endpoints

@ruphy ruphy self-requested a review August 4, 2023 15:30
Copy link
Collaborator

@grausof grausof left a comment

Choose a reason for hiding this comment

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

LGTM

@peppelinux peppelinux merged commit 5696e16 into versione-corrente Sep 1, 2023
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants