search

i8beef / SAML2

Other
88 stars 43 forks source link

Attribute Statement List being incompletely read #24

Open TheDarkTrumpet opened 6 years ago

TheDarkTrumpet commented 6 years ago

Hello,

I came across a recent issue, and am looking into it a bit more but figured I'd report it here first.

While I got the library to work with what I need, the big issue that I'm running into is an incomplete list of assertions being pulled.

The assertions being read from the breakpoint are, in order: eduPersonEntitlement uid eduPersonScopedAffiliation eduPersonTargetedID

The others available are in the raw xml below, but it stops around DisplayName. My guess is that is being seen as an invalid attribute, and stops reading.

The assertion list, that comes from SAMLTracer is below (some private information removed):


<saml2:AttributeStatement>
     <saml2:Attribute FriendlyName="eduPersonEntitlement"
          Name="urn:oid:1.3.6.1.4.1.5923.1.1.1.7"
          NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
           >
            <saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
               xsi:type="xsd:string"
               >urn:mace:dir:entitlement:common-lib-terms</saml2:AttributeValue>
      </saml2:Attribute>
      <saml2:Attribute FriendlyName="uid"
            Name="urn:oid:0.9.2342.19200300.100.1.1"
            NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
            >
            <saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                     xsi:type="xsd:string"
                                      >dthole</saml2:AttributeValue>
            </saml2:Attribute>
        <saml2:Attribute FriendlyName="eduPersonScopedAffiliation"
             Name="urn:oid:1.3.6.1.4.1.5923.1.1.1.9"
             NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
             >
            <saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                      xsi:type="xsd:string"
                      >affiliate@unc.edu</saml2:AttributeValue>
               <saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                      xsi:type="xsd:string"
                      >member@unc.edu</saml2:AttributeValue>
            </saml2:Attribute>
       <saml2:Attribute FriendlyName="eduPersonTargetedID"
               Name="urn:oid:1.3.6.1.4.1.5923.1.1.1.10"
               NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
               >
                <saml2:AttributeValue>
                    <saml2:NameID Format="urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"
                                  NameQualifier="https://sso.unc.edu/idp"
                                  SPNameQualifier="https://dev.newview.io/saml"
                                  >REMOVED</saml2:NameID>
                </saml2:AttributeValue>
            </saml2:Attribute>
            <saml2:Attribute FriendlyName="displayName"
                             Name="urn:oid:2.16.840.1.113730.3.1.241"
                             NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
                             >
                <saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                                      xsi:type="xsd:string"
                                      >David Thole</saml2:AttributeValue>
            </saml2:Attribute>
            <saml2:Attribute FriendlyName="sn"
                             Name="urn:oid:2.5.4.4"
                             NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
                             >
                <saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                                      xsi:type="xsd:string"
                                      >Thole</saml2:AttributeValue>
            </saml2:Attribute>
            <saml2:Attribute FriendlyName="givenName"
                             Name="urn:oid:2.5.4.42"
                             NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
                             >
                <saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                                      xsi:type="xsd:string"
                                      >David</saml2:AttributeValue>
            </saml2:Attribute>
            <saml2:Attribute FriendlyName="pid"
                             Name="urn:oid:1.3.6.1.4.1.10411.3103.1.1.1.1"
                             NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
                             >
                <saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                                      xsi:type="xsd:string"
                                      >NUMBERREMOVED</saml2:AttributeValue>
            </saml2:Attribute>
            <saml2:Attribute FriendlyName="eduPersonPrincipalName"
                             Name="urn:oid:1.3.6.1.4.1.5923.1.1.1.6"
                             NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
                             >
                <saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                                      xsi:type="xsd:string"
                                      >dthole@unc.edu</saml2:AttributeValue>
            </saml2:Attribute>
        </saml2:AttributeStatement>
i8beef commented 6 years ago

Is that the ACTUAL assertion or an example? Are you sure those attributes are being SENT in the assertion at all?

Also do you have a RequestedAttributes (https://github.com/i8beef/SAML2/wiki/Metadata-Element) element in your config specifying these extra attributes you want? SAML let's you specify what attributes your application requires from the IDP, and if you aren't specifying these, it's possible they are only sending you their "defaults".

TheDarkTrumpet commented 6 years ago

Yeah, this is pretty much the exact thing sent by the IDP. I removed some very private information, but described the element type it was. To describe that process, I used Firefox and SamlTracer to pull the SAML from the redirect, and what you see pasted above is a slight modification of exactly what they provided. So yeah, I'm sure that's the actual assertion.

Regarding the Metadata element, I actually tried that before pasting here. I tried removing all but eduPersonPrincipalName, since that's the one I actually need - and no change. The 4 that came through originally were the 4 that came through after changing the Web.conf. One thing I haven't tried in this case is trying to specify the name specifically (urn:oid:1.3.6.1.4.1.5923.1.1.1.6) to see if that'd come through.

I did end up stepping through a fair amount of the code looking into this before I asked UNC to update their attributes and where I left off is https://github.com/i8beef/SAML2/blob/c07b7a475779ddb24daac1ffb15400b367da2d34/src/SAML2/Validation/Saml20StatementValidator.cs#L62 - which didn't have the extra attributes (only the first 4). Really comparing the DisplayName and others that parsed, the only thing I could think of that was different was the space. Looking in the code, I didn't see any default number to pull in.

They, since this morning, modified the attributes being sent back, so I'm not able to test it with that instance. The example app I made you a contributor on is where I'll continue testing this on. I asked the uiowa identity team (my group) to expose those same attributes to my test app, which has a lot more logging in place for me to go further on.

Which, leads me to the other issue I submitted. I apologize for not getting back with you on these in a timely manner. I'm the only developer working on this product, and I have a lot of hard deadlines that won't ease up til the end of the month. I have been keeping a log of items that may need addressing (at least in documentation), and I have a few code fixes I may make pull requests for. I have every intention to really help out on this, right now I'm more or less in crunch time so just keeping notes of what's going on right now.

As an aside, I'm working with my university and Internet2 to create some documentation on Shibboleth authentication in Azure that references your library and getting it all setup. My intention is to take that example app, and through a wiki, include this guide. I'm not sure if Internet2 will update their documentation to include my stuff, or provide a link, or whatever. Still, I recently had Wisconsin asking for help, which I provided the example app and they got Shibboleth working. If you're interested in seeing this document, I can share it with you while I work on generalizing it and getting the example app totally finished.

TheDarkTrumpet commented 6 years ago

One thing I forgot to mention that's worth mentioning. This isn't a high priority item exactly, and the task is as much for me as it is for you. I'm hoping to dive into the code and try and fix this issue and include a pull request, assuming you don't do it first. I'm finding that I'm learning quite a bit from the way you handle XML and the deserializing from the assertion, and as a programmer - I want to learn how this stuff works. This, right now, isn't blocking anything.

i8beef commented 6 years ago

I suspect that you have a serialization issue around eduPersonTargetedID. Notice there is a saml2:NameID element within the AttributeValue. That isn't supported by the SAML spec. You sure you pasted that right?

TheDarkTrumpet commented 6 years ago

I was pretty sure I pasted it exactly from SamlTrace, and I didn't notice this til you mentioned it.

Once my university (uiowa) updates the attributes released to my test app, I'll try it again with them. If it successfully pulls everything in, then I'll see if UNC minds updating the released attributes they send to me and go from there.

I'll need to be a bit careful. I don't want to inconvenience our partner university too much, but if this is a bug in how they are sending information out of the shibboleth idp, then I would assume they want to know about it and would want to fix it. I'll be in touch on that. I think my university will have this updated this week, so by the end of the week I should have an update once they release the attributes.

Thanks again for your help. I'll be in touch as soon as they get this setup, earliest tomorrow after this immediate deadline.

sosumi commented 6 years ago

So, saml2:NameID as the AttributeValue of eduPersonTargetedID is perfectly valid per the SAML spec. In fact, it's literally the definition of the the attribute.

The other university (who I won't name here) isn't doing anything wrong. It's the same way my institution releases the attribute.

If that's the issue, then that's a deserialization bug in this library.

i8beef commented 6 years ago

That may be what they are sending, but according to the OASIS SAML 2 specification, I'm not seeing that as a valid option: https://www.oasis-open.org/committees/download.php/35711/sstc-saml-core-errata-2.0-wd-06-diff.pdf

That particular element typically occurs in the saml:Subject element, not under Attributes, kind of like these examples: https://www.samltool.com/generic_sso_res.php

Can you point to the spec location that says complex types are supported under AttributeValue?

i8beef commented 6 years ago

If this is indeed the issue, I'd expect you would be throwing an InvalidOperation exception during the deserialization step of the assertion load. If this is to be supported, I suspect some sort of custom serialization would have to be put in place to explicitely treat all sub-nodes of AttributeValue as a string value instead of attempting deserialize any found XML as an object. I'm not sure how to do that, so I'd have to look a bit and see if I can figure out how to do that without some drastic changes to the serialization approach.

sosumi commented 6 years ago

Per the latest version of the standard (with errata included): https://www.oasis-open.org/committees/download.php/56777/sstc-saml-core-errata-2.0-wd-07-diff.pdf

Lines 1313 - 1314:

The element supplies the value of a specified SAML attribute. It is of the xs:anyType type, which allows any well-formed XML to appear as the content of the element.

eduPersonTargetedID is formally defined in the eduPerson schema here: http://software.internet2.edu/eduperson/internet2-mace-dir-eduperson-201602.html#eduPersonTargetedID

It's more specifically defined in a SAML context in the MACE-Dir SAML Attribute Profile: http://macedir.org/docs/internet2-mace-dir-saml-attributes-latest.pdf

A definition put together by the Swiss Identity and Access Federation (with links to relevant documentation) can be found here: https://www.switch.ch/aai/support/documents/attributes/edupersontargetedid/

For Scott Cantor's take on the history of the eduPersonTargetedID attribute and how it came about (you'll note that he's the lead editor of the OASIS SAML 2.0 standard), see here: https://wiki.shibboleth.net/confluence/display/SHIB2/NativeSPTargetedID

I'm not saying it's a fantastic solution, but it's nearly universal practice in the higher education SAML 2.0 space, and is perfectly allowable per the specification.

i8beef commented 6 years ago

Yeah, that's interesting. Kind throws a wrench in a few things, but I think we could probably handle this by taking a slightly longer route around the problem... Right now this library assumes that everything passed to it as an AttributeValue will be a string, but the serializer is going to hate having XML in those fields. I can't find an easy way to make it just take the content and treat it as a string value during serialize / deserialize, so I think the best bet is probably going to be to make AttributeValue and actual type in Schema/Core, with an XmlText property as well as an XmlAnyElement property to catch both cases.

We don't use any of the attributes DIRECTLY in code (aside from attribute queries, which I still need to look at), and it actually just slaps the attribute element directly into the Principal (something we inherited...) That would let anyone using the Saml20Principal set by the included SamlPrincpalAction read the value of the attribute as EITHER an Attribute.Value property (which would be null in this case), or an Attribute.AnyElement property (which would contain the XmlElement structure passed in the attribute).

I can put together those changes this week.

What actually makes me real curious though is how you don't have an exception getting thrown that's blowing up the whole process. This totally should cause an XmlSerialization issue, and I can't for the life of me find anywhere that I SWALLOW said exception... in the Saml20AbstractEndpoint class there's a try catch wrapping the handlers, but on Exception it rethrows (it only catches for logging purposes).

i8beef commented 6 years ago

Can you guys try that branch and see if it gets you over the hump here? I still need to review the rest of the pieces this has an effect on, but I think this will make your assertion deserialize at least. When you go to get the values that are passed, if a compelx object is passed like the NameId above, you will have to access the Attribute.AttributeValues and check if it's Value is null. You will have access to an XmlElement structure though in there as "AnyElements" for any pieces you want.

TheDarkTrumpet commented 6 years ago

I can try it around Thursday, I have a few major issues I need to get out by tomorrow afternoon.

TheDarkTrumpet commented 6 years ago

Hey,

Sorry for the late reply on all this. We were changing things a bit on the SP side today, and I figured this would be a good time to test out the changes. I checked out, built the source from source, and gave it a try. I think the changes are in the right direction, but some attributes may need to be added. I experimented a bit with adding some of the attributes to the AttributeValue sections, seeing if I could get it, but nada so far. I haven't had to mess with XML in C# yet, so my knowledge of how to solve this is limited. I wanted to get the system back up quickly, so I added in a patch that pulls the first attribute that does parse and gets passed in. Meaning, I can't link it directly to a user without some intermediate steps, but it works to verify authentication for now.

Assuming the SAML response is read from top down, and chokes somewhere, it appears the displayName may be the best place to look. Part of me thinks it's the xsi:type part in the XML that's causing the issue.

The error I got is the following:

The specified type was not recognized: name='string', namespace='http://www.w3.org/2001/XMLSchema', at <AttributeValue xmlns='urn:oasis:names:tc:SAML:2.0:assertion'>.
 Description: An unhandled exception occurred during the execution of the current web request. Please review the stack trace for more information about the error and where it originated in the code.

 Exception Details: System.InvalidOperationException: The specified type was not recognized: name='string', namespace='http://www.w3.org/2001/XMLSchema', at <AttributeValue xmlns='urn:oasis:names:tc:SAML:2.0:assertion'>.

 Source Error:

 An unhandled exception was generated during the execution of the current web request. Information regarding the origin and location of the exception can be identified using the exception stack trace below.

 Stack Trace:

 [InvalidOperationException: The specified type was not recognized: name='string', namespace='http://www.w3.org/2001/XMLSchema', at <AttributeValue xmlns='urn:oasis:names:tc:SAML:2.0:assertion'>.]
    Microsoft.Xml.Serialization.GeneratedAssembly.XmlSerializationReaderAssertion.Read47_SamlAttributeValue(Boolean isNullable, Boolean checkType) +810
    Microsoft.Xml.Serialization.GeneratedAssembly.XmlSerializationReaderAssertion.Read49_SamlAttribute(Boolean isNullable, Boolean checkType) +1271
    Microsoft.Xml.Serialization.GeneratedAssembly.XmlSerializationReaderAssertion.Read50_AttributeStatement(Boolean isNullable, Boolean checkType) +570
    Microsoft.Xml.Serialization.GeneratedAssembly.XmlSerializationReaderAssertion.Read64_Assertion(Boolean isNullable, Boolean checkType) +1354
    Microsoft.Xml.Serialization.GeneratedAssembly.XmlSerializationReaderAssertion.Read65_Assertion() +96

 [InvalidOperationException: There is an error in the XML document.]
    System.Xml.Serialization.XmlSerializer.Deserialize(XmlReader xmlReader, String encodingStyle, XmlDeserializationEvents events) +694
    System.Xml.Serialization.XmlSerializer.Deserialize(XmlReader xmlReader) +28
    SAML2.Utils.Serialization.Deserialize(XmlReader reader) in Z:\dthole\Programming.NEW\SAML2\src\SAML2\Utils\Serialization.cs:37
    SAML2.Saml20Assertion.get_Assertion() in Z:\dthole\Programming.NEW\SAML2\src\SAML2\Saml20Assertion.cs:134
    SAML2.Saml20Assertion.LoadXml(XmlElement element, IEnumerable`1 trustedSigners) in Z:\dthole\Programming.NEW\SAML2\src\SAML2\Saml20Assertion.cs:630
    SAML2.Saml20Assertion..ctor(XmlElement assertion, IEnumerable`1 trustedSigners, String profile, Boolean quirksMode) in Z:\dthole\Programming.NEW\SAML2\src\SAML2\Saml20Assertion.cs:96
    SAML2.Protocol.Saml20SignonHandler.HandleAssertion(HttpContext context, XmlElement elem) in Z:\dthole\Programming.NEW\SAML2\src\SAML2\Protocol\Saml20SignonHandler.cs:381
    SAML2.Protocol.Saml20SignonHandler.HandleResponse(HttpContext context) in Z:\dthole\Programming.NEW\SAML2\src\SAML2\Protocol\Saml20SignonHandler.cs:488
    SAML2.Protocol.Saml20SignonHandler.Handle(HttpContext context) in Z:\dthole\Programming.NEW\SAML2\src\SAML2\Protocol\Saml20SignonHandler.cs:159
    SAML2.Protocol.Saml20AbstractEndpointHandler.ProcessRequest(HttpContext context) in Z:\dthole\Programming.NEW\SAML2\src\SAML2\Protocol\Saml20AbstractEndpointHandler.cs:81
    System.Web.CallHandlerExecutionStep.System.Web.HttpApplication.IExecutionStep.Execute() +811
    System.Web.HttpApplication.ExecuteStepImpl(IExecutionStep step) +132
    System.Web.HttpApplication.ExecuteStep(IExecutionStep step, Boolean& completedSynchronously) +73

The attribute statement is below, I'm removing a lot of the values for more security-related reasons, but leaving in most:

        <saml2:AttributeStatement>
            <saml2:Attribute FriendlyName="eduPersonTargetedID"
                             Name="urn:oid:1.3.6.1.4.1.5923.1.1.1.10"
                             NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
                             >
                <saml2:AttributeValue>
                    <saml2:NameID Format="urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"
                                  NameQualifier="https://IDP"
                                  SPNameQualifier="https://SP"
                                  >REMOVED</saml2:NameID>
                </saml2:AttributeValue>
            </saml2:Attribute>
            <saml2:Attribute FriendlyName="displayName"
                             Name="urn:oid:2.16.840.1.113730.3.1.241"
                             NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
                             >
                <saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                                      xsi:type="xsd:string"
                                      > REMOVED</saml2:AttributeValue>
            </saml2:Attribute>
            <saml2:Attribute FriendlyName="sn"
                             Name="urn:oid:2.5.4.4"
                             NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
                             >
                <saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                                      xsi:type="xsd:string"
                                      > REMOVED</saml2:AttributeValue>
            </saml2:Attribute>
            <saml2:Attribute FriendlyName="givenName"
                             Name="urn:oid:2.5.4.42"
                             NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
                             >
                <saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                                      xsi:type="xsd:string"
                                      > REMOVED</saml2:AttributeValue>
            </saml2:Attribute>
            <saml2:Attribute FriendlyName="eduPersonPrincipalName"
                             Name="urn:oid:1.3.6.1.4.1.5923.1.1.1.6"
                             NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
                             >
                <saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                                      xsi:type="xsd:string"
                                      > REMOVED</saml2:AttributeValue>
            </saml2:Attribute>
        </saml2:AttributeStatement>

I won't have a chance to mess with this again til the weekend since the system is actually used during active hours. If I get any further on this, I'll post, but maybe the issue is very apparent. Thanks again for your help.

i8beef commented 6 years ago

Man you might have me stumped there, I'm not sure. Have you tried putting the XML response through an actual XML validator to make sure all the namespaces are actually valid?

Do you see this NS definition anywhere in the actual message?

xmlns:xsd="http://www.w3.org/2001/XMLSchema"
TheDarkTrumpet commented 6 years ago

Yeah, that's there. Near the top, by the assertion id:

<saml2:Assertion ID="_3ced8f2e1293f949985c6fedac0f38de"
                     IssueInstant="2018-02-07T21:23:03.932Z"
                     Version="2.0"
                     xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion"
                     xmlns:xsd="http://www.w3.org/2001/XMLSchema"
                     >

Ran it though https://www.xmlvalidation.com and it's valid.

i8beef commented 6 years ago

Ill have to setup a unit test to test the serialization on this. My best guess is that it doesn't like the NameID part of this. I know the error reads like its having trouble with the ones after that, but I wonder if it isn't complaining that the object its trying to deserialize ISN'T of type string, which is what it expects.

One option might be to make a real AttributeValue type that takes both an XmlText Value and object[] Items and see if the serializer will take that instead. But all the current tests are a little dependent on a full assertion, I'd like to set one up that attempts to just deserialize this part.

Luismoteando commented 3 years ago

Hi @TheDarkTrumpet! How did you solved it?