KEYCLOAK-19866 Fix user-defined-/anyType parsing and introduce XPathAttributeMapper by KnauerSecunet · Pull Request #10650 · keycloak/keycloak

KnauerSecunet · 2022-03-08T15:12:46Z

@hmlnarik following my email from end of january here is my proposed solution for KEYCLOAK-19866 which also renders KEYCLOAK-19608 unnecessary.

It fixes parsing user-defined types and anyType saml attributes. And lets you later processthem via a xpath attribute mapper.

Please let me know what you think about it and which parts need improving/changes. Concerning the tests for the xpath mapper I plan to add one similar to AbstractUserAttributeMapperTest? Is there a documentation that needs to be expanded (with examples)?

dpulrichth · 2022-04-19T12:49:28Z

I cannot comment on whether this fits in with the guidelines and conventions within the code base but from a purely functional point of view this looks promising. It allows us to work with complex XML types with relative ease and removes the need for a customized keycloak. I got it up and running and can confirm that this works as expected.

A bit of documentation or examples might be helpful. I figured out how to use the included Mapper but had to refer to the source code to realize that the XPath must be relative to the SAML Attribute, not the SAML Assertion.

ahus1

Thank you for creating this pull request. I can see that this offers a very flexible way to extract information from SAML and that it will be useful to a lot of users.

@hmlnarik asked me to review it, please find my comments below.

Please let me know if any of my comments leaves open questions, or if you think any of the suggestions doesn't fit the context of this pull quest.

I'm looking forward to you replies or an updated pull request.

KnauerSecunet · 2022-05-02T12:43:30Z

@ahus1: I agree to and basically implemented all of your suggestions in 5dec051. Comments:

parseAnyTypeAsString was lacking the handling of the top element and namespace, but has different handlings for basic elements compared to parseCustomTypeAsString. But there was an error anyway (xmls have to have a single root element) so I needed to add a root for anyType and decided to use AttributeValue (as it seems more intuitive, see @dpulrichth comment) and both method are now similar enough to be merged together.
I used a warning log when the xpath expression is bad (because that's something that shouldn't suddenly happen in a production environment). And error log for parsing problems (as we could parse the xml before that shouldn't happen).
Good catch on the XXE!

Is there anything else I can or have to do?

ahus1

Thanks for updating this PR, looking at the new functionality this looks now good to me.

As this is your first PR to the Keycloak project, the GitHub actions don't run automatically to provide feedback. They will require a manual approval by a maintainer.

I ran some unit tests on the SAML module using

mvn clean install -am -pl saml-core

and I see that there are two failing tests:

[ERROR] Failures: 
[ERROR]   SAMLParserTest.testSaml20AssertionExample:1007 expected:<[<saml:AttributeValue xsi:type="xs:anyType" xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">060D00000000SHZ
            </saml:AttributeValue>]> but was:<[060D00000000SHZ]>
[ERROR]   SAMLParserTest.testSaml20AssertionsAdviceTag:1137 
Expected: iterable containing [is "060D00000000SHZ"]
     but: item 0: was "<saml:AttributeValue xsi:type=\"xs:anyType\" xmlns:saml=\"urn:oasis:names:tc:SAML:2.0:assertion\">060D00000000SHZ\n            </saml:AttributeValue>"
[INFO] 
[ERROR] Tests run: 85, Failures: 2, Errors: 0, Skipped: 0

Maybe something broke during refactoring, please have a look.

In addition to that, I'd prefer if you could rebase your PR with the lastest changes onto main. This will ensure that your changes work well with the latest changes in the main branch, even when there are no code conflicts.

Please some additional small review comments below.

KnauerSecunet · 2022-05-04T12:02:36Z

Sorry for closing it ... rebased to main, changed the help text (two better examples) and added some trace logging to help getting the right xpath. Now checking the unit tests.

ahus1 · 2022-05-04T12:08:33Z

@KnauerSecunet - good to see it the PR back open and no worries!

Please let me know when you got any questions or code I should look into!

KnauerSecunet · 2022-05-04T13:29:44Z

@ahus1 Yeah the test gave me some more insight what's possible already and what let to exceptions:

<saml:Attribute Name="xyzAnyA">
    <saml:AttributeValue xsi:type="xs:anyType"><A><B>b</B><C>c</C></A></saml:AttributeValue>
</saml:Attribute> <!-- case 1a: returns String "<A><B>b</B><C>c</C></A>" -->
<saml:Attribute Name="xyzAnyB">
    <saml:AttributeValue xsi:type="xs:anyType">Foo</saml:AttributeValue>
</saml:Attribute> <!-- case 1b: returns String "Foo" -->

<saml:Attribute Name="xyzCustom">
    <saml:AttributeValue xmlns:myCustomType="http://www.whatever.de/schema/myCustomType/saml/extensions" xsi:type="myCustomType:AddressType"><myCustomType:A><myCustomType:B>b</myCustomType:B><myCustomType:C>c</myCustomType:C></myCustomType:A></saml:AttributeValue>
</saml:Attribute> <!-- case 2: currently skipped -->

<saml:Attribute Name="xyzAny2">
    <saml:AttributeValue xsi:type="xs:anyType"><B>b</B><C>c</C></saml:AttributeValue>
</saml:Attribute> <!-- case 3: valid xml as far as I know? But leads to a "continuation" exception due to an error in SAMLAttributeValueParser#parseAnyTypeAsString -->

<saml:Attribute Name="xyzCustom2">
    <saml:AttributeValue  xmlns:myCustomType="http://www.whatever.de/schema/myCustomType/saml/extensions" xsi:type="myCustomType:AddressType"><myCustomType:B>b</myCustomType:B><myCustomType:C>c</myCustomType:C></saml:AttributeValue>
</saml:Attribute> <!-- case 4: also valid but same situation as case 3 with StaxParserUtil.bypassElementBlock -->

I guess my change should not change the result for case 1a/b (or anything in saml20-assertion-example.xml)? So I suggest I revert the parser to an earlier version and return:

case 1a/b and saml20-assertion-example.xml: No changes
case 2: <myCustomType:A xmlns:myCustomType="..."><myCustomType:B>b</myCustomType:B><myCustomType:C>c</myCustomType:C></myCustomType:A>
case 3: <anyType><B>b</B><C>c</C></anyType> (hard coded root tag "anyType")
case 4: <myCustomType:AddressType xmlns:myCustomType="..." xsi:type="myCustomType:AddressType"><myCustomType:A><myCustomType:B>b</myCustomType:B><myCustomType:C>c</myCustomType:C></myCustomType:A></myCustomType:AddressType>

Then case 2 would behave similar to an anyType of case 1, but I lose the AddressType type (any better ideas?). case 3 and case 4 also return similar results (and no longer throw exceptions).

ahus1 · 2022-05-04T17:09:30Z

I had a look at the code an tried different variants. Yes, the existing parsing should remain the same. On the other hand I see that there is some inconsistency how values are returned.

Here's some variants, I probably need to discuss with @hmlnarik what's best here, and if we could afford to break the existing behavior:

On top you your latest change 1c0a304df15ec8510992be5abc725540663f02f7, I pushed a commit here that makes all old tests pass, but not one of your new tests: 7591453aa0a5086975a844d9aa035a1bae69a01f
Still, I'm not happy as there are so many variants for an XML snippet to end up as a string. The following would always include the <saml:AttributeValue ..., and would break two old test cases: 3d62b3d1ada82faa6a6e77b0af2315b51e8ec11f

Expected: is "<status><code><status>XYZ</status></code></status>"
     but: was "<saml:AttributeValue xsi:type=\"xs:anyType\" xmlns:saml=\"urn:oasis:names:tc:SAML:2.0:assertion\"><status><code><status>XYZ</status></code></status></saml:AttributeValue>"

Expected: iterable containing [is "<n1:elem2 xmlns:n1=\"http://example.net\" xml:lang=\"en\"><n3:stuff xmlns:n3=\"ftp://example.org\">00DD0000000F7L5</n3:stuff></n1:elem2>"]
     but: item 0: was "<saml:AttributeValue xsi:type=\"xs:anyType\" xmlns:saml=\"urn:oasis:names:tc:SAML:2.0:assertion\"><n1:elem2 xmlns:n1=\"http://example.net\" xml:lang=\"en\"><n3:stuff xmlns:n3=\"ftp://example.org\">00DD0000000F7L5</n3:stuff></n1:elem2></saml:AttributeValue>"

ahus1 · 2022-05-04T17:27:12Z

Thinking a bit more, a consistent way with the existing logic would be to return an XML fragment, and to never return the saml:AttributeValue as a wrapper. Only XPathAttributeMapper would then add a dummy <root>...</root> around it to make the fragment a valid XML (there might be a better way to parse fragments, I'm not familiar with fragment parsing)

ahus1 · 2022-05-05T06:54:34Z

Reading yesterday's comments, they sound rather draft and exploring. After a night's sleep, I suggest the following:

SAMLAttributeValueParser: Responsibility to return everything inside an attributeValue - mostly as a String, with the exception of a XMLGregorianCalendar for dates
- Old behavior: ignored some unknown types, and for multiple XML children returns only the first (Note: There are no existing tests for multiple children in the old code)
- New behavior: ignoring fewer unknown types, returning all children within an attribute
XPathAttributeMapper: Treat attributeValue contents as an XML fragment and apply an XPath
- A simple (maybe too simple approach): Wrap the contents extracted by SAMLAttributeValueParser in a new root tag, then apply the XPath

With regards to namespace handling in SAMLAttributeValueParser: While my namespace handling is rusty, I suppose any namespace on the attributeValue that would be relevant for its children might as well be declared on the children, so they can be preserved even if the attributeValue tag is not part of the fragment.

@KnauerSecunet, @hmlnarik - what do you think about this suggestion? I know this different from the original suggestion, still I see that it matches the behavior in the existing tests. This drops the information in xsi:type, I don't know what downsides that might have, still I assume most XPath evaluations would still work.

KnauerSecunet · 2022-05-05T07:10:52Z

Thinking a bit more, a consistent way with the existing logic would be to return an XML fragment, and to never return the saml:AttributeValue as a wrapper. Only XPathAttributeMapper would then add a dummy <root>...</root> around it to make the fragment a valid XML (there might be a better way to parse fragments, I'm not familiar with fragment parsing)

Reading yesterday's comments, they sound rather draft and exploring. After a night's sleep, I suggest the following: ...

As I understand it your draft and the proposed solution are the same: Both return the xml string (fragmet) including namespace and the mapper handles the missing root tag? It won't be that easy for the mapper to recognize a missing root element but basically I like that idea very much and think it would be feasible.

The missing xml-type also came to my mind, but I actually think it should be possible to rely on the attribute name (which can be configured/filtered in the xpath mapper).

@ahus1 Works for me, just waiting for your go when you get the projects feedback ...

ahus1 · 2022-05-05T07:14:58Z

@KnauerSecunet - thanks for the feedback. Regarding the root tag: I'd suggest that the mapper will always wrap the string it receives within the root tag. That will make it simpler to implement, and it would not depend on the number of siblings in the string. Any user who would use an XPath with "//" wouldn't notice it, anyway IMHO.

I'll update this issue once I get feedback from the project.

hmlnarik · 2022-05-06T08:15:00Z

A simple (maybe too simple approach): Wrap the contents extracted by SAMLAttributeValueParser in a new root tag, then apply the XPath

I agree with this approach, and agree also with the proposal to propagate namespaces to the first-level tags of the value as suggested here:

While my namespace handling is rusty, I suppose any namespace on the attributeValue that would be relevant for its children might as well be declared on the children, so they can be preserved even if the attributeValue tag is not part of the fragment.

ahus1

@KnauerSecunet - after talking to @hmlnarik today, the suggested approach is now generally approved (thank you, @hmlnarik!).

While the new code uses the safe XXE methods, he also asked for a test to show that it is safe. This shouldn't affect the implementation, and I'll have more information on that, soon.

If you have additional questions, I'm happy to get the answers for them.

KnauerSecunet · 2022-05-06T16:07:27Z

Changed the code as discussed (parsing without root-tag and type; adding a root tag in mapping). Changed the help texts and tests accordingly. Added some new tests in SAMLParserTest#testInvalidEndElement.

While the new code uses the safe XXE methods, he also asked for a test to show that it is safe. This shouldn't affect the implementation, and I'll have more information on that, soon.

I was not able to add a XXE test for the mapper. XXE via DOCTYPE should no longer be possible as we add a root tag on top level. If I change the input to new InputSource(new StringReader(xml)), use the payload <foo xmlns:xi="http://www.w3.org/2001/XInclude"><xi:include parse="text" href="file:///etc/passwd"/></foo> and the xpath / I just get null and nothing from passwd. I am not sure whether InputSource is subsceptible to xinclude injection.

@ahus1 Any ideas for the XXE test (or comments for the other changes)?

hmlnarik

Thanks for the PR! The overall direction looks good. I left a few comments inline, could you please have a look?

KnauerSecunet · 2022-05-18T11:36:39Z

Thanks for the PR! The overall direction looks good. I left a few comments inline, could you please have a look?

All comments make sense. @hmlnarik Could please answer ahus1 open question whether attributes can be used / whether that was your intention?

Once that is done I can implement all changes / answer the namespace questions ... I think sometime next week

KnauerSecunet · 2022-06-03T11:34:27Z

I will continue working on the issue around June 20th ...

KnauerSecunet · 2022-07-01T10:48:04Z

My changes are available here: KnauerSecunet#9 ... please review them, merge them, and squash them with your changes. Then this PR is good to be merged from my perspective.

Your changes and @hmlnarik comments are now included in my pull request. I either resolved the comment above or added some reasoning for my change.

I made a separate branch in my clone keycloak_pull_10650 which contains a commit 71244a61 for my recent changes only.

hmlnarik · 2022-07-25T08:16:03Z

@KnauerSecunet Thank you for updates. Could you please rebase the PR?

KnauerSecunet · 2022-07-25T10:55:57Z

@KnauerSecunet Thank you for updates. Could you please rebase the PR?

@hmlnarik Rebased on current main

hmlnarik

@KnauerSecunet Thank you for the work, this is very nice piece of code! Once you address the one last comment regarding XPath, this PR is ready for merging.

…tributeMapper

KnauerSecunet · 2022-08-03T08:26:45Z

@hmlnarik Done and rebased on current main

hmlnarik

Approved on the condition that the tests pass

svenhartmann · 2022-10-29T14:01:52Z

@ahus1 @hmlnarik Is there a expected date when the XPathAttributeMapper will be available in a release?

ahus1 · 2022-11-01T09:06:32Z

@svenhartmann - it will be part of Keycloak 20. You can watch the progress on KC20 by looking at the milestone https://github.com/keycloak/keycloak/milestone/22 ... the last open issues are actively being worked on, at the same time it will be released when it is ready.

To give this feature a try, please use the Keycloak nightly builds. Given the small number of open issue for the milestone it should be quite close.

KnauerSecunet mentioned this pull request Apr 25, 2022

Crash when parsing user-defined or anyType SAML attributes #11644

Closed

hmlnarik requested a review from ahus1 April 25, 2022 09:47

hmlnarik self-assigned this Apr 25, 2022

hmlnarik added the area/saml SAML 2.0 protocol — IdP and SP roles, artifact binding, ECP, metadata label Apr 25, 2022

ahus1 self-assigned this Apr 25, 2022

ahus1 requested changes Apr 25, 2022

View reviewed changes

KnauerSecunet force-pushed the main branch from ae31c8e to 5dec051 Compare May 2, 2022 12:38

ahus1 self-requested a review May 2, 2022 15:25

ahus1 requested changes May 2, 2022

View reviewed changes

ahus1 added the status/triage label May 2, 2022

KnauerSecunet closed this May 4, 2022

KnauerSecunet force-pushed the main branch from 5dec051 to 4856583 Compare May 4, 2022 11:14

KnauerSecunet reopened this May 4, 2022

ahus1 removed the status/triage label May 5, 2022

ahus1 self-requested a review May 6, 2022 08:30

ahus1 requested changes May 6, 2022

View reviewed changes

Comment thread .../java/org/keycloak/saml/processing/core/parsers/saml/assertion/SAMLAttributeValueParser.java Resolved

KnauerSecunet force-pushed the main branch from 1c0a304 to 4d34b86 Compare May 6, 2022 15:54

d-michail mentioned this pull request May 7, 2022

Missing parts for a 1.0 version grnet/eidas-keycloak-extension#1

Open