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

[oidc] Clarify startup warning: Secret key for encrypting tokens should be 32 characters long #33532

Closed
rgmz opened this issue May 22, 2023 · 3 comments · Fixed by #33550
Closed
Assignees
Milestone

Comments

@rgmz
Copy link
Contributor

rgmz commented May 22, 2023

Description

When starting Quarkus, the following warning message will be displayed if your client secret is less than 32 characters in length.

Configuration

quarkus:
  oidc:
    client-id: "example-client-id.apps.googleusercontent.com"
    credentials:
      secret: "example-client-secret"

Logs

__  ____  __  _____   ___  __ ____  ______ 
 --/ __ \/ / / / _ | / _ \/ //_/ / / / __/ 
 -/ /_/ / /_/ / __ |/ , _/ ,< / /_/ /\ \   
--\___\_\____/_/ |_/_/|_/_/|_|\____/___/   
2023-05-22 16:18:03,236 WARN  [io.qua.oid.run.TenantConfigContext] (vert.x-eventloop-thread-1) Secret key for encrypting tokens should be 32 characters long

It is unclear what the significance of this warning is or what action should be taken by users. Furthermore a client secret is usually something generated by an external Identity Provider that you cannot customize.

Looking at #32192, and specifically the TenantConfigContext#createTokenEncSecretKey method, it appears that Quarkus is using the client secret (quarkus.oidc.credentials.secret) to encrypt the OIDC session cookie. However, this is not mentioned in the OIDC configuration reference or the Migration Guide and it's unclear whether this is intentional or if it should be referencing quarkus.oidc.token-state-manager.encryption-secret (as mentioned in the migration guide).

if (secretBytes.length < 32) {
LOG.warn("Secret key for encrypting tokens should be 32 characters long");
}

Implementation ideas

Add further clarifications to the warning message so that users can easily tell:

  • what the issue is,
  • how severe the issue is,
  • and how they can fix it
@rgmz rgmz added the kind/enhancement New feature or request label May 22, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented May 22, 2023

/cc @pedroigor (oidc), @sberyozkin (oidc)

@sberyozkin
Copy link
Member

sberyozkin commented May 22, 2023

Sure, that can be clarified in the log message.

it appears that Quarkus is using the client secret (quarkus.oidc.credentials.secret) to encrypt the OIDC session cookie.

It falls back to it if the documented quarkus.oidc.token-state-manager.encryption-secret is not set, the log message should recommend it or suggest disabling the encryption if users think it is safe in their setup.

how severe the issue is

The warning message can say that a key length should correspond to the encryption algorithm key requirements (in a shorter form) for the best encryption output be produced

@sberyozkin sberyozkin self-assigned this May 22, 2023
@rgmz
Copy link
Contributor Author

rgmz commented May 22, 2023

It falls back to it if the documented quarkus.oidc.token-state-manager.encryption-secret is not set

It's worth documenting this under quarkus.oidc.token-state-manager.encryption-secret since that fallback isn't obvious right now if you look through the config reference:

/**
* Secret which will be used to encrypt the tokens.
* This secret must be set if the token encryption is required but no client secret is set.
* The length of the secret which will be used to encrypt the tokens must be 32 characters long.
*/
@ConfigItem
public Optional<String> encryptionSecret = Optional.empty();

e.g.

          * The length of the secret which will be used to encrypt the tokens must be 32 characters long. 
+         * <p>
+         * If this value is not set, Quarkus will fallback to either `quarkus.oidc.credentials.secret` 
+         * or `quarkus.oidc.credentials.jwt.secret-provider.key` (I think?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants