Improve log messages related to OIDC session cookie encryption secret #33550
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
geoand
reviewed
May 23, 2023
extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java
Outdated
Show resolved
Hide resolved
sberyozkin
force-pushed
the
oidc_improve_key_enc_warn_message
branch
2 times, most recently
from
94283fc to
323127b
Compare
|
I'll add another comment as proposed at #33532 (comment) after all a bit later :-) |
sberyozkin
force-pushed
the
oidc_improve_key_enc_warn_message
branch
2 times, most recently
from
6e14f39 to
dc4ccde
Compare
This comment has been minimized.
This comment has been minimized.
gsmet
previously requested changes
May 30, 2023
extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/TenantConfigContext.java
Outdated
Show resolved
Hide resolved
sberyozkin
force-pushed
the
oidc_improve_key_enc_warn_message
branch
from
95fee00 to
2ca90c2
Compare
|
✔️ 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. |
pedroigor
approved these changes
May 31, 2023
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #33532.
@geoand Can you please review and help to improve the wording ? No OIDC logic has been affected, only more log messages are added, and the warning is issued when the encryption key strength is low.
@rgmz FYI.
Note, OIDC code could've avoided falling back to the configured secrets and just generated a random secret but that would increase the risk of the applications being broken (after restarts in devmode, multiple pods) and force users disable the encryption in any case. Seeing a log message is less intrusive.
What I have also done is to log a warning when the key strength is very low (less than 16 chars) but only debug when the key is strong enough - that should improve the experience as the secrets generated by providers will most definitelybe at least 16 characters long. It looks like the warning message can frustrate quite a few users - the whole session cookie encryption is a hardening fix, so for users running securely over HTTPS or internal networks it can be a bit annoying having to go and disable the encryption just to get rid of the warning.
I also did not go into all the details how the fallback works, as the client secret can be configured in one of the 2 ways, and then if it is a
client_jwt_secretauthentication between Quarkus and OIDC then it is a jwt secret property, and listing all of that would make a difficult to follow doc. But I added more log messages to show how the fallback might work.