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.

Sign up for GitHub

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

Jump to bottom

improvements to the keycloak_realm_key module #7698

Merged
merged 6 commits into from
Dec 28, 2023
Merged

improvements to the keycloak_realm_key module #7698

merged 6 commits into from
Dec 28, 2023

Conversation

gbolo
Copy link
Contributor

@gbolo gbolo commented Dec 7, 2023 •
edited
Loading

SUMMARY

This PR makes the following improvements to the keycloak_realm_key module:

  • adds support for RSA enc key usage rsa-enc
  • adds support for an additional 8 key algorithms
  • fixes a bug where the user must specify a certificate (no longer required)
  • removes no_log for certificate since this is not sensitive information
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

module: keycloak_realm_key

ADDITIONAL INFORMATION

Tested against Keycloak v22.0.4 API.

@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot
Copy link
Collaborator

cc @eikef @mattock @ndclt
click here for bot help

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI feature This issue/PR relates to a feature request module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor plugins plugin (any type) labels Dec 7, 2023
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Dec 7, 2023
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-8 Automatically create a backport for the stable-8 branch labels Dec 7, 2023
felixfontein
felixfontein reviewed Dec 8, 2023
View reviewed changes
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@gbolo
Copy link
Contributor Author

gbolo commented Dec 8, 2023

Thanks for your contribution!

My pleasure. I made the suggested code changes, thanks for putting the time in to review this PR.

@danekja
Copy link
Contributor

danekja commented Dec 13, 2023

If I may (not a maintainer, merely a user of the collection) - I would propose to cover the whole set of supported algorithms by tests. And thanks for contribution, we were going to implement this addition ourselves in the following months as well.

@gbolo
Copy link
Contributor Author

gbolo commented Dec 14, 2023

If I may (not a maintainer, merely a user of the collection) - I would propose to cover the whole set of supported algorithms by tests. And thanks for contribution, we were going to implement this addition ourselves in the following months as well.

There is also some other functionality I have working but did not include in this PR, which involves managing the generated keys (for example the rsa-generated). In my use case, I create new keys and remove the default realm generated ones. But I wanted to keep this PR small so that it gets quickly merged. Maybe I can submit another PR for that and include test cases there. @felixfontein is there anything else needed to get this merged?

@felixfontein
Copy link
Collaborator

Some feedback from the module maintainers would be nice.

In general the change looks OK to me, except that default / removal of required=true. The module simply passes the values on to the API, so I don't think unit tests are needed. (Integration tests would be nice, but these don't run in CI, so their value is limited.)

mattock
mattock reviewed Dec 15, 2023
View reviewed changes
plugins/modules/keycloak_realm_key.py
"PS512",
"RSA1_5",
"RSA-OAEP",
"RSA-OAEP-256",
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding these makes sense. I only had a need for RS256 and I did not want to overreach. However, I think adding one integration test per algorithm would be useful as the list of supported algorithms might change over time. Or these might be a typo in there 😄.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I can add some basic integration tasks to check these I guess, although some algorithms belong on certain key types only. For example, you would not want to attempt to set RSA-OAEP on your signing key. I was hoping that the end user of this module would understand the values they are setting so that we don't have to write too much validation code. Ideally the keycloak api will return an error in these cases so we can just pass that error on. I think maybe we should tackle this in another PR though...

felixfontein
felixfontein approved these changes Dec 22, 2023
View reviewed changes
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Looks good to me. If nobody objects, I'll merge this in ~a week.

@felixfontein felixfontein merged commit 702dd9b into ansible-collections:main Dec 28, 2023
@patchback Patchback
Copy link

patchback bot commented Dec 28, 2023 •
edited
Loading

Backport to stable-8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-8/702dd9bbda63b42e705025a1296c062ab5334284/pr-7698

Backported as #7768

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Dec 28, 2023
patchback bot pushed a commit that referenced this pull request Dec 28, 2023
@gbolo @patchback
improvements to the keycloak_realm_key module (#7698)
fe9b7a2 
* add support for rsa enc key usage, more algorithms, and make certficate optional

* fix formatting

* adding changelog fragment

* made suggested code changes based on review

* fix typo and be more clear

* revert certificate to previous defined settings

(cherry picked from commit 702dd9b)
@patchback patchback bot mentioned this pull request Dec 28, 2023
Merged
@felixfontein
Copy link
Collaborator

@gbolo thanks for your contribution!
@danekja @mattock thanks for reviewing!

felixfontein pushed a commit that referenced this pull request Dec 28, 2023
@patchback @gbolo
[PR #7698/702dd9bb backport][stable-8] improvements to the keycloak_r…
c596558 
…ealm_key module (#7768)

improvements to the keycloak_realm_key module (#7698)

* add support for rsa enc key usage, more algorithms, and make certficate optional

* fix formatting

* adding changelog fragment

* made suggested code changes based on review

* fix typo and be more clear

* revert certificate to previous defined settings

(cherry picked from commit 702dd9b)

Co-authored-by: George Bolo <george.bolo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein approved these changes

@mattock mattock Awaiting requested review from mattock

Assignees
No one assigned
Labels
backport-8 Automatically create a backport for the stable-8 branch feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor plugins plugin (any type)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gbolo @ansibullbot @danekja @felixfontein @mattock