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

KEYCLOAK-19519 Encryption algorithm RSA-OAEP with A256GCM #8553

Merged
merged 1 commit into from
Feb 17, 2022
Merged

KEYCLOAK-19519 Encryption algorithm RSA-OAEP with A256GCM #8553

merged 1 commit into from
Feb 17, 2022

Conversation

fbrissi
Copy link
Contributor

@fbrissi fbrissi commented Oct 8, 2021 •
edited
Loading

Based on #8243

Jira issue KEYCLOAK-19519

@fbrissi fbrissi marked this pull request as draft November 20, 2021 00:27
@fbrissi fbrissi force-pushed the feature/KEYCLOAK-19519 branch from 46c3091 to 1ee18f7 Compare November 20, 2021 00:30
@tnorimat
Copy link
Contributor

Hello @fbrissi , could #8708 resolve this issue?

@fbrissi fbrissi force-pushed the feature/KEYCLOAK-19519 branch from a9a123b to 655dce5 Compare November 22, 2021 13:31
@fbrissi
Copy link
Contributor Author

fbrissi commented Nov 22, 2021 •
edited
Loading

Hello @fbrissi , could #8708 resolve this issue?

@tnorimat It's different, because the #8708 generate the key, and my implement can be import an key in RSA provider, I changed the interface to be select Algorithm by Key User.

image
image

@fbrissi fbrissi marked this pull request as ready for review November 22, 2021 13:41
@fbrissi
Copy link
Contributor Author

fbrissi commented Jan 18, 2022 •
edited
Loading

@tnorimat do you have a question about this?
I want know if i answered your question.

@tnorimat
Copy link
Contributor

tnorimat commented Jan 24, 2022 •
edited
Loading

@fbrissi Hello, It seems that this PR lacks the integration tests.

@tnorimat
Copy link
Contributor

@fbrissi In #8708, I've done the same thing for generated key as this PR does for imported key.

I've proposed two options in #8708 (comment) and taken the option 1. It seems that this PR may take the option 2.

Could you consider the option 1? Namely providing ImportedRsaKey for use = sig and ImportedRsaKey for use = enc.
By doing so, it might be not needed to modify Keycloak's core component used elsewhere, eg. ProviderConfigProperty and so on.

@mposolda what do you think about it?

@fbrissi
Copy link
Contributor Author

fbrissi commented Jan 26, 2022 •
edited
Loading

@tnorimat I think I understood.

You suggest option 1, split into 2 providers, one for enc and the other for sig, similarly that you do that in generated, this is option 1.

It's ok for me. Being able to separate somehow It's ok.

I'm waiting for an opinion from @mposolda.

@mposolda
Copy link
Contributor

@fbrissi +1 to the proposal of @tnorimat

@mposolda mposolda requested a review from tnorimat January 31, 2022 09:15
@mposolda mposolda added the area/oidc Indicates an issue on OIDC area label Jan 31, 2022
@fbrissi
Copy link
Contributor Author

fbrissi commented Jan 31, 2022

Right, thanks guys.
I'm going to change, and I'm converting to a draft for now.

@fbrissi fbrissi marked this pull request as draft January 31, 2022 13:49
@fbrissi fbrissi force-pushed the feature/KEYCLOAK-19519 branch 2 times, most recently from 8bf7341 to 7948d8d Compare February 2, 2022 21:53
@fbrissi fbrissi marked this pull request as ready for review February 2, 2022 21:55
@fbrissi
Copy link
Contributor Author

fbrissi commented Feb 2, 2022 •
edited
Loading

Hey guys, I made the suggested changes

tnorimat
tnorimat suggested changes Feb 4, 2022
View reviewed changes
Copy link
Contributor

@tnorimat tnorimat left a comment

Choose a reason for hiding this comment

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

@fbrissi I've added some review comments. Could you check them?

server-spi-private/src/main/java/org/keycloak/models/utils/DefaultKeyProviders.java Outdated Show resolved Hide resolved
server-spi-private/src/main/java/org/keycloak/models/utils/DefaultKeyProviders.java Outdated Show resolved Hide resolved
services/src/main/java/org/keycloak/keys/AbstractImportedRsaKeyProviderFactory.java Outdated Show resolved Hide resolved
services/src/main/java/org/keycloak/keys/ImportedRsaEncKeyProviderFactory.java Outdated Show resolved Hide resolved
server-spi-private/src/main/java/org/keycloak/models/utils/DefaultKeyProviders.java Outdated Show resolved Hide resolved
server-spi-private/src/main/java/org/keycloak/models/utils/DefaultKeyProviders.java Outdated Show resolved Hide resolved
@fbrissi fbrissi force-pushed the feature/KEYCLOAK-19519 branch from 7948d8d to 085e69e Compare February 6, 2022 12:53
@fbrissi fbrissi requested a review from tnorimat February 6, 2022 13:04
@fbrissi fbrissi force-pushed the feature/KEYCLOAK-19519 branch from 085e69e to cfd0148 Compare February 6, 2022 14:47
tnorimat
tnorimat suggested changes Feb 7, 2022
View reviewed changes
Copy link
Contributor

@tnorimat tnorimat left a comment

Choose a reason for hiding this comment

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

@fbrissi I've added one review comment. Could you check it?

...uillian/tests/base/src/test/java/org/keycloak/testsuite/keys/ImportedRsaKeyProviderTest.java Outdated Show resolved Hide resolved
@fbrissi fbrissi force-pushed the feature/KEYCLOAK-19519 branch from cfd0148 to 55e5268 Compare February 7, 2022 19:19
@fbrissi fbrissi requested a review from tnorimat February 7, 2022 19:21
tnorimat
tnorimat approved these changes Feb 8, 2022
View reviewed changes
Copy link
Contributor

@tnorimat tnorimat left a comment

Choose a reason for hiding this comment

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

LGTM

@tnorimat
Copy link
Contributor

tnorimat commented Feb 8, 2022

@mposolda Could you check this PR?

mposolda
mposolda approved these changes Feb 17, 2022
View reviewed changes
Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

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

@fbrissi Thanks for the PR!

@tnorimat Thanks for the review! I am approving mostly based on your review since you are most familiar with this codebase due your recent work on this.

Before merging this, I am trying to rerun the GH actions. Also it will be needed to create GH issue instead of JIRA and add this issue to the commit message (Although I can add it when I am merging PR as I can change commit message and attach GH issue to it. So no need to update PR and change commit message just because of this).

@mposolda mposolda merged commit 323c08c into keycloak:main Feb 17, 2022
@fbrissi fbrissi deleted the feature/KEYCLOAK-19519 branch February 18, 2022 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnorimat tnorimat tnorimat approved these changes

@mposolda mposolda mposolda approved these changes

Assignees

@mposolda mposolda

@tnorimat tnorimat

Labels
area/oidc Indicates an issue on OIDC area
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fbrissi @tnorimat @mposolda