Generate RSA-256 keys on dev mode #44272

mcruzdev · 2024-11-02T20:39:25Z

Description

This PR aims to add on DEV mode, to generate a RSA-256 pair key.

It is great for development and test environments, the user just need to set two 3 config properties:

smallrye.jwt.sign.key.location=privateKey.pem
mp.jwt.verify.publickey.location=publicKey.pem
quarkus.smallrye-jwt.generate-sign-keys=true

Status: In progress

Add tests
Add documentation
Improve javadoc
Self review

quarkus-bot · 2024-11-02T20:39:28Z

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

title should preferably start with an uppercase character (if it makes sense!)

_{This message is automatically generated by a bot.}

sberyozkin · 2024-11-03T12:37:40Z

Hey @mcruzdev Thanks for giving it a try, indeed, we'd like to make it easy for quarkus-smalrye-jwt developers to start in devmode...

But what should really be done is that none of those properties should be required in devmode, no any temporary files should be created...

If mp.jwt.verify.publickey is not configured (or any other verification key properties - I can cover this part later), and no smallrye.jwt.sign.key is not configured (or any other signing key properties - I can also cover this part later) then the quarkus-smallrye-jwt devservice build step should generate an RSA key pair, and set those 2 properties to the encoded key values.

The code which will generate tokens using the published private key smallrye.jwt.sign.key has to be written by the user as only the user knows what claims should be added to the token, using a no-arg sign() and we'll also do something interactive in DevUI later as well. but the very first step is to have this key pair accessible via those 2 properties.

@michalvavrik, do you recall which build item can be used to report build time properties ? (smallrye-jwt ones related to keys are currently build time only...)

michalvavrik · 2024-11-03T13:01:45Z

@michalvavrik, do you recall which build item can be used to report build time properties ? (smallrye-jwt ones related to keys are currently build time only...)

If this https://github.com/smallrye/smallrye-jwt/blob/74638c415a0096e1916363e51571e6ed4aecf8d2/implementation/jwt-auth/src/main/java/io/smallrye/jwt/config/JWTAuthContextInfoProvider.java#L219 is the only place where this property is used, then I'd expect returning them from DevServicesResultBuildItem should work. However if you are certain they are build-time properties then I think you need to define your own SmallRye Config source, or maybe use customizer as is done here (but make sure nothing happens when it is not a DEV mode) https://github.com/quarkusio/quarkus/blob/main/core/deployment/src/main/java/io/quarkus/deployment/configuration/BuildTimeConfigBuilderCustomizer.java.

mcruzdev · 2024-11-03T14:33:29Z

Hi @sberyozkin, was necessary to get the properties (*.location) from the user, because I do not know how to change configuration values on build time, I think that config is read-only. I will try the customizer here.

mcruzdev · 2024-11-03T18:57:20Z

Thank you, it works! Now I will continue here...

github-actions · 2024-11-03T20:15:03Z

🎊 PR Preview 9777061 has been successfully built and deployed to https://quarkus-pr-main-44272-preview.surge.sh/version/main/guides/

Images of blog posts older than 3 months are not available.
Newsletters older than 3 months are not available.

docs/src/main/asciidoc/security-jwt.adoc

.../deployment/src/main/java/io/quarkus/smallrye/jwt/deployment/SmallRyeJwtBuildTimeConfig.java

...ye-jwt/deployment/src/main/java/io/quarkus/smallrye/jwt/deployment/SmallRyeJwtProcessor.java

...-jwt/deployment/src/test/java/io/quarkus/jwt/test/devmode/KeyPairOutOfTheBoxDevModeTest.java

sberyozkin · 2024-11-03T22:21:15Z

Thanks @mcruzdev, IMHO it will be a nice addition, I've left a few comments, but it all is going well, thanks for the effort, and please take your time to address the comments

sberyozkin · 2024-11-11T22:20:42Z

Hey @mcruzdev , thanks for starting devui work, it looks promising, but let's deal with it in another PR, this PR is not finalised yet, and it already has too many comments for the DevUI work be accommodated...

Speaking of setting the issuer , why did you have to set it in props for generating new tokens if it is set in dev mode anyway ?

mcruzdev · 2024-11-11T22:45:39Z

Hey @mcruzdev , thanks for starting devui work, it looks promising, but let's deal with it in another PR, this PR is not finalised yet, and it already has too many comments for the DevUI work be accommodated...

Speaking of setting the issuer , why did you have to set it in props for generating new tokens if it is set in dev mode anyway ?

Sorry for both, I forgot to remove them from tests.

mcruzdev · 2024-11-12T02:32:03Z

Done @sberyozkin, I think now it is ok!

FroMage · 2024-11-12T09:46:03Z

Well, the point is to make it work by default, so we're not going to add an option.
The thing is that if you replace the key, all your JWT tokens become invalid, so application restarts will automatically log the users out. That's very inconvenient.

As far as I know devservices don't leave any state behind them... This is a dev mode, not prod... If the user logged in and the server reloaded in hot mode, all is good with the current PR... If the server was shutdown, then why can't a user start again... Where is a guarantee that with a cold restart no real verification key or location has been configured, the user will get 401 in this case... The latter can happen in any case once the properties become runtime ready.

Is the above convincing at all ? If you do prefer generating files in Renarde, perhaps we can introduce a build item which Renarde will produce and the code in this PR will consume to check if it still needs to generate a key pair in memory... Does it work for you ?

I'm not convinced. I also don't see why this is tied to a dev service, from the start. Sounds like a dev-mode build step, for sure.

Not being able to stay logged in after a restart feels like a step back from what I have in Renarde, and I haven't read a compelling argument for not generating a file for DEV mode so that cold restarts are supported.

What I mean is: what is the argument in favour of not supporting cold restarts for JWT sessions? What does it gain us? What is the advantage?

sberyozkin · 2024-11-12T18:21:11Z

Hi @FroMage

I also don't see why this is tied to a dev service, from the start. Sounds like a dev-mode build step, for sure.

Sure, I only asked @mcruzdev to have the build step in a class file whose name contains devservice to support possible Keycloak startup in the future, it is only a name, you are right that right now it is purely a devmode step

What I mean is: what is the argument in favour of not supporting cold restarts for JWT sessions? What does it gain us? What is the advantage?

My own understanding is that there should be no state left behind, it seems not great, but I have no strong opinion, I'd only like to have an option where nothing is generated on the disc.

So which is why I suggest we add a build item here in this PR, and Renarde will produce it, and if this build item is consumed here, then we just return without doing any in memory key generation....

Does it work ?

docs/src/main/asciidoc/security-jwt.adoc

FroMage · 2024-11-13T10:22:59Z

Sure, I only asked @mcruzdev to have the build step in a class file whose name contains devservice to support possible Keycloak startup in the future, it is only a name, you are right that right now it is purely a devmode step

Using that name is confusing when there is no dev service for this.

What I mean is: what is the argument in favour of not supporting cold restarts for JWT sessions? What does it gain us? What is the advantage?

My own understanding is that there should be no state left behind, it seems not great, but I have no strong opinion, I'd only like to have an option where nothing is generated on the disc.

So which is why I suggest we add a build item here in this PR, and Renarde will produce it, and if this build item is consumed here, then we just return without doing any in memory key generation....

Does it work ?

That's one option, but I think we should open the discussion to more people, as this is a UX issue, so let's ask @cescoffier and @maxandersen:

Here we're trying to auto-generate JWT keys in DEV mode, so users can get started with JWT without manually generating them. Otherwise JWT doesn't work OOTB.

We have the choice between two approaches:

This PR generates the keys in memory, which means they get re-generated every time we cold restart the application. This only works for hot-reload.
Renarde already supports this, by generating the keys in target/classes/dev.privateKey.pem which means they don't get re-generated until the user cleans the project. This works for cold restarts and hot reload.

Re-generating the keys mean the JWT cookies will become invalid and users need to re-login.

I find it better to generate a file and only force users to re-login on mvn clean, while Sergey prefers not generating a file and force users to re-login on cold restarts.

Any opinion?

sberyozkin · 2024-11-13T11:51:44Z

@FroMage Hi, sure, lets rename that, we can replace DevService with DevMode, @mcruzdev please change the processor file name to avoid the confusion, sorry I did not think about it earlier...

As far as hot start vs cold start is concerned, when we are in the DevUI screen which is what @mcruzdev already started experimenting with, we don't have cookies, and there is no login/logout flow, and similarly, when running tests. So here, leaving the generated files when the server is shutdown is not great IMHO...

Also as I said, I'm not sure that retaining files after the cold start always works, lets say you started, all works, then you shutdown, decide to put the real key verification material, and after the cold restart the user JWT cookie is not verified...

Can you consider requesting a user reauthentication in devmode if the current cookie can not be verified ? That would work with hot and cold restarts...

FroMage · 2024-11-13T14:05:34Z

As far as hot start vs cold start is concerned, when we are in the DevUI screen…

But this is for DEV mode and test mode, most people spend time in DEV or test mode than the DEV UI. If they're using cookies (and any web application will use cookies), this will affect them.

Also as I said, I'm not sure that retaining files after the cold start always works, lets say you started, all works, then you shutdown, decide to put the real key verification material, and after the cold restart the user JWT cookie is not verified...

This would work, because once you start adding a real key, you have configuration that points to the new key. This will indeed force you to relogin, but hey, you changed the key, that's fair.

Can you consider requesting a user reauthentication in devmode if the current cookie can not be verified ? That would work with hot and cold restarts...

This is what people are forced to do indeed, but that's adding an extra step and making the UX worse than it could be.

cescoffier · 2024-11-13T16:34:31Z

...deployment/src/main/java/io/quarkus/smallrye/jwt/deployment/SmallryeJwtDevModeProcessor.java

+
+    private static String getStringKey(Key key) {
+        return Base64.getEncoder()
+                .encodeToString(key.getEncoded());


I don't know that part of the codebase, but is this intended to be a PEM format?
RSA Key needs to use PKCS#1 format.

Hi @cescoffier It is intended to be a PEM format. mp.jwt.verify.publickey uses it, see here.

EDIT: intended to be a base64 encoded key

ok, this is NOT PEM format :-) This is just a base64 encoded key.
If you want PEM format you need the header and footer explaining what's the content (RSA KEY, PRIVATE KEY ...)

cescoffier · 2024-11-14T06:20:43Z

I agree with @FroMage about generating a proper PEM file (Be aware, this is a trap) in the output directory (target or build) so it can be reused between runs and used for both test and dev.

Now, generating a proper PEM file can be tricky because there are several formats: PKCS#1, PKCS#8, PKCS#7., SEC1 (EC)... Also, I learned that PKCS#8 can be encrypted. In this case, I would just pick one (but the runtime would need to support most of them, if not all)

sberyozkin · 2024-11-22T14:31:00Z

Hi @FroMage

But this is for DEV mode and test mode, most people spend time in DEV or test mode than the DEV UI. If they're using cookies (and any web application will use cookies), this will affect them.

Well, our plan is to offer a DevUI support for quarkus-smallrye-jwt users be able to interactively create signed JWTs, and having PEM files generated on the disk and have them staying around after the server is shutdown is unnecessary.
It probably won't even work if we go to the remote DevUI option again...

Also, while quarkus-smallrye-jwt can verify tokens provided as cookie values, most of its cases are about bearer access token verification where a token is generated and submitted - these cases would be supported in dev and test modes.

You say users spent most of their time in devmode, but in devmode one does not cold-stop the server, usually we do it if something goes wrong with the live coding. What is the scenario that you have in mind where a server is cold-stopped when the authenticated user is around ?
These users would get Can't access the server error while the server is down then, which is not a good UX. Having to re-login occasionally (rarely in devmode as cold-stopping the server in devmode should be rare) is the normal operational flow.

quarkus-bot · 2024-12-06T02:12:12Z

Status for workflow `Quarkus Documentation CI`

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

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

Warning

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

quarkus-bot · 2024-12-06T04:00:03Z

Status for workflow `Quarkus CI`

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

Failing Jobs

Status	Name	Step	Failures	Logs	Raw logs	Build scan
✖	Quickstarts Compilation - JDK 17	`Compile Quickstarts`	Failures	Logs	Raw logs	🚧

You can consult the Develocity build scans.

Failures

⚙️ Quickstarts Compilation - JDK 17 #

- Failing: security-webauthn-quickstart security-webauthn-reactive-quickstart

📦 security-webauthn-quickstart

✖ Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project security-webauthn-quickstart: Compilation failure

📦 security-webauthn-reactive-quickstart

✖ Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project security-webauthn-reactive-quickstart: Compilation failure

mcruzdev · 2024-12-18T21:57:15Z

@FroMage, @sberyozkin any update about this one? We have a direction to follow?

FroMage · 2024-12-19T09:59:56Z

Just rebase on the latest quarkus main and this will pass.

quarkus-bot bot added the area/smallrye label Nov 2, 2024

quarkus-bot bot added the area/core label Nov 3, 2024

quarkus-bot bot added the area/documentation label Nov 3, 2024