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

Add OpenSaml 4 support #9095

Closed
MichaelVetter opened this issue Oct 5, 2020 · 12 comments
Closed

Add OpenSaml 4 support #9095

MichaelVetter opened this issue Oct 5, 2020 · 12 comments
Assignees
Labels
in: saml2 An issue in SAML2 modules type: enhancement A general enhancement
Milestone

Comments

@MichaelVetter
Copy link

Please upgrade to OpenSaml 4 libraries. Currently the latest version in the Shibboleth repository is 4.0.1.
OpenSaml 3 will reach EOL soon and depends on some library versions with security issues:
https://shibboleth.1660669.n2.nabble.com/Security-issue-on-Java-OpenSaml-Library-td7646686.html
Furthermore the dependencies of OpenSaml have been cleaned up:
https://issues.shibboleth.net/jira/browse/OSJ-264
Maybe you could exclude even more transitive dependencies that are not necessary for spring-security-saml.

@MichaelVetter MichaelVetter added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Oct 5, 2020
@jzheaux jzheaux added in: saml2 An issue in SAML2 modules and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 5, 2020
@jzheaux jzheaux added this to the 6.x milestone Oct 5, 2020
@jzheaux jzheaux changed the title Upgrade to OpenSaml 4 Add OpenSaml 4 support Oct 5, 2020
@jzheaux jzheaux removed this from the 6.x milestone Oct 5, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Oct 5, 2020

Since we'll be supporting Spring Security 5.5 longer than OpenSAML 3 will be supported, it might be better to add support instead of upgrade.

@tadgh
Copy link

tadgh commented Oct 8, 2020

Would be nice to see get this upgraded to OpenSaml 4 in a minor version, if at all possible. CVE scanners are lighting up Santuario/Xmlsec as well as Cryptacular, which are dependencies of OpenSaml.

CVE-2020-7226
CVE-2019-12400

@jzheaux jzheaux added this to the 5.5.x milestone Oct 14, 2020
@mibo
Copy link
Contributor

mibo commented Mar 18, 2021

Would also vote for an update to OpenSaml 4 as the 3.x version are using Apache Velocity 1.7 which is vulnerable by
CVE-2020-13936 (which has a high severity as it allows remote code execution).

Unfortunately it is not that easy to override the used Apache Velocity 1.7 dependency with a newer 2.3 dependency as there may issues based on the behaviour and API changes mentioned in the release notes.

And if someone has good ideas or experience by replacing Velocity 1.7 with the newer 2.3 version in OpenSAML 3.x I'm interested 😉

Edit: Just saw that a PR #9267 is already open 👍

@MichaelVetter
Copy link
Author

@jzheaux : Are you sure that it is enough to use Java 11 just for the build and not at runtime (your comment in #9418)?
On this page it is stated that only Java 11 is supported at runtime for OpenSAML v4:
https://wiki.shibboleth.net/confluence/display/DEV/Product+Platforms
"The Java 11 platform is based on Java 11 as both a development and execution environment. It is used by the following supported products: ... OpenSAML v4.x"

@MichaelVetter
Copy link
Author

The latest OpenSAML version 4.1.0 still uses Apache Velocity 2.2 but overriding this with 2.3 should be ok according to the release notes of Velocity.

jzheaux added a commit to jzheaux/spring-security that referenced this issue Mar 24, 2021
@jzheaux
Copy link
Contributor

jzheaux commented Mar 29, 2021

Hi, @MichaelVetter, sorry that I wasn't clearer on that point.

Spring Security's runtime baseline is JDK 8, and it remains that way. However since OpenSAML 4's runtime baseline is JDK 11, an application that uses OpenSAML 4 will need to run with JDK 11.

Introducing Spring Security into an application will not require that application to use JDK 11, but introducing OpenSAML 4 will. This is because Spring Security optionally chooses its OpenSAML 3 or OpenSAML 4-compatible implementation by inspecting the OpenSAML version at runtime.

For example, Saml2LoginConfigurer contains the following idiom:

if (Version.getVersion().startsWith("4")) {
    return new OpenSaml4XYZComponent(...);
} else {
    return new OpenSaml3XYZComponent(...);
}

Are you sure that it is enough to use Java 11 just for the build and not at runtime

Have you experienced any issues with running JDK 8 + OpenSAML 3 in Spring Security?

@MichaelVetter
Copy link
Author

@jzheaux: Thank you for the clarification.
I did not test the 5.5.x version of spring security yet.

jzheaux added a commit that referenced this issue Mar 30, 2021
- We don't want to have public top-level classes extending or
implementing OpenSAML classes

Issue gh-9095
jzheaux added a commit that referenced this issue Apr 6, 2021
- Produce sources jar
- Produce Javadoc jar

Issue gh-9095
jzheaux added a commit that referenced this issue Apr 6, 2021
- To make upgrade passive

Issue gh-9095
@MichaelVetter
Copy link
Author

@mibo: It seems that velocity is not needed by spring-security-saml2-service-provider 5.4. The velocity templates for HTTP-POST binding are replaced by this method org.springframework.security.saml2.provider.service.servlet.filter.Saml2WebSsoAuthenticationRequestFilter.createSamlPostRequestFormData(Saml2PostAuthenticationRequest)
We could exclude the velocity dependency without problems.

@jzheaux jzheaux self-assigned this Apr 8, 2021
@jzheaux jzheaux modified the milestones: 5.5.x, 5.5.0-RC1 Apr 8, 2021
@mibo
Copy link
Contributor

mibo commented Apr 9, 2021

@MichaelVetter Thanks a lot for this information.

Regarding OpenSaml 4 update it is now that Java >= 11 is required (see d0d0a8d) and the OpenSaml 4 libraries must be provided by ourself (as it is a provided dependency).
Is my understanding correct?

jzheaux added a commit that referenced this issue Apr 9, 2021
jzheaux added a commit that referenced this issue Apr 10, 2021
@jzheaux
Copy link
Contributor

jzheaux commented Apr 16, 2021

Correct, @mibo.

@mukulchakane
Copy link

Would also vote for an update to OpenSaml 4 as the 3.x version are using Apache Velocity 1.7 which is vulnerable by
CVE-2020-13936 (which has a high severity as it allows remote code execution).

Unfortunately it is not that easy to override the used Apache Velocity 1.7 dependency with a newer 2.3 dependency as there may issues based on the behaviour and API changes mentioned in the release notes.

And if someone has good ideas or experience by replacing Velocity 1.7 with the newer 2.3 version in OpenSAML 3.x I'm interested 😉

Edit: Just saw that a PR #9267 is already open 👍

pac4j-saml-opensamlv3 4.5.2 now has velocity-core-engine v2.3 it is based on Java 8

akohli96 pushed a commit to akohli96/spring-security that referenced this issue Aug 25, 2021
- We don't want to have public top-level classes extending or
implementing OpenSAML classes

Issue spring-projectsgh-9095
akohli96 pushed a commit to akohli96/spring-security that referenced this issue Aug 25, 2021
- Produce sources jar
- Produce Javadoc jar

Issue spring-projectsgh-9095
akohli96 pushed a commit to akohli96/spring-security that referenced this issue Aug 25, 2021
- To make upgrade passive

Issue spring-projectsgh-9095
akohli96 pushed a commit to akohli96/spring-security that referenced this issue Aug 25, 2021
akohli96 pushed a commit to akohli96/spring-security that referenced this issue Aug 25, 2021
@rmkellogg
Copy link

Can someone please at least add a note to the fact that Java 11 is required to use the spring-security-saml2-service-provider module? It took me many hours to track this down.

If not in the main documentation, at least add a line in the spring-security-sample/servlet/java-configuration/saml2/login project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: saml2 An issue in SAML2 modules type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

6 participants