Skip to content

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

Merged
mposolda merged 1 commit into
keycloak:mainfrom
fbrissi:feature/KEYCLOAK-19519
Feb 17, 2022
Merged

KEYCLOAK-19519 Encryption algorithm RSA-OAEP with A256GCM#8553
mposolda merged 1 commit into
keycloak:mainfrom
fbrissi:feature/KEYCLOAK-19519

Conversation

@fbrissi

@fbrissi fbrissi commented Oct 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

tnorimat commented Nov 20, 2021

Copy link
Copy Markdown
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

fbrissi commented Nov 22, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

fbrissi commented Jan 18, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@tnorimat

tnorimat commented Jan 24, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@tnorimat

tnorimat commented Jan 24, 2022

Copy link
Copy Markdown
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

fbrissi commented Jan 26, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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

mposolda commented Jan 31, 2022

Copy link
Copy Markdown
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

fbrissi commented Jan 31, 2022

Copy link
Copy Markdown
Contributor Author

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

fbrissi commented Feb 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hey guys, I made the suggested changes

tnorimat
tnorimat suggested changes Feb 4, 2022
View reviewed changes

@tnorimat tnorimat left a comment

Copy link
Copy Markdown
Contributor

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?

Comment thread server-spi-private/src/main/java/org/keycloak/models/utils/DefaultKeyProviders.java Outdated
Comment thread server-spi-private/src/main/java/org/keycloak/models/utils/DefaultKeyProviders.java Outdated
Comment thread services/src/main/java/org/keycloak/keys/AbstractImportedRsaKeyProviderFactory.java Outdated
Comment thread services/src/main/java/org/keycloak/keys/ImportedRsaEncKeyProviderFactory.java Outdated
Comment thread server-spi-private/src/main/java/org/keycloak/models/utils/DefaultKeyProviders.java Outdated
Comment thread server-spi-private/src/main/java/org/keycloak/models/utils/DefaultKeyProviders.java Outdated
@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

@tnorimat tnorimat left a comment

Copy link
Copy Markdown
Contributor

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?

Comment thread ...uillian/tests/base/src/test/java/org/keycloak/testsuite/keys/ImportedRsaKeyProviderTest.java Outdated
@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

@tnorimat tnorimat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@tnorimat

tnorimat commented Feb 8, 2022

Copy link
Copy Markdown
Contributor

@mposolda Could you check this PR?

mposolda
mposolda approved these changes Feb 17, 2022
View reviewed changes

@mposolda mposolda left a comment

Copy link
Copy Markdown
Contributor

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

@mposolda mposolda mposolda approved these changes
+1 more reviewer
@tnorimat tnorimat tnorimat approved these changes
Reviewers whose approvals may not affect merge requirements

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