Skip to content

Conversation

@sprytnyserek
Copy link

Formatted XML is good to read by humans but not so good for systems. This request is to address an issue at relying party being .NET Core / .NET 5 web app when SimpleSAMLphp STS is hosted under Windows.

MS-based solutions are main beneficients of WS-Fed. However they get crazy if an XML payload from STS contains newline characters, especially CR. They see CR as an actual XML entity what effectively prevents the whole payload from further processing - throws exception. Compacting the XML at STS can help to solve that without any risk.

Here in company we have another STS that is based on DirX Access. It produces compact XML by default so we have never had issues with that. We also use this modified version of SimpleSAMLphp STS on one of our productions so it well-tested over there.

@tvdijen
Copy link
Member

tvdijen commented Dec 9, 2020

Hi @sprytnyserek !

I see your problem, but I don't like the preg_match/trim solution..
I'd like to explore another option first..

I think something along the lines of this would be better;

$doc = new DOMDocument();
$doc->preserveWhiteSpace = false;
$doc->formatOutput = false;

$doc->loadXML($formattedXml);
$unformattedXml = $doc->saveXML();

It has to be done in a different place, before adding the signature.

@tvdijen
Copy link
Member

tvdijen commented Dec 9, 2020

@sprytnyserek Would you mind trying if this has the same result for you? Does it solve your problem?

@sprytnyserek
Copy link
Author

Hi Tim,

Thanks for reminding this to me :) My initial solution was more or less similar to that. However that wasn't enough (and good enough - selected wrong place for changes) and I struggled around what resulted in messy approach.

Does it solve the problem? Well, yes and no :)

In this particular part - yes, it's solved and this change is necessary. So, if you want, please put it to the upstream. We can cancel this PR then.

But still our dependency at XMLSecurityDSig.php makes its own indentation and it isn't configurable. What I did locally to this module was to do the same on its contructor right before loadXML():

$sigdoc->preserveWhiteSpace = false;
$sigdoc->formatOutput = false;

As that's general-purpose XMLSec, I'll try to incorporate these changes to the project as options. Then, depending on what defaults we select, we might need to reflect those options here in ADFS.php.

@tvdijen
Copy link
Member

tvdijen commented Dec 10, 2020

Thanks @sprytnyserek ! We're trying to break away from xmlseclibs and started some work on it here.
Right now it's more-or-less a wrapper around xmlseclibs, but eventually it should be completely independent. Let's leave this open for future reference until we have a proper fix

@tvdijen
Copy link
Member

tvdijen commented Dec 10, 2020

btw, I think it should be no problem to remove formatting from the signature on our end.. I'll try that in the other branch and then you can test it again?

@tvdijen
Copy link
Member

tvdijen commented Dec 10, 2020

@sprytnyserek Would you mind testing with https://github.com/simplesamlphp/simplesamlphp-module-adfs/compare/normalize_response again?

@sprytnyserek
Copy link
Author

@tvdijen Unfortunately it doesn't help. The signature part is still indented. The XML DSig method returns a DOM object that we append to the parent response, not the XML text that we would process somehow. So all spaces and newlines are actual text nodes in that DOM object. I guess that preserveWhiteSpace and formatOutput don't take any effect on that.

@tvdijen
Copy link
Member

tvdijen commented Dec 11, 2020

I think maybe they only have an effect when loading the input.. I'll try some other things when I have a little bit more time

@sprytnyserek
Copy link
Author

fyi I also double-checked that if we remove indents and newlines straight from the resulting XML right before applying as wresult, it also won't work as it impacts signature's verification.
So it must be solved at XMLDsig level. Eventually we can try it with xml-security you mentioned.

@tvdijen
Copy link
Member

tvdijen commented Dec 11, 2020

That shouldn't be the case.. The signature is over the response.. We should be able to manipulate the signature without verification issues..

@tvdijen tvdijen force-pushed the master branch 2 times, most recently from c41f99c to b12a0ad Compare July 5, 2023 20:15
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 2f5cddb to 66f0fc5 Compare March 24, 2024 19:32
@tvdijen
Copy link
Member

tvdijen commented Sep 1, 2024

Sorry this took so long, but it couldn't be done with the xmlseclibs library we used for signing metadata. I'm hoping to release v3 of this adfs-module somewhere next week and the metadata will be nice and flat then ;)

@tvdijen tvdijen closed this Sep 1, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants