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

Update docs to make it easy to see that the code flow access token fails, update tests #40523

Merged

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented May 8, 2024

Closes #40494.

I've spent a lot of time today testing #40494 and so is @effusion :-), verifying several times all works as expected with latest Quarkus with the custom Azure customizer, but it was not clear at all why it worked in 3.8.3.

In the end of the day it occurred to me it must be due to a hardening (correct) fix where users inject an authorization code flow access token but do not enable its verification, in addition to the mandatory ID token verification. So the signature failure reported in #40521 was related to the extra code flow access token verification but is confusingly logged as ID token verification failure.

So this PR updates docs to make it much clearer when the code flow access token is also verified now and how it can be disabled back to the old (though not too secure state, if really necessary). Tests are also updated (with an extra UserInfo test too).

Note it is not a breaking change, but I'll add a note to the migration guide too once this PR is reviewed/merged

@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation area/oidc labels May 8, 2024
Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

I think this will help users. Thank you

This comment has been minimized.

Copy link

github-actions bot commented May 8, 2024

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

@gastaldi
Copy link
Contributor

gastaldi commented May 8, 2024

The associated issue number seems wrong?

@michalvavrik
Copy link
Member

The associated issue number seems wrong?

Good catch! I thought this issue is about #40494.

@sberyozkin
Copy link
Member Author

Thanks @michalvavrik , @gastaldi , will address suggestions tomorrow morning, as I'm away from laptop, cheers

@sberyozkin sberyozkin marked this pull request as draft May 9, 2024 08:22
@sberyozkin
Copy link
Member Author

Converted to draft as the exception related update is not great, need to improve it

@sberyozkin sberyozkin force-pushed the improve_code_flow_at_failure_message branch from ce74a0b to e558e84 Compare May 9, 2024 11:18
@sberyozkin sberyozkin marked this pull request as ready for review May 9, 2024 11:19
@sberyozkin
Copy link
Member Author

sberyozkin commented May 9, 2024

Hi @michalvavrik @gastaldi I'm happy enough now with how it is logged, the previous version was unfortunately incomplete, it was covering the initial authorization code completion failure logging but not the one during the re-authentication (when the authenticated user returns), and OidcIdentityProvider updates were looking strange to me this morning (but not yesterday :-) ), so I reverted them and have a single piece of code dealing with checking if it is the the code flow access token or ID token verification that has failed.

I've actually spent a couple of hours trying to figure out how to make it simpler :-), as I thought there was no obvious way at the code authentication mechanism level to detect which token failed the verification, but then I found the current boolean expression which is precise.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@sberyozkin sberyozkin force-pushed the improve_code_flow_at_failure_message branch from aec5b6e to f030a33 Compare May 9, 2024 16:46
@sberyozkin
Copy link
Member Author

@gastaldi Yeah, it looks nicer with your suggestion, applied to both docs

Copy link

quarkus-bot bot commented May 9, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit f030a33.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented May 9, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit f030a33.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@sberyozkin sberyozkin merged commit 6ce0c8a into quarkusio:main May 9, 2024
25 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.11 - main milestone May 9, 2024
@sberyozkin sberyozkin deleted the improve_code_flow_at_failure_message branch May 9, 2024 18:14
@gsmet gsmet modified the milestones: 3.11 - main, 3.10.1 May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

Issues in verifying signature of bearer token generated for Azure AD
4 participants