Ensure that the EncryptedKey is passed to the DecryptionKeyLocator for SAML by rmartinc · Pull Request #23203 · keycloak/keycloak

rmartinc · 2023-09-13T10:10:45Z

Two little issues in SAML decryption of elements:

The EncyptedKey sometimes is outside the EncryptedData (next sibling). And it was not passed to the SAMLDecryptionKeysLocator. So the locator couldn't filter by the encryption alg set in the response. This threw a NPE iterating the keys. Code also changed to not decode the EncyptedKey twice (if it was already inside the data) and to avoid keys without private key.
Besides the admin console is showing/hiding the encryption algorithm property if the wantAuthnRequestsSigned is ON. That makes no sense, the encryption alg is only used to force the alg in decrypting the received elements, not when we sign the requests. I have moved it to show/hide when wantAssertionsEncrypted is ON. (I think that there is no lint or test errors, but I'm never sure. 😄 )

The combination of the two problems threw the NPE in the related issue. The alg was not forced (because the box was hidden and the user didn't see how to force it) and then the EncryptedKey was outside the EncrypedData in okta. Test added to modify the XML and be similar to the one produced by okta.

cypress · 2023-09-13T10:32:23Z

Passing run #8994 ↗︎

0	527	48	0	0

Details:

Merge `1a26027` into `617044f`...
Project: Keycloak Admin UI	Commit: `9607771f7b ℹ️`
Status: Passed	Duration: 12:44 💡
Started: Sep 20, 2023 8:45 AM	Ended: Sep 20, 2023 8:58 AM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

jonkoops · 2023-09-14T09:20:31Z

We recently made some changes to the way labels are handled, so the namespace can now be omitted.

Suggested change

fieldLabelId="identity-provider:encryptionAlgorithm"

fieldLabelId="encryptionAlgorithm"

@jonkoops I had just moved it! 😄 Done now!

But one question, modifying this I saw that the help message was not OK. I needed to also change the helpText={th("encryptionAlgorithmHelp")} (adding suffix Help) in the HelpItem. I see all the HelpItem (at least in the saml provider) are not showing the proper help message. I just modified the one that I was moving.

Could you log a separate issue for that one? I think this might be because of the recent changes made to the labels. ping @edewit

Created: #23255 (It just happens in main, not in 22.0.3.)

ghost · 2023-09-14T13:27:32Z

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.AppInitiatedActionWebAuthnTest#proceedSetupWebAuthnLogoutOtherSessionsChecked

Keycloak CI - WebAuthn IT (chrome)

org.openqa.selenium.TimeoutException: 
timeout: Timed out receiving message from renderer: 0.644
  (Session info: headless chrome=116.0.5845.187)
Build info: version: '4.1.0', revision: '87802e897b'
System info: host: 'fv-az436-812', ip: '10.1.0.146', os.name: 'Linux', os.arch: 'amd64', os.version: '6.2.0-1011-azure', java.version: '17.0.8.1'
...

Report flaky test

ghost

Unreported flaky test detected, please review

rmartinc · 2023-09-14T14:45:52Z

I give up today, something is failing today. Tomorrow I will rebase and try again.

ghost

Unreported flaky test detected, please review

ghost · 2023-09-14T16:29:36Z

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.DeleteAccountTest#navigationTest

Keycloak CI - Account Console IT (firefox)

org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a org.keycloak.testsuite.util.URLAssert URL expected to begin with: https://localhost:8543/auth/realms/test/protocol/openid-connect/auth ; actual URL: https://localhost:8543/auth/realms/test/account/#/personal-info within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:985)
...

Report flaky test

ghost

Unreported flaky test detected, please review

ghost · 2023-09-15T08:05:41Z

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.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

Keycloak CI - FIPS IT (non-strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	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

rmartinc · 2023-09-15T08:09:50Z

@jonkoops It seems that the Account UI E2E is failing consistently. And I see it happens in other PRs (for example #23271). I have not changed anything in account so I think I'm not guilty here. 😄
The other failure seems to be spureous because something with node not being executable at themes.

ghost

Unreported flaky test detected, please review

ghost · 2023-09-19T14:59:56Z

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.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

Keycloak CI - FIPS IT (non-strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	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

jonkoops · 2023-09-20T07:55:55Z

It seems that the Account UI E2E is failing consistently.

This should be fixed now, could you rebase your PR on main and see if everything passes?

…r SAML Closes keycloak#22974

rmartinc · 2023-09-20T09:57:13Z

OK! All CI passed now. Thanks @jonkoops! @pedroigor ready to merge now if you don't see anything more.

mposolda

@rmartinc @jonkoops @pedroigor Thanks everyone for the work on this and the review

rmartinc requested review from a team as code owners September 13, 2023 10:10

ghost added team/ui labels Sep 13, 2023

ghost added the flaky-test label Sep 13, 2023

ghost mentioned this pull request Sep 13, 2023

Flaky test: org.keycloak.testsuite.forms.RecoveryAuthnCodesAuthenticatorTest#test03AuthenticateRecoveryAuthnCodes #22691

Closed

rmartinc added the team/core label Sep 13, 2023

jonkoops suggested changes Sep 14, 2023

View reviewed changes

rmartinc force-pushed the issue-22974 branch from 3ba623a to 75c76f5 Compare September 14, 2023 11:13

jonkoops previously approved these changes Sep 14, 2023

View reviewed changes

ghost reviewed Sep 14, 2023

View reviewed changes

pedroigor force-pushed the issue-22974 branch from 75c76f5 to 60e0e63 Compare September 14, 2023 13:32

rmartinc mentioned this pull request Sep 14, 2023

Several help text messages missing in saml identity provider #23255

Closed

2 tasks

pedroigor reviewed Sep 14, 2023

View reviewed changes

Comment thread saml-core/src/main/java/org/keycloak/saml/processing/core/util/XMLEncryptionUtil.java Outdated

pedroigor previously approved these changes Sep 14, 2023

View reviewed changes

ghost reviewed Sep 14, 2023

View reviewed changes

rmartinc force-pushed the issue-22974 branch from 60e0e63 to 6d1114c Compare September 15, 2023 06:51

ghost reviewed Sep 15, 2023

View reviewed changes

ghost reviewed Sep 19, 2023

View reviewed changes

Ensure that the EncryptedKey is passed to the DecryptionKeyLocator fo…

1a26027

…r SAML Closes keycloak#22974

rmartinc dismissed stale reviews from pedroigor and jonkoops via 1a26027 September 20, 2023 08:36

rmartinc force-pushed the issue-22974 branch from 6d1114c to 1a26027 Compare September 20, 2023 08:36

jonkoops approved these changes Sep 20, 2023

View reviewed changes

mposolda approved these changes Sep 20, 2023

View reviewed changes

mposolda merged commit f8a9e01 into keycloak:main Sep 20, 2023

rmartinc mentioned this pull request Sep 22, 2023

Ensure that the EncryptedKey is passed to the DecryptionKeyLocator for SAML #23468

Merged

	fieldLabelId="identity-provider:encryptionAlgorithm"
	fieldLabelId="encryptionAlgorithm"

Uh oh!

Conversation

rmartinc commented Sep 13, 2023

Uh oh!

cypress Bot commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Passing run #8994 ↗︎

Uh oh!

jonkoops Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

rmartinc Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonkoops Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

rmartinc Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 14, 2023

Unreported flaky test detected

org.keycloak.testsuite.webauthn.AppInitiatedActionWebAuthnTest#proceedSetupWebAuthnLogoutOtherSessionsChecked

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rmartinc commented Sep 14, 2023

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 14, 2023

Unreported flaky test detected

org.keycloak.testsuite.ui.account2.DeleteAccountTest#navigationTest

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 15, 2023

Unreported flaky test detected

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

Uh oh!

rmartinc commented Sep 15, 2023

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 19, 2023

Unreported flaky test detected

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

Uh oh!

jonkoops commented Sep 20, 2023

Uh oh!

rmartinc commented Sep 20, 2023

Uh oh!

mposolda left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cypress Bot commented Sep 13, 2023 •

edited

Loading

rmartinc Sep 14, 2023 •

edited

Loading