Update bcpkix and bcprov dependencies #21543

vmuzikar · 2023-07-10T10:03:15Z

Closes #21360 (important to see for more context and discussion)

Supersedes #21369

ghost

Unreported flaky test detected, please review

ghost · 2023-07-10T11:37:29Z

Unreported flaky test detected

If the below 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.ui.account2.ApplicationsTest#toggleApplicationDetailsTest

Keycloak CI - Account Console IT (firefox)

java.lang.AssertionError: Expected ApplicationsPage but was Keycloak Account Management (https://localhost:8543/auth/realms/test/account/#/applications&state=fbb8aab0-1b12-4a4e-a93b-93596915c9db&session_state=35840859-0395-403b-9569-7de773e60438&code=319a478d-9df7-4815-8518-2a05120c768f.35840859-0395-403b-9569-7de773e60438.d136b9ad-9a34-470d-8d70-bc7106fc0deb)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.page.AbstractPage.assertCurrent(AbstractPage.java:110)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...

Report flaky test

Closes keycloak#21360

vmuzikar · 2023-07-17T11:30:50Z

testsuite/integration-arquillian/tests/pom.xml

        <app.server.2.jboss.jvm.debug.args>-agentlib:jdwp=transport=dt_socket,server=y,suspend=${app.server.2.debug.suspend},address=localhost:${app.server.2.debug.port}</app.server.2.jboss.jvm.debug.args>
        <app.server.memory.Xms>64m</app.server.memory.Xms>
-        <app.server.memory.Xmx>512m</app.server.memory.Xmx>
+        <app.server.memory.Xmx>768m</app.server.memory.Xmx>


Not sure if this change is safe. Seems like newer versions of BC are more memory demanding (see a failing test without this change). Is that something we want to risk as it might be directly affecting users.

That's good point. Hopefully it affects just the app-server (which may not be that important AFAIK due the adapter tests executed on embedded undertow by default?).

Isn't it possible that app-server has both jdk15on and jdk18on dependencies and that being the cause of those issues?

We run with app-server-wildfly by default.

I did a little bit more digging into this. I realized we're actually not including any BC dependencies with the WF/EAP adapter. We use the default BC module that's already present in WF. You can simply double check that by building the adapters dist and checking the JARs included. There are actually no 3rd party deps bundled in our adapters dist. That means WF/EAP adapter should not be affected by this change at all.

The reason for the out of memory test failures is that we're including the BC deps in our test app WARs. The newer versions of BC libs are a few MBs larger. Hence I believe we're reaching the memory limit because of this.

However, where we are also including the BC deps is the Tomcat and Jetty adapters. Those are affected by this upgrade. This is NOT tested in GHA. @mposolda @miquelsi Do we need to run the full internal pipeline to test this?

Running internal adapter pipeline can be nice, however our java adapters are deprecated. So not 100% sure it is strictly needed if it is too much work... IMO any performance/memory "regression" specific only to java adapters is fine due their deprecation (as long as it does not affect the Keycloak server).

@mposolda Are you then fine with merging this? :) It'd be nice to have it in 22.0.1 to fix #21664

Yes, ok. I am approving

mposolda

In general, it looks as a good change, but might worth to investigate the memory consumption issue as pointed in the inline comments.

mposolda · 2023-07-17T12:16:18Z

testsuite/integration-arquillian/tests/pom.xml

        <app.server.2.jboss.jvm.debug.args>-agentlib:jdwp=transport=dt_socket,server=y,suspend=${app.server.2.debug.suspend},address=localhost:${app.server.2.debug.port}</app.server.2.jboss.jvm.debug.args>
        <app.server.memory.Xms>64m</app.server.memory.Xms>
-        <app.server.memory.Xmx>512m</app.server.memory.Xmx>
+        <app.server.memory.Xmx>768m</app.server.memory.Xmx>


That's good point. Hopefully it affects just the app-server (which may not be that important AFAIK due the adapter tests executed on embedded undertow by default?).

Isn't it possible that app-server has both jdk15on and jdk18on dependencies and that being the cause of those issues?

ghost

Unreported flaky test detected, please review

ghost · 2023-07-17T13:55:24Z

Unreported flaky test detected

If the below 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.webauthn.account.WebAuthnTransportLocaleTest#localizationTransportNFC

Keycloak CI - WebAuthn IT (chrome)

java.lang.AssertionError: Expected SigningInPage but was Sign in to test (https://localhost:8543/auth/realms/test/login-actions/authenticate?client_id=account-console&tab_id=TtolwhKIJj0)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.page.AbstractPage.assertCurrent(AbstractPage.java:110)
	at jdk.internal.reflect.GeneratedMethodAccessor612.invoke(Unknown Source)
...

Report flaky test

ASzc

It seems like this will do what you want it to do. There are a limited number of productized versions available for bcprov-jdk18on, so weigh that potential impact before merging.

vmuzikar · 2023-07-17T16:20:44Z

There are a limited number of productized versions available for bcprov-jdk18on, so weigh that potential impact before merging.

@ASzc Thanks for the review. Is 1.74 fine from the prod perspective? It's what Quarkus uses at the moment, so I assume should be fine.

ASzc · 2023-07-17T19:49:19Z

@vmuzikar The closest version I see is 1.73. I don't see any bouncycastle artifacts on my list from RHBQ that they claim to support. I can inquire if needed. Are we shipping this jar, or is it being used in a "provided" context, given these are adapters that seem to be using it?

vmuzikar · 2023-07-18T06:29:50Z

@ASzc We're shipping the JARs.

ASzc · 2023-07-18T14:05:11Z

@vmuzikar I have sent an inquiry about these GAVs

Closes keycloak#21360 (cherry picked from commit 776bcbc)

ASzc · 2023-07-21T12:54:36Z

@vmuzikar RHBQ has decided to productize these BC GAVs, so we're good

vmuzikar · 2023-07-21T13:22:45Z

@ASzc Thanks!

Closes #21360 (cherry picked from commit 776bcbc)

vmuzikar mentioned this pull request Jul 10, 2023

21360 updated bouncycastle to jdk15to18 1.75 #21369

Closed

vmuzikar marked this pull request as ready for review July 10, 2023 10:08

vmuzikar requested review from a team as code owners July 10, 2023 10:08

vmuzikar requested a review from a team July 10, 2023 10:08

ghost added team/core team/cloud-native labels Jul 10, 2023

vmuzikar marked this pull request as draft July 10, 2023 10:36

ghost added the flaky-test label Jul 10, 2023

ghost mentioned this pull request Jul 10, 2023

Flaky test: org.keycloak.testsuite.ui.account2.UpdateEmailTest#aiaCancellationSucceeds #21016

Closed

ghost reviewed Jul 10, 2023

View reviewed changes

vmuzikar force-pushed the bc-alignment branch from ecf7a01 to ad51ada Compare July 10, 2023 12:15

vmuzikar mentioned this pull request Jul 10, 2023

Update bcpkix and bcprov dependencies #21360

Closed

ghost mentioned this pull request Jul 10, 2023

Flaky test: org.keycloak.testsuite.admin.authentication.FlowTest#testAddRemoveFlow #20763

Closed

vmuzikar force-pushed the bc-alignment branch from ad51ada to ca7a9d9 Compare July 17, 2023 10:36

Update bcpkix and bcprov dependencies

2fd36ca

Closes keycloak#21360

vmuzikar force-pushed the bc-alignment branch from ca7a9d9 to 2fd36ca Compare July 17, 2023 11:23

vmuzikar commented Jul 17, 2023

View reviewed changes

vmuzikar requested review from ASzc, mposolda and sschu July 17, 2023 11:34

vmuzikar marked this pull request as ready for review July 17, 2023 11:34

mposolda reviewed Jul 17, 2023

View reviewed changes

ghost reviewed Jul 17, 2023

View reviewed changes

vmuzikar requested a review from miquelsi July 17, 2023 16:06

ASzc approved these changes Jul 17, 2023

View reviewed changes

vmuzikar linked an issue Jul 18, 2023 that may be closed by this pull request

CVE-2023-33201 - Information Exposure vulnerability in org.bouncycastle:bcprov-jdk15on #21664

Closed

mposolda approved these changes Jul 18, 2023

View reviewed changes

miquelsi approved these changes Jul 18, 2023

View reviewed changes

sschu approved these changes Jul 19, 2023

View reviewed changes

vmuzikar merged commit 776bcbc into keycloak:main Jul 20, 2023

vmuzikar deleted the bc-alignment branch July 20, 2023 09:57

vmuzikar added a commit to vmuzikar/keycloak that referenced this pull request Jul 20, 2023

Update bcpkix and bcprov dependencies (keycloak#21543)

f20052d

Closes keycloak#21360 (cherry picked from commit 776bcbc)

stianst pushed a commit that referenced this pull request Jul 26, 2023

Update bcpkix and bcprov dependencies (#21543) (#21845)

13893cb

Closes #21360 (cherry picked from commit 776bcbc)

This was referenced Sep 4, 2023

sync github #22926

Closed

testsuite cnditional #23002

Closed

release/22.0 #23125

Closed

bhatpmk mentioned this pull request Sep 22, 2023

Cherry-pick bcpkix and bcprov dependency updates, to use v1.74 verrazzano/keycloak#24

Merged

stianst mentioned this pull request Nov 14, 2023

eap docs #24733

Closed

stianst mentioned this pull request May 6, 2024

Ignore github links when checking links #29291

Merged

Update bcpkix and bcprov dependencies #21543

Update bcpkix and bcprov dependencies #21543

Uh oh!

Conversation

vmuzikar commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 10, 2023

Unreported flaky test detected

org.keycloak.testsuite.ui.account2.ApplicationsTest#toggleApplicationDetailsTest

Uh oh!

vmuzikar Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

mposolda Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

vmuzikar Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

mposolda Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

vmuzikar Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

mposolda Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

mposolda left a comment

Choose a reason for hiding this comment

Uh oh!

mposolda Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 17, 2023

Unreported flaky test detected

org.keycloak.testsuite.webauthn.account.WebAuthnTransportLocaleTest#localizationTransportNFC

Uh oh!

ASzc left a comment

Choose a reason for hiding this comment

Uh oh!

vmuzikar commented Jul 17, 2023

Uh oh!

ASzc commented Jul 17, 2023

Uh oh!

vmuzikar commented Jul 18, 2023

Uh oh!

ASzc commented Jul 18, 2023

Uh oh!

ASzc commented Jul 21, 2023

Uh oh!

vmuzikar commented Jul 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vmuzikar commented Jul 10, 2023 •

edited

Loading