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 mp.jwt.verify.publickey check in SmallryeJwtDevModeProcessor #46932

Merged
merged 1 commit into from
Mar 23, 2025

Conversation

rsvoboda
Copy link
Member

@rsvoboda rsvoboda commented Mar 21, 2025

Fix mp.jwt.verify.publickey check in SmallryeJwtDevModeProcessor

Fixes #46928 / native mode case

This comment has been minimized.

@rsvoboda
Copy link
Member Author

rsvoboda commented Mar 21, 2025

oki, I will fix this. it's (SmallryeJwtDevModeProcessor) obviously also for tests

@rsvoboda rsvoboda force-pushed the SmallryeJwtDevModeProcessor branch from 79a0c1a to b359947 Compare March 21, 2025 12:19
@rsvoboda rsvoboda changed the title Attach SmallryeJwtDevModeProcessor to development mode only Don't run SmallryeJwtDevModeProcessor in native Mar 21, 2025

This comment has been minimized.

@@ -55,7 +56,7 @@ public class SmallryeJwtDevModeProcessor {
*
* @throws NoSuchAlgorithmException if RSA-256 key generation fails.
*/
@BuildStep(onlyIfNot = { IsNormal.class })
@BuildStep(onlyIfNot = { IsNormal.class, NativeOrNativeSourcesBuild.class })
Copy link
Member

Choose a reason for hiding this comment

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

I would expect the native build to run in normal mode? So I'm a bit surprised it makes a difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

my bet is NativeSourcesBuild part, but I can look more into it

Copy link
Member Author

Choose a reason for hiding this comment

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

Having just @BuildStep(onlyIfNot = { IsNormal.class, NativeSourcesBuild.class }) doesn't fix the issue

So my change is the minimal one.

gsmet
gsmet previously requested changes Mar 21, 2025
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I'm not exactly excited about this change and I would like us to get to the bottom of it.

What I understand is that you are pointing to a non-existing public key and hoping things will work.

Also I'm not exactly sure why you have JWT around but jwt is not really used.

Could you clarify exactly what you're doing in this test, why you need to define a JWT key that points to nowhere - and why you expect this to be fine.

Maybe your patch is right but at this point I can't be sure.

@rsvoboda
Copy link
Member Author

BeefyTS is legacy and we keep it alive to see if old approaches work. The 003-quarkus-many-extensions module combines a lot of extensions and the intention is to ensure it can be built in JVM and Native mode. JWT properties in https://github.com/quarkus-qe/beefy-scenarios/blob/main/003-quarkus-many-extensions/src/main/resources/application.properties#L4-L5 were needed when the app was composed, now the JWT extension doesn't need these properties to be defined when no JWT logic is triggered. I can remove them and things will work again.

I identified the PR that caused failure of the module in native. It's #44272 as mentioned in #46928 (comment)

Very strange thing is that JVM mode doesn't fail, but native mode fails trying to read the /foo/bar from mp.jwt.verify.publickey.location property.

#44272 states that Generate RSA-256 keys on dev mode and that got me slightly sidetracked, it's apparently for dev mode and tests. So the title of PR should be renamed and also maybe alsoSmallryeJwtDevModeProcessor.java should be renamed to indicate also test, e.g. SmallryeJwtDevTestModeProcessor.java.

So in the end I think I will just adjust 003-quarkus-many-extensions and drop the mp.jwt dummy config.

@rsvoboda
Copy link
Member Author

oh, I think I hit something :) I will update the PR

@rsvoboda rsvoboda force-pushed the SmallryeJwtDevModeProcessor branch from b359947 to 1675e64 Compare March 21, 2025 18:12
@rsvoboda rsvoboda changed the title Don't run SmallryeJwtDevModeProcessor in native Fix mp.jwt.verify.publickey check in SmallryeJwtDevModeProcessor Mar 21, 2025
@rsvoboda
Copy link
Member Author

@gsmet PR updated to check userConfigs for mp.jwt.verify.publickey, it also fixes my old/weird case

@rsvoboda
Copy link
Member Author

I have updated #44272 title to mention also test

Let me know about the SmallryeJwtDevModeProcessor file naming. Should I also rename it to SmallryeJwtDevTestModeProcessor or I'm just being too choosey :) ?

@rsvoboda rsvoboda requested a review from gsmet March 21, 2025 18:16
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

Thanks @rsvoboda 🙏

sberyozkin

This comment was marked as duplicate.

Copy link

quarkus-bot bot commented Mar 21, 2025

Status for workflow Quarkus CI

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

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

sberyozkin commented Mar 21, 2025

As far as I understand, this fix from @rsvoboda fixes the typo in the @mcruzdev's PR, @mcruzdev can you please check?

@sberyozkin
Copy link
Member

I'll hold off the merge for now, I'm on the road and missed there was the discussion, I think the fix is good, but would appreciate @mcruzdev having a look.

@mcruzdev
Copy link
Contributor

I am taking a look now

@mcruzdev
Copy link
Contributor

mcruzdev commented Mar 21, 2025

If the user does not set the property then should be setted as NONE. Thanks a lot @rsvoboda, and sorry guys!

@sberyozkin, @gsmet I think we can merge this one.

@sberyozkin
Copy link
Member

Thanks @mcruzdev

@sberyozkin sberyozkin dismissed gsmet’s stale review March 23, 2025 20:01

Latest PR update fixes the typo, with the rationale behind this update also clarified. #44272 has tests but I think the ordering of checks have prevented detecting the typo earlier

@sberyozkin sberyozkin merged commit d8fade0 into quarkusio:main Mar 23, 2025
25 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.22 - main milestone Mar 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants