Skip to content

Fix error in 1.12 that breaks SloLogoutresponse and LogoutRequest#575

Merged
pitbulk merged 3 commits intoSAML-Toolkits:masterfrom
mentimeter:fix_slo_logoutresponse
Apr 8, 2021
Merged

Fix error in 1.12 that breaks SloLogoutresponse and LogoutRequest#575
pitbulk merged 3 commits intoSAML-Toolkits:masterfrom
mentimeter:fix_slo_logoutresponse

Conversation

@JCB-K
Copy link
Copy Markdown

@JCB-K JCB-K commented Mar 4, 2021

In 1.12 idp_slo_target_url was deprecated in favour of idp_slo_service_url. However, the latter was still in use in SloLogoutresponse, which broke in the following way:

idp_metadata = <<~XML
<EntityDescriptor xmlns="urn:oasis:names:tc:SAML:2.0:metadata" xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion" ID="_d0886a22-c7b4-4dad-a631-62ce5ef18356" entityID="https://stubidp.sustainsys.com/Metadata" cacheDuration="PT15M" validUntil="2021-03-03T19:27:37Z">
  <Signature xmlns="http://www.w3.org/2000/09/xmldsig#">
    <SignedInfo>
      <CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#" />
      <SignatureMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256" />
      <Reference URI="#_d0886a22-c7b4-4dad-a631-62ce5ef18356">
        <Transforms>
          <Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature" />
          <Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#" />
        </Transforms>
        <DigestMethod Algorithm="http://www.w3.org/2001/04/xmlenc#sha256" />
        <DigestValue>0tl3z7LGVtCEWh8DxF+YTgDQkHrYG49vs9iBzOXYqIg=</DigestValue>
      </Reference>
    </SignedInfo>
    <SignatureValue>gZ4v8Vm/BP+GVg0s0h+JE9LgCq+IJCDusDEI6vTjARghbGsrkww26225Q9brV55zwq9WUyembvxOd/7Z7YTvv7z1n51a0e2qi+UK9fsX2Pdl8pX/DSAVciQkdVWJ9BLYeDAuAusnm/b8l/39xD2tFdNvb9z8frtvf0ybpWmh9As=</SignatureValue>
    <KeyInfo>
      <X509Data>
        <X509Certificate>MIICFTCCAYKgAwIBAgIQzfcJCkM1YahDtRGYsLphrDAJBgUrDgMCHQUAMCExHzAdBgNVBAMTFnN0dWJpZHAuc3VzdGFpbnN5cy5jb20wHhcNMTcxMjE0MTE1NDUwWhcNMzkxMjMxMjM1OTU5WjAhMR8wHQYDVQQDExZzdHViaWRwLnN1c3RhaW5zeXMuY29tMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDSSq8EX46J1yprfaBdh4pWII+/E7ypHM1NjG7mCwFwbkjq2tpSBuoASrQftbjIKqjVzxtxETw802VJu5CJR4d3Zdy5jD8NRTesfaQDazX7iiqisfnxmIdDhtJS0lXeBlj4MipoUW6l8Qsjx7ltZSwdfFLyh+bMqIrwOhMWGs82vQIDAQABo1YwVDBSBgNVHQEESzBJgBCBBNba7KNF5wnXqmYcejn6oSMwITEfMB0GA1UEAxMWc3R1YmlkcC5zdXN0YWluc3lzLmNvbYIQzfcJCkM1YahDtRGYsLphrDAJBgUrDgMCHQUAA4GBAHonBGahlldp7kcN5HGGnvogT8a0nNpM7GMdKhtzpLO3Uk3HyT3AAIKWiSoEv2n1BTalJ/CY/+te/JZPVGhWVzoi5JYytpj5gM0O7RH0a3/yUE8S8YLV2h0a2gsdoMvTRTnTm9CnXezCKqhjYjwsmOZtiCIYuFqX71d/pg5uoJfs</X509Certificate>
      </X509Data>
    </KeyInfo>
  </Signature>
  <IDPSSODescriptor protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
    <KeyDescriptor>
      <KeyInfo xmlns="http://www.w3.org/2000/09/xmldsig#">
        <X509Data>
          <X509Certificate>MIICFTCCAYKgAwIBAgIQzfcJCkM1YahDtRGYsLphrDAJBgUrDgMCHQUAMCExHzAdBgNVBAMTFnN0dWJpZHAuc3VzdGFpbnN5cy5jb20wHhcNMTcxMjE0MTE1NDUwWhcNMzkxMjMxMjM1OTU5WjAhMR8wHQYDVQQDExZzdHViaWRwLnN1c3RhaW5zeXMuY29tMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDSSq8EX46J1yprfaBdh4pWII+/E7ypHM1NjG7mCwFwbkjq2tpSBuoASrQftbjIKqjVzxtxETw802VJu5CJR4d3Zdy5jD8NRTesfaQDazX7iiqisfnxmIdDhtJS0lXeBlj4MipoUW6l8Qsjx7ltZSwdfFLyh+bMqIrwOhMWGs82vQIDAQABo1YwVDBSBgNVHQEESzBJgBCBBNba7KNF5wnXqmYcejn6oSMwITEfMB0GA1UEAxMWc3R1YmlkcC5zdXN0YWluc3lzLmNvbYIQzfcJCkM1YahDtRGYsLphrDAJBgUrDgMCHQUAA4GBAHonBGahlldp7kcN5HGGnvogT8a0nNpM7GMdKhtzpLO3Uk3HyT3AAIKWiSoEv2n1BTalJ/CY/+te/JZPVGhWVzoi5JYytpj5gM0O7RH0a3/yUE8S8YLV2h0a2gsdoMvTRTnTm9CnXezCKqhjYjwsmOZtiCIYuFqX71d/pg5uoJfs</X509Certificate>
        </X509Data>
      </KeyInfo>
    </KeyDescriptor>
    <ArtifactResolutionService Binding="urn:oasis:names:tc:SAML:2.0:bindings:SOAP" Location="https://stubidp.sustainsys.com/ArtifactResolve" index="0" isDefault="true" />
    <SingleLogoutService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" Location="https://stubidp.sustainsys.com/Logout" />
    <SingleLogoutService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" Location="https://stubidp.sustainsys.com/Logout" />
    <SingleSignOnService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" Location="https://stubidp.sustainsys.com/" />
    <SingleSignOnService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" Location="https://stubidp.sustainsys.com/" />
  </IDPSSODescriptor>
</EntityDescriptor>
XML

idp_metadata_parser = OneLogin::RubySaml::IdpMetadataParser.new
settings = idp_metadata_parser.parse(idp_metadata)

logout_response = OneLogin::RubySaml::SloLogoutresponse.new.create(settings)

it fails with this error:

OneLogin::RubySaml::SettingError: Invalid settings, idp_slo_target_url is not set!

I've addressed this in this PR, and done the same thing in the LogoutRequest class which has the same issue. Note: it might be worth removing the attr_accessor for idp_slo_target_url altogether from the Settings class? Because the reason that tests didn't catch this is that this attribute was/is still settable, even though it isn't in use as far as I understand.

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 4, 2021

Coverage Status

Coverage remained the same at 96.795% when pulling 9186660 on mentimeter:fix_slo_logoutresponse into 61d09d0 on onelogin:master.

@JCB-K
Copy link
Copy Markdown
Author

JCB-K commented Mar 10, 2021

@pitbulk could you have a look at this? Unless I'm misunderstanding SloLogoutresponse and LogoutRequest are broken in the latest release.

@yu-allen
Copy link
Copy Markdown

Hello @JCB-K @pitbulk, when do we plan to merge this PR? The fix looks good to me.

@pitbulk
Copy link
Copy Markdown
Collaborator

pitbulk commented Apr 2, 2021

Sorry, I will review and merge this next week and work in a minor release.

Apologies for the delay

@JCB-K
Copy link
Copy Markdown
Author

JCB-K commented Apr 7, 2021

@pitbulk I noticed you cut a new release but this PR wasn't included - what is blocking it from being merged? We (and presumably all other users of SloLogoutresponse) can't upgrade right now.

@pitbulk
Copy link
Copy Markdown
Collaborator

pitbulk commented Apr 7, 2021

Sorry I missed this PR.
I will merge it and execute a new release later today

@pitbulk pitbulk merged commit 39bd342 into SAML-Toolkits:master Apr 8, 2021
@JCB-K JCB-K deleted the fix_slo_logoutresponse branch April 8, 2021 10:45
@JCB-K
Copy link
Copy Markdown
Author

JCB-K commented Apr 12, 2021

@pitbulk Thanks for merging, will you make release 1.12.2 as well? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants