simplesamlphp / simplesamlphp-module-adfs

This module adds support for WS-Federation
GNU Lesser General Public License v2.1
5 stars 5 forks source link

Compact resulting XML #7

Closed sprytnyserek closed 1 week ago

sprytnyserek commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

sprytnyserek commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

sprytnyserek commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

tvdijen commented 1 week ago

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 ;)