Upgrade OpenSAML version from 4.3.2 to 5.1.3 #90
Conversation
|
Seems like 5.x drops support for Java 11 which is why that job fails. Some legitimate failures in the Java 17 job tho |
|
@camsaul how do you feel about dropping support for Java 11? https://www.metabase.com/docs/latest/installation-and-operation/running-the-metabase-jar-file#1-install-java-jre seems to suggest Java 21 is your minimum supported version of Metabase. |
danielcompton
force-pushed
the
dc/update-to-5
branch
from
28e5821 to
3af890f
Compare
|
The other test failure appears to be coming from the ID attribute seems to not be valid there? IntelliJ also complains about it being there, which suggests that maybe OpenSAML got stricter in validating the XML. I tried removing the ID attribute, but a new test fails now because the message signature was no longer valid. I'm not quite sure how to fix this, I assume we might need a new test file here? It seems more like a validation issue, than a bug in saml20-clj. Update: this looks like a similar issue, where OpenSAML 5 is stricter in its validation: https://shibboleth.atlassian.net/browse/OSJ-392. |
|
Ah, yep, here's where the issue was introduced: https://shibboleth.atlassian.net/browse/OSJ-293. OpenSAML introduced a strict mode to reject unexpected content. The tests all pass with: I'm not too sure what the right thing to do is here. Probably to document the change and how to opt out of it? And then fix the test file so it is valid and signed. The Shibboleth release notes say:
I reviewed https://shibboleth.atlassian.net/browse/OSJ-392 and am not 100% sure what the actual outcome of the ticket was. We need to test this, but I have to assume that ADFS works by default without disabling strict mode. |
|
@metabase/core-backend-admin-webapp can you help shepherd this PR? |
|
@danielcompton I'm working on getting a new valid test file which I'll post here, so you can incorporate it. Documenting the new strictness and how to opt out is a fine path forward. We should also accompany that by bumping library version, which you can set by updating the VERSION.txt file. A major bump seems appropriate here but not exactly sure the Metabase philosophy on lib version is since I'm relatively new here. |
|
@danielcompton Here's a clean test file
|
danielcompton
force-pushed
the
dc/update-to-5
branch
from
3af890f to
c7ae375
Compare
|
@edpaget thanks for the file. Do I need to do something else with it? I'm getting a test failure with it: |
|
@danielcompton it looks like the indentation changed a bit in the version of the file you checked in which makes the signature invalid. I'll send a PR to your branch |
|
Also there seems to be some issue with cloverage and xml parsing. |
danielcompton
force-pushed
the
dc/update-to-5
branch
from
cf5a10c to
c01e378
Compare
|
@edpaget I've resolved the merge conflict and squashed the commits, I think this is good to go now? CI: https://github.com/danielcompton/saml20-clj/actions/runs/13465086881 |
There was a problem hiding this comment.
Do you mind also updating the VERSION.txt to 3.0.0 since the release will be run after master builds.
deps.edn
Outdated
| org.opensaml/opensaml-saml-impl {:mvn/version "4.3.2"} | ||
| org.opensaml/opensaml-xmlsec-api {:mvn/version "4.3.2"} | ||
| org.opensaml/opensaml-xmlsec-impl {:mvn/version "4.3.2"} | ||
| ;org.apache.santuario/xmlsec {:mvn/version "4.0.2"} ; use latest version and override transient dep from OpenSAML |
There was a problem hiding this comment.
mind deleting these commented out deps.
There was a problem hiding this comment.
I checked, and opensaml 5.1.3 still hasn't bumped them past this version, and even includes a version of xmlsec with a CVE. So I uncommented them to keep them.
https://shibboleth.atlassian.net/wiki/spaces/OSAML/overview Updates signed and encrypted SAML assertion as it wasn't valid under OpenSAML 5. Also typehint getchildnodes for cloverage. cloverage fails with a reflection issue without it. Co-authored-by: Edward Paget <ed.paget@gmail.com>
danielcompton
force-pushed
the
dc/update-to-5
branch
from
c01e378 to
a96169a
Compare
Yep, done! |
https://shibboleth.atlassian.net/wiki/spaces/OSAML/overview
This fixes one deprecated method removal, but there are a few other test failures I'm not so sure about.