Add ECDH-ES JWE Algorithm Provider by justin-tay · Pull Request #23928 · keycloak/keycloak

justin-tay · 2023-10-12T02:58:36Z

This adds support for the ECDH-ES, ECDH-ES+A128KW, ECDH-ES+A192KW and ECDH-ES+A256KW JWE algorithms.

This required changes to the JWEAlgorithmProvider interface.

For decoding the CEK the following parameters were needed

The JWE Header in order to get the alg, enc, epk, apu and apv attributes
The JWE Encryption Provider in order to determine the CEK length if ECDH-ES is used

For encoding the CEK the following parameters were needed

The JWE Header Builder in order to get the alg, enc, apu and apv attributes and set the epk, apu and apv attributes

This also required changes to the JWEHeader to add the epk, apu and apv attributes and to make it mutable via a builder.

keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2024-08-05T13:36:24Z

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.saml.AudienceProtocolMappersTest#testAudienceResolveFullScope

Keycloak CI - Base IT (6)

jakarta.ws.rs.InternalServerErrorException: HTTP 500 Internal Server Error
	at org.jboss.resteasy.client.jaxrs.internal.ClientInvocation.handleErrorStatus(ClientInvocation.java:250)
	at org.jboss.resteasy.client.jaxrs.internal.proxy.extractors.DefaultEntityExtractorFactory$3.extractEntity(DefaultEntityExtractorFactory.java:41)
	at org.jboss.resteasy.client.jaxrs.internal.proxy.ClientInvoker.invokeSync(ClientInvoker.java:136)
	at org.jboss.resteasy.client.jaxrs.internal.proxy.ClientInvoker.invoke(ClientInvoker.java:103)
...

Report flaky test

tnorimat

@justin-tay Thank you. I added some review comments. Could you check them?

BCFIPSEcdhEsAlgorithmProvider, ElytronEcdhEsAlgorithmProvider, and BCEcdhEsAlgorithmProvider have a lot of shared codes.

Also, BCFIPSEcdhEsAlgorithmProviderTest, ElytronEcdhEsAlgorithmProviderTest, and BCEcdhEsAlgorithmProviderTest have a lot of shared codes.

It might be good if you consider designing those classes to reduce redundant codes.

tnorimat · 2024-08-06T04:26:44Z

Could you replace 2016 with 2024 or 2023?

Have replaced it with 2024.

tnorimat · 2024-08-06T04:30:42Z

How about setting ecdhAlgorithm directly?

return createKeyWrapper(keyPair, ecdhAlgorithm, KeyUse.ENC);

Have set the ecdhAlgorithm directly as suggested.

tnorimat · 2024-08-06T04:48:12Z

How about the following (inserting a space) ?

if (Algorithm.ECDH_ES.equals(algorithm)) {

Have fixed the formatting.

justin-tay · 2024-08-06T08:59:35Z

@tnorimat Thanks for the review! You are correct that there appears to be shared codes between the different EcdhEsAlgorithmProvider implementations and tests but I'm not sure where would be an appropriate place to put them as the crypto modules seem quite self contained and in a lot of cases there are also subtle differences in the implementation due to the differences in the API provided between BC, BCFIPS and Elytron so it's not that easy to share.

tnorimat · 2024-08-07T07:10:50Z

@justin-tay I see, so we keep your original codes in each BC, BCFIPS and Elytron modules even if these have shared codes.

rmartinc · 2024-08-07T07:24:59Z

@tnorimat @justin-tay The problem with duplicated code in crypto projects is old and it happens with other files (for example check BCFIPSOCSPProvider and BCOCSPProvider and many others). Maybe one possible solution is creating a crypto common project and put there the shared classes. But I think that is clearly outside the scope of this PR.

rmartinc

Thanks @justin-tay! For me it's OK now. Approved!

tnorimat

LGTM
@justin-tay Thank you very much for your contribution.

justin-tay · 2024-08-07T10:14:18Z

@rmartinc @tnorimat Thanks a lot for your time in doing the review! Let me know if I should squash and rebase the changes.

mposolda

@justin-tay @rmartinc @tnorimat Thanks everyone for the review!

I have only small concern that this test adds huge amount of new test classes - like 25 new classes of KcOidcBrokerJWEEcdh* . I appreciate the good test coverage, but at the same time, it can be quite an overhead... Is it maybe possible to keep around 5-6 most relevant combinations and delete the rest of those test classes from this PR? WDYT?

mposolda · 2024-08-07T12:37:35Z

@justin-tay Also just an update, that you don't need to care about squashing commits for now. I can squash the commits during merging the PR.

justin-tay · 2024-08-07T13:02:24Z

@mposolda Thanks for the update! Have left in 8 of the tests and removed the rest.

keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2024-08-07T13:44:43Z

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.forms.BruteForceTest#testRaceAttackPermanentLockout

Keycloak CI - Forms IT (chrome)

java.lang.AssertionError: Invalid count should be less than or equal 2 but was: 3
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.forms.BruteForceTest.raceAttack(BruteForceTest.java:820)
	at org.keycloak.testsuite.forms.BruteForceTest.testRaceAttackPermanentLockout(BruteForceTest.java:790)
...

Report flaky test

org.keycloak.testsuite.actions.RequiredActionUpdateProfileTest#updateProfile

Keycloak CI - Forms IT (firefox)

org.openqa.selenium.TimeoutException: 
Navigation timed out after 10000 ms
Build info: version: '4.21.0', revision: '79ed462ef4'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.5.0-1025-azure', java.version: '21.0.4'
Driver info: org.openqa.selenium.firefox.FirefoxDriver
...

Report flaky test

mposolda · 2024-08-07T13:45:19Z

@justin-tay Thanks! The update is needed in fips-suite file as well. Besides that, I hope we can merge.

Closes keycloak#23596 Signed-off-by: Justin Tay <49700559+justin-tay@users.noreply.github.com>

Closes keycloak#23597 Signed-off-by: Justin Tay <49700559+justin-tay@users.noreply.github.com>

Signed-off-by: Justin Tay <49700559+justin-tay@users.noreply.github.com>

keycloak-github-bot · 2024-08-08T08:12:48Z

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.javascript.JavascriptAdapterTest#testSilentCheckSso

Keycloak CI - Base IT (5)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:307)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

keycloak-github-bot

Unreported flaky test detected, please review

mposolda

@justin-tay @tnorimat @rmartinc Thanks again to everyone for the participation on this PR!

justin-tay requested review from a team as code owners October 12, 2023 02:58

justin-tay requested a review from a team October 12, 2023 02:58

justin-tay requested a review from a team as a code owner October 12, 2023 02:58

ghost added team/core team/cloud-native labels Oct 12, 2023

justin-tay force-pushed the gh23597 branch from cedee1d to c50dec4 Compare February 13, 2024 08:10

justin-tay requested review from a team as code owners February 13, 2024 08:10

keycloak-github-bot Bot added team/core-clients team/core-iam labels Feb 13, 2024

stianst added team/core-shared and removed team/core labels Feb 13, 2024

justin-tay force-pushed the gh23597 branch from c50dec4 to 5987cec Compare June 25, 2024 07:38

keycloak-github-bot Bot added the flaky-test label Jun 25, 2024

This was referenced Jun 25, 2024

Flaky test: org.keycloak.testsuite.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled #30188

Closed

Flaky test: org.keycloak.testsuite.organization.exportimport.OrganizationExportTest#testExport #30598

Closed

justin-tay force-pushed the gh23597 branch from 5987cec to 09339a9 Compare June 25, 2024 08:52

keycloak-github-bot Bot mentioned this pull request Jun 25, 2024

Flaky test: org.keycloak.testsuite.oauth.TokenIntrospectionTest#testUnsupportedToken #30111

Closed

justin-tay force-pushed the gh23597 branch from 09339a9 to 040409f Compare July 26, 2024 13:07

keycloak-github-bot Bot mentioned this pull request Aug 5, 2024

Flaky test: org.keycloak.testsuite.forms.BruteForceTest#testRaceAttackPermanentLockout #31881

Closed

keycloak-github-bot Bot reviewed Aug 5, 2024

View reviewed changes

tnorimat reviewed Aug 6, 2024

View reviewed changes

rmartinc approved these changes Aug 7, 2024

View reviewed changes

tnorimat approved these changes Aug 7, 2024

View reviewed changes

mposolda reviewed Aug 7, 2024

View reviewed changes

keycloak-github-bot Bot mentioned this pull request Aug 7, 2024

Flaky test: org.keycloak.testsuite.actions.RequiredActionUpdateProfileTest#updateProfileWithoutRemoveCustomAttributes #28953

Closed

keycloak-github-bot Bot reviewed Aug 7, 2024

View reviewed changes

mposolda previously approved these changes Aug 7, 2024

View reviewed changes

justin-tay dismissed mposolda’s stale review via d33f944 August 7, 2024 14:09

justin-tay added 6 commits August 8, 2024 15:29

Add generated ECDH key provider

ef05914

Closes keycloak#23596 Signed-off-by: Justin Tay <49700559+justin-tay@users.noreply.github.com>

Add ECDH-ES JWE Algorithm Provider

eb217cd

Closes keycloak#23597 Signed-off-by: Justin Tay <49700559+justin-tay@users.noreply.github.com>

Leave apu and apv empty and fix ephemeral key generation

7930c5c

Signed-off-by: Justin Tay <49700559+justin-tay@users.noreply.github.com>

Address review findings

e29cad9

Signed-off-by: Justin Tay <49700559+justin-tay@users.noreply.github.com>

Remove some tests

cb629e0

Signed-off-by: Justin Tay <49700559+justin-tay@users.noreply.github.com>

Remove tests for fips-suite

6c91a2a

Signed-off-by: Justin Tay <49700559+justin-tay@users.noreply.github.com>

justin-tay force-pushed the gh23597 branch from d33f944 to 6c91a2a Compare August 8, 2024 07:29

keycloak-github-bot Bot reviewed Aug 8, 2024

View reviewed changes

mposolda approved these changes Aug 8, 2024

View reviewed changes

mposolda merged commit 966a454 into keycloak:main Aug 8, 2024

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Unreported flaky test detected

org.keycloak.testsuite.saml.AudienceProtocolMappersTest#testAudienceResolveFullScope

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Unreported flaky test detected

org.keycloak.testsuite.forms.BruteForceTest#testRaceAttackPermanentLockout

org.keycloak.testsuite.actions.RequiredActionUpdateProfileTest#updateProfile

Uh oh!

Uh oh!

Uh oh!

Unreported flaky test detected

org.keycloak.testsuite.javascript.JavascriptAdapterTest#testSilentCheckSso

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants