Update apache.santuario.xmlsec dep from 2.1.4 to 2.2.6#112022
Merged
albertzaharovits merged 7 commits intoelastic:7.17from Aug 23, 2024
Merged
Update apache.santuario.xmlsec dep from 2.1.4 to 2.2.6#112022albertzaharovits merged 7 commits intoelastic:7.17from
albertzaharovits merged 7 commits intoelastic:7.17from
Conversation
Comment on lines
+172
to
+186
| final String signatureAlg = AlgorithmSupport.getKeyAlgorithm(signature.getSignatureAlgorithm()); | ||
| final String keyAlg = credential.getPublicKey().getAlgorithm(); | ||
| if (signatureAlg != null && signatureAlg.equals(keyAlg) == false) { | ||
| if (logger.isDebugEnabled()) { | ||
| String keyFingerprint = "SHA265:" | ||
| + MessageDigests.toHexString(MessageDigests.sha256().digest(credential.getPublicKey().getEncoded())); | ||
| logger.debug( | ||
| "Skipping [{}] key [{}] because it is not compatible with signature algorithm [{}]", | ||
| keyAlg, | ||
| keyFingerprint, | ||
| signatureAlg | ||
| ); | ||
| } | ||
| return false; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
This bit is from #98119 . Without it there are test failures such as https://gradle-enterprise.elastic.co/s/6hj4qm6ntsbpc/tests/task/:x-pack:plugin:security:test/details/org.elasticsearch.xpack.security.authc.saml.SamlAuthenticatorTests/testSigningWhenIdpHasMultipleKeys?top-execution=1 .
This shows that the internals of the lib changed in between the version updates, so there is a certain risk in the update.
Collaborator
|
Pinging @elastic/es-security (Team:Security) |
Contributor
Author
|
@elasticsearchmachine test this please |
slobodanadamovic
approved these changes
Aug 23, 2024
Contributor
slobodanadamovic
left a comment
There was a problem hiding this comment.
LGTM
Thanks for updating this!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
apache.santuario.xmlsecversion2.1.4is documented vulnerable. We should update to mitigate the vulnerabilities.But
apache.santuario.xmlsecis a dependency ofopensamlversion3.*.However, in a patch release of elasticsearch (i.e.
7.17.*) it's best we avoid updating dependencies across major versions (i.e.opensamlfrom version3.*to version4.*), particularly for such a complex dependency asopensaml(we did update theopensamldep in this way, but in a minor elasticsearch8.*release, i.e. #98199). The latestopensaml3.*release (i.e.3.4.6) still requires a vulnerableapache.santuario.xmlsecdep: https://mvnrepository.com/artifact/org.opensaml/opensaml-xmlsec-impl/3.4.6).In this case, our best hope is to find a non-vulnerable version of
apache.santuario.xmlsecthat is still on the same major version as the version listed in the deps ofopensaml(i.e.2.*). That's version2.2.6: https://mvnrepository.com/artifact/org.apache.santuario/xmlsec/2.2.6 , which is not vulnerableThis PR updates
apache.santuario.xmlsecfrom the existing2.1.4version to the2.2.6version. The release notes of the2.2.0version from https://santuario.apache.org/javareleasenotes.html look simple, and the dependencies differences (from https://mvnrepository.com/artifact/org.apache.santuario/xmlsec/2.1.4) are minimal as well (hopefully optional dependencies, which we don't pull in, stay optional in the same way in the new version).So, it looks to me that the dep update is relatively safe (and it also passes CI)!
2 last points that I've considered:
apache.santuario.xmlsecto the2.1.8https://mvnrepository.com/artifact/org.apache.santuario/xmlsec/2.1.8 version instead. This has the advantage that there's a smaller chance of problems, since in this case the update only covers the patch version (2.1.4->2.1.8). However2.1.8is still vulnerable.opensamlfrom3.4.5to3.4.6. The benefit of this is that3.4.6is a later release, closer by calendar date to the apache.santuario.xmlsec2.2.6or2.1.8releases, which also decreases incompatibilities risks (just at a hope level).