Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error in 1.12 that breaks SloLogoutresponse and LogoutRequest #575

Merged
merged 3 commits into from
Apr 8, 2021

Conversation

JCB-K
Copy link
Contributor

@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

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
Contributor 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

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

@pitbulk
Copy link
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
Contributor 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
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
Contributor 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