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

Update pac4j to 5.2.0 #257

Merged
merged 8 commits into from
Sep 2, 2022
Merged

Update pac4j to 5.2.0 #257

merged 8 commits into from
Sep 2, 2022

Conversation

Riliane
Copy link

@Riliane Riliane commented Aug 26, 2022

See JENKINS-69462.

This needs jenkins-infra/helpdesk#3108 to successfully be built - now it will fail because of the shibboleth artifacts.
Now that we are dropping Java 8 we can update pac4j further.

Submitter checklist

  • JIRA issue is well described
  • Appropriate autotests or explanation to why this change has no tests

@kuisathaverat
Copy link

kuisathaverat commented Aug 26, 2022

I am preparing the plugin for update pac4j, first I have to merge #261

pom.xml Outdated Show resolved Hide resolved
Riliane and others added 3 commits August 29, 2022 10:17
Co-authored-by: Ivan Fernandez Calvo <[email protected]>
this is a major change so we have to update the major version to the next
@kuisathaverat
Copy link

kuisathaverat commented Aug 29, 2022

I have resolved the conflicts and updated the major version

@Riliane Riliane marked this pull request as ready for review August 29, 2022 14:18
pom.xml Outdated Show resolved Hide resolved
@kuisathaverat
Copy link

I will try to make some manual testing, If I do not see anything weird, I would merge and release a new version.

Co-authored-by: Vincent Latombe <[email protected]>
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@kuisathaverat
Copy link

kuisathaverat commented Aug 31, 2022

Summary of my tests, there is an error in the redirections 302 code is not correctly processed. The version of pac4j used is not the latest, is from about a year ago, it is possible to update to the latest with minor changes.

I have added comments with the changes needed to fix the 302 redirections and bump the pac4j version to 5.4.6

I have used this environment to test the incremental https://github.com/kuisathaverat/jenkins-issues/tree/main/test-pac4j-5, it is possible to test new binaries if you copy the saml.hpi file as test-pac4j-5/jenkins/jenkins_home/plugins/saml.jpi

Riliane and others added 2 commits September 1, 2022 10:37
Co-authored-by: Ivan Fernandez Calvo <[email protected]>
Co-authored-by: Ivan Fernandez Calvo <[email protected]>
@Riliane
Copy link
Author

Riliane commented Sep 1, 2022

committed suggestions to pull them but still got things to fix (JEESessionStore)

Copy link

@kuisathaverat kuisathaverat left a comment

Choose a reason for hiding this comment

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

tested Manually

@kuisathaverat kuisathaverat merged commit fb182c9 into jenkinsci:main Sep 2, 2022
@jkbszpg
Copy link

jkbszpg commented Sep 7, 2022

the new pac4j-saml 5.4.6, requires an Optional "Destination" field in the SAML response to be present unless disabled with a setting

cfg.setResponseDestinationAttributeMandatory(false);

pac4j/pac4j#1871

resulting in

WARNING o.j.p.saml.SamlSecurityRealm#doFinishLogin: Unable to validate the SAML Response: SAML configuration does not allow response Destination to be null
For more info check 'Maximum Authentication Lifetime' at https://github.com/jenkinsci/saml-plugin/blob/master/doc/CONFIGURE.md#configuring-plugin-settings
If you have issues check the troubleshoting guide at https://github.com/jenkinsci/saml-plugin/blob/master/doc/TROUBLESHOOTING.md
org.pac4j.saml.exceptions.SAMLEndpointMismatchException: SAML configuration does not allow response Destination to be null
        at org.pac4j.saml.profile.impl.AbstractSAML2ResponseValidator.verifyEndpoint(AbstractSAML2ResponseValidator.java:204)
        at org.pac4j.saml.sso.impl.SAML2AuthnResponseValidator.validateSamlProtocolResponse(SAML2AuthnResponseValidator.java:247)
        at org.pac4j.saml.sso.impl.SAML2AuthnResponseValidator.validate(SAML2AuthnResponseValidator.java:91)
        at org.pac4j.saml.profile.impl.AbstractSAML2MessageReceiver.receiveMessage(AbstractSAML2MessageReceiver.java:53)
        at org.pac4j.saml.sso.impl.SAML2WebSSOProfileHandler.receive(SAML2WebSSOProfileHandler.java:35)
        at org.pac4j.saml.credentials.extractor.SAML2CredentialsExtractor.receiveLogin(SAML2CredentialsExtractor.java:71)
        at org.pac4j.saml.credentials.extractor.SAML2CredentialsExtractor.extract(SAML2CredentialsExtractor.java:66)
        at org.pac4j.core.client.BaseClient.retrieveCredentials(BaseClient.java:71)
        at org.pac4j.core.client.IndirectClient.getCredentials(IndirectClient.java:145)
        at org.jenkinsci.plugins.saml.SamlProfileWrapper.process(SamlProfileWrapper.java:56)
Caused: org.springframework.security.authentication.BadCredentialsException: SAML configuration does not allow response Destination to be null
        at org.jenkinsci.plugins.saml.SamlProfileWrapper.process(SamlProfileWrapper.java:61)
        at org.jenkinsci.plugins.saml.SamlProfileWrapper.process(SamlProfileWrapper.java:35)

is there a way for saml-plugin to pass this config through?

@Palmer-Reed-bah
Copy link

@kuisathaverat, we are also seeing the above issue when upgrading to the latest SAML plugin version.

@kuisathaverat
Copy link

kuisathaverat commented Sep 27, 2022

@jkbszpg @rdpa-bah I am not subscribed to PR (so comments here would not arrive in my inbox). I will take a look. It is simple to add that option. To file an issue, there is a Jira instance https://issues.jenkins.io/browse/, and for questions and so on, there are several groups. Please check https://www.jenkins.io/participate/report-issue/

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

Successfully merging this pull request may close these issues.

5 participants