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.

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

[RHPAM-3888] SSO integration fails for multiple Realm certificates #275

Merged
merged 1 commit into from Nov 26, 2021
Merged

[RHPAM-3888] SSO integration fails for multiple Realm certificates #275

merged 1 commit into from Nov 26, 2021

Conversation

desmax74
Copy link

@desmax74 desmax74 commented Nov 4, 2021

if [ -n "$realms_certificates" ]; then
if [ -n "$token" ]; then
# SSO Server 7.0
realms_certificates=`$CURL -H "Accept: application/json" -H "Authorization: Bearer ${token}" ${sso_service}/admin/realms/${SSO_REALM} | $(jq '.keys[].certificate') `
Copy link
Contributor

Choose a reason for hiding this comment

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

@desmax74 , jq command line is not installed in the EAP image. I suspect that it is installed in RHPAM image right?

@desmax74
Copy link
Author

@jfdenise reverted the changes and applied only the jq command to retrieve the certificate with use SIG

@desmax74 desmax74 marked this pull request as ready for review November 23, 2021 14:00
@spolti
Copy link

spolti commented Nov 23, 2021

@jfdenise @iankko
Can you please review?

#SSO Server 7.1
realm_certificate=`$CURL -H "Accept: application/json" -H "Authorization: Bearer ${token}" ${sso_service}/admin/realms/${SSO_REALM}/keys | grep -Po '(?<="certificate":")[^"]*'`
#SSO Server 7.1 and newer. If is 7.5 we skip certificate with use=ENC
realm_certificate=`$CURL -H "Accept: application/json" -H "Authorization: Bearer ${token}" ${sso_service}/admin/realms/${SSO_REALM}/keys | $(jq '.keys[] | select( .certificate != null and .use == "SIG")') | $(jq '.certificate') `
Copy link
Contributor

Choose a reason for hiding this comment

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

This change introduce a new dependency on jq command line.
Is this fix: jboss-container-images/redhat-sso-7-openshift-image@214fca4
possibly a better candidate (no use of jq)? It covers 7.0, 7.1 to 7.4 and 7.5.

Copy link
Author

Choose a reason for hiding this comment

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

@jfdenise fix applied on the script

Signed-off-by: desmax74 <mdessi@redhat.com>
Copy link
Contributor

@jfdenise jfdenise left a comment

Choose a reason for hiding this comment

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

@desmax74 Thank-you. The fix looks good to me. We will handle porting this change to other branches.

@jfdenise jfdenise requested a review from luck3y November 23, 2021 16:41
@spolti
Copy link

spolti commented Nov 23, 2021

Hi @jfdenise, @luck3y thanks for approving it.
Do you think we can have this merged this week or next and a new tag from 0.18.x created?

@luck3y
Copy link
Contributor

luck3y commented Nov 23, 2021

@spolti That sounds fine to me -- @jfdenise do you want me to open a version of this against main as well?

@jfdenise
Copy link
Contributor

@spolti we are waiting confirmation from @desmax74 that the new implementation fix (patch shared with keycloak team) actually works for his use-case, then we can merge. @luck3y thank-you for proposing, go ahead for master (23.x too perhaps?). I will handle it for v2 (some PR are currently opened and I will need to integrate the fix too there).

@spolti
Copy link

spolti commented Nov 24, 2021

Thanks @jfdenise, about the cherry-pick, if we can also have it on 23.x, we are planning to move to this branch in a near future
@desmax74 please let us know about the tests.

@desmax74
Copy link
Author

@spolti @jfdenise @luck3y Green light on our QE tests with SSO 7.4 and SSO 7.5

@jfdenise jfdenise merged commit 39ec2bb into wildfly:0.18.x Nov 26, 2021
@jfdenise
Copy link
Contributor

@desmax74 Thank-you! Merged in 0.18.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants