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

Add CLI command for Config #34493

Merged
merged 1 commit into from
Jan 25, 2024
Merged

Add CLI command for Config #34493

merged 1 commit into from
Jan 25, 2024

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Jul 3, 2023

Adds a top-level Config CLI with two subcommands:

  • Set
    • Can Add, Update and Remove a configuration from application.properties
    • Will encrypt the value with the -k flag.
  • Encryptor
    • Can encrypt arbitrary values in AES/GCM/NoPadding (will support more algorithms)

Requires SmallRye Config 3.4.x (still to be released).

@quarkus-bot quarkus-bot bot added area/cli Related to quarkus cli (not maven/gradle/etc.) area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins labels Jul 3, 2023
@maxandersen
Copy link
Member

Could this not be a quarkus plugin that is contributed by relevant extension?

/Cc @iocanel

@sberyozkin
Copy link
Member

sberyozkin commented Jul 3, 2023

Great stuff @radcortez, thanks for this PR, I have a few comments, but looks like a perfect start, cheers

@quarkus-bot

This comment has been minimized.

1 similar comment
@quarkus-bot

This comment has been minimized.

@iocanel
Copy link
Contributor

iocanel commented Jul 4, 2023

Could this not be a quarkus plugin that is contributed by relevant extension?

/Cc @iocanel

Yeah. AFAIR I have created a PR for the file vault extension that contributed a CLI plugin: quarkiverse/quarkus-file-vault#67. Don't recall why it wasn't merged but we could definitely do it like that.

@sberyozkin
Copy link
Member

@iocanel Yeah, I recall the PR, thanks for opening it, the reason it was not merged was because it is in the Quarkiverse extensions, but now it evolves in the Quarkus CLI

@radcortez
Copy link
Member Author

Would you like us to move this outside the main CLI? Since this is Config related, all the Config integration is done at the core level, so there is no Config extension to add this. The file vault behaviour is now provided by SmallRye Config core and a couple of dependencies: SmallRye Config Source Keystore and SmallRye Config Crypto.

@maxandersen
Copy link
Member

Would you like us to move this outside the main CLI? Since this is Config related, all the Config integration is done at the core level, so there is no Config extension to add this. The file vault behaviour is now provided by SmallRye Config core and a couple of dependencies: SmallRye Config Source Keystore and SmallRye Config Crypto.

ah! that is a good point - I hadn't thought about it from that perspective. I somehow thought it was only relevant for OIDC /security stuff but yeah it is not really.

@maxandersen
Copy link
Member

okey so if we want it in the default quarkus cli it needs to actually "fit".

what is the actual usage pattern here?

the old tool at https://github.com/quarkiverse/quarkus-file-vault/tree/main/vault-utils printed out:

######################################################################################################
Please add the following parameters on your application.properties file, and replace the <name> value!
The <name> will be used in the consumer to refer to this provider.

quarkus.file.vault.provider.<name>.encryption-key=the_encryption_key
quarkus.file.vault.provider.<name>.secret=4VLLc9bk+WMnQMR3ezJcpw==
######################################################################################################

this script seem to just be about generating the encrypted value for a key/property?

so if i grok it right ultimately what we would do is do allow something like:

quarkus config encrypt --secret blah.file quarkus.datasource.password=blablah and it would go and actually update the properties file for you ?

now it just prints the value - which is nice; but in quarkus we should be able to do that for the user, right?

@Option(required = true, names = { "-k", "--encryption-key" }, description = "Encryption Key")
String encryptionKey;

@Option(hidden = true, names = { "-a", "--algorithm" }, description = "Algorithm", defaultValue = "AES")
Copy link
Member

Choose a reason for hiding this comment

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

is there some hints in users config on what algorithm they might be using for this so default value could be figured out from there ?

not a blocker for this pr - just trying to think what could be best in future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the secret handler expression. For instance, to set a secret value and a way to decrypt it you need something like:

quarkus.datasource.password=${aes-gcm-nopadding::SECRET}
smallrye.config.secret-handler.aes-gcm-nopadding.encryption-key=key

But I'm probably going to change that to be more generic, like:

quarkus.datasource.password=${dec::SECRET}
smallrye.config.secret-handler.dec.alg=AES
...

The only problem is that you can have multiple handlers. We can discover all the available handlers and the user can select which one to use. We can also retrieve the encryption key if available (not likely, because I would expect that the encryption key will not be stored in application.properties or any other source available to the CLI, but we can do it as a best effort).

@radcortez
Copy link
Member Author

now it just prints the value - which is nice; but in quarkus we should be able to do that for the user, right?

Yes, it would be possible if we know the configuration property name. With the current Secret Handler approach and encryption, it is possible for the user to encrypt any key,

Maybe an alternative way is to just pass the configuration property name and we can pick the value, encrypt it and update the configuration file. Note that if the user has higher ordinal sources or sources that we cannot update, the user will need to do it manually.

@quarkus-bot

This comment has been minimized.

@maxandersen
Copy link
Member

okey. then my suggestion is that instead of creating a high-level "encryptor" we make a top level "config" with an "encrypt" sub command. as we can imagine adding other helpful config commands to the cli.

wdyt?

@ebullient @iocanel

@radcortez
Copy link
Member Author

Sounds good to me :)

@ebullient
Copy link
Member

okey. then my suggestion is that instead of creating a high-level "encryptor" we make a top level "config" with an "encrypt" sub command. as we can imagine adding other helpful config commands to the cli.

wdyt?

@ebullient @iocanel

sounds reasonable ...

you're thinking a la git config ... ?

@maxandersen
Copy link
Member

@ebullient yeah - though we actually have two kind of configs... the config of the app and the config of the cli...
for quarkus config i'm thinking mainly the app config part.

@radcortez radcortez changed the title Add CLI command to encrypt secrets Add CLI command for Config Jul 7, 2023
@radcortez radcortez marked this pull request as draft July 7, 2023 17:03
@radcortez
Copy link
Member Author

I've updated the PR (and the description) with some of the ideas. If we are happy with the direction, then I can move forward and complete the PR.

@sberyozkin
Copy link
Member

Thanks @radcortez for making the key optional - as various algorithms may require different length, etc, should be easier for users if they don't already have the one.
LGTM, thanks

@radcortez radcortez marked this pull request as ready for review July 11, 2023 01:08
@quarkus-bot

This comment has been minimized.

@radcortez radcortez force-pushed the fix-29172 branch 3 times, most recently from dee3bad to d24e62e Compare July 12, 2023 22:59
@github-actions
Copy link

github-actions bot commented Jul 12, 2023

🙈 The PR is closed and the preview is expired.

@radcortez radcortez marked this pull request as draft July 17, 2023 16:04
@quarkus-bot quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label Jul 17, 2023
@radcortez radcortez force-pushed the fix-29172 branch 2 times, most recently from c38dfd6 to 0a09ab7 Compare July 19, 2023 19:14
@radcortez radcortez marked this pull request as ready for review July 19, 2023 19:15
@quarkus-bot

This comment has been minimized.

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.

Let's get this in and gather feedback!

@maxandersen
Copy link
Member

wanted to use this today and noticed it isn't in...anything holding it back @radcortez ?

@radcortez
Copy link
Member Author

Nothing is holding it back. Let me rebase this with the latest.

@radcortez radcortez merged commit bd6465f into quarkusio:main Jan 25, 2024
22 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.9 - main milestone Jan 25, 2024
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Related to quarkus cli (not maven/gradle/etc.) area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation kind/enhancement New feature or request release/noteworthy-feature
Projects
Development

Successfully merging this pull request may close these issues.

Integrate FileVault Utils with Quarkus CLI
6 participants