Put <ds:Signature> as the first child for enveloped signature.

[smstong]

The current code has two issues:

(1) It add sig to the signed element's Child, but does NOT update the index and parent. It should call "Element.AddChild(t)"

(2) the ds:Signature is added as the last child. But based on SAML standard, this should be the first child.

Both of these issues will be solved by this update.

[russellhaering]

Do you have a reference on SAML wanting ds:Signature to be the first child?

I think you're right that there's an issue here, but at a glance what I'm seeing suggests that in SAML the ds:Signature should always come after the saml:Issuer but before anything else (where "anything else" varies depending on whether we're talking about a Request, Response, etc).

[smstong]

As you said, for SAML Assertion, ds:Signature is the 2nd child element.

<element name="Assertion" type="saml:AssertionType"/>
<complexType name="AssertionType">
<sequence>
<element ref="saml:Issuer"/>
<element ref="ds:Signature" minOccurs="0"/>
<element ref="saml:Subject" minOccurs="0"/>
<element ref="saml:Conditions" minOccurs="0"/>
<element ref="saml:Advice" minOccurs="0"/>
<choice minOccurs="0" maxOccurs="unbounded">
<element ref="saml:Statement"/>
<element ref="saml:AuthnStatement"/>
<element ref="saml:AuthzDecisionStatement"/>
<element ref="saml:AttributeStatement"/>
</choice>
</sequence>
<attribute name="Version" type="string" use="required"/>
<attribute name="ID" type="ID" use="required"/>
<attribute name="IssueInstant" type="dateTime" use="required"/>
</complexType>

But for SAML Metadata, ds:Signature is always the first child element.

<element name="EntitiesDescriptor" type="md:EntitiesDescriptorType"/>
<complexType name="EntitiesDescriptorType">
<sequence>
<element ref="ds:Signature" minOccurs="0"/>
<element ref="md:Extensions" minOccurs="0"/>
<choice minOccurs="1" maxOccurs="unbounded">
<element ref="md:EntityDescriptor"/>
<element ref="md:EntitiesDescriptor"/>
</choice>
</sequence>
<attribute name="validUntil" type="dateTime" use="optional"/>
<attribute name="cacheDuration" type="duration" use="optional"/><attribute name="ID" type="ID" use="optional"/>
<attribute name="Name" type="string" use="optional"/>
</complexType>
<element name="Extensions" type="md:ExtensionsType"/>
<complexType final="#all" name="ExtensionsType">
<sequence>
<any namespace="##other" processContents="lax" maxOccurs="unbounded"/>
</sequence>
</complexType>
<element name="EntityDescriptor" type="md:EntityDescriptorType"/>
<complexType name="EntityDescriptorType">
<sequence>
<element ref="ds:Signature" minOccurs="0"/>
<element ref="md:Extensions" minOccurs="0"/>
<choice>
<choice maxOccurs="unbounded">
<element ref="md:RoleDescriptor"/>
<element ref="md:IDPSSODescriptor"/>
<element ref="md:SPSSODescriptor"/>
<element ref="md:AuthnAuthorityDescriptor"/>
<element ref="md:AttributeAuthorityDescriptor"/>
<element ref="md:PDPDescriptor"/>
</choice>
<element ref="md:AffiliationDescriptor"/>
</choice>
<element ref="md:Organization" minOccurs="0"/>
<element ref="md:ContactPerson" minOccurs="0" maxOccurs="unbounded"/>
<element ref="md:AdditionalMetadataLocation" minOccurs="0" maxOccurs="unbounded"/>
</sequence>
<attribute name="entityID" type="md:entityIDType" use="required"/>
<attribute name="validUntil" type="dateTime" use="optional"/>
<attribute name="cacheDuration" type="duration" use="optional"/>
<attribute name="ID" type="ID" use="optional"/>
<anyAttribute namespace="##other" processContents="lax"/>
</complexType>

To make this API to acommodate all different situations, probaly it needs another parmamer to denote the index of ds:Signature, like below:

func (ctx *SigningContext) SignEnveloped(el *etree.Element, index int) (*etree.Element, error) {
    sig, err := ctx.ConstructSignature(el, true)
    if err != nil {
        return nil, err
    }

    ret := el.Copy()
    ret.InsertChildAt(index, sig)

    return ret, nil
}