search

perl-net-saml2 / perl-Net-SAML2

Net::SAML2 - SAML bindings and protocol implementation
https://metacpan.org/pod/Net::SAML2
Other
8 stars 7 forks source link

Metadata containing multiple "signing" keys not parsed properly #12

Closed timlegge closed 2 years ago

timlegge commented 3 years ago

Net::SAML2::IdP assumes only new signing certificate in metadata.xml. Multiple signing certificates will cause the verification to fail and the IdP object creation to fail

ghost commented 3 years ago

Hi! Please check this metadata xml from Microsoft Azure. It not work also, because you combine all X509Certificates into one, and this broke cert validation.

ghost commented 3 years ago

azure.txt

timlegge commented 3 years ago

There are two signing keys see https://github.com/perl-net-saml2/perl-Net-SAML2/issues/12

If you are retrieving it from a local server change signing to signing-old for one of them.

There is a fix in the https://github.com/perl-net-saml2/perl-Net-SAML2/pull/17 but I still need to do real testing on it

Tim

On Tue, Feb 2, 2021 at 7:05 AM aaalexby notifications@github.com wrote:

azure.txt https://github.com/perl-net-saml2/perl-Net-SAML2/files/5910641/azure.txt

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/perl-net-saml2/perl-Net-SAML2/issues/12#issuecomment-771556936, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH3N6ZJMTETPCL7BWQESFLS47MAVANCNFSM4WGVIQMA .

ghost commented 3 years ago

I have tested Azure with your latest changes also. The same result. I open Azure metadata url, and copy all content into azure.txt

timlegge commented 3 years ago

So you cloned from https://github.com/timlegge/perl-Net-SAML2/tree/issue12-multiple-keys and ensured that the old module was replaced?

ghost commented 3 years ago

There are two signing keys see #12 If you are retrieving it from a local server change signing to signing-old for one of them. There is a fix in the #17 but I still need to do real testing on it Tim … On Tue, Feb 2, 2021 at 7:05 AM aaalexby @.***> wrote: azure.txt https://github.com/perl-net-saml2/perl-Net-SAML2/files/5910641/azure.txt — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#12 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH3N6ZJMTETPCL7BWQESFLS47MAVANCNFSM4WGVIQMA .

So you cloned from https://github.com/timlegge/perl-Net-SAML2/tree/issue12-multiple-keys and ensured that the old module was replaced?

I just edit IdP.pm on my host and replace old content with new, from this: https://github.com/timlegge/perl-Net-SAML2/commit/c8e4285a17bdd20534e516931009661498113ef4#diff-38d42ddde77ce9b23a7b5689c6185793bee6f3ac92b5eceb741176342132ad10

timlegge commented 3 years ago

Also lib/Net/SAML2/Binding/Redirect.pm and send me the microsoft CACert for testing

timlegge commented 3 years ago

I can reproduce

ghost commented 3 years ago

Also lib/Net/SAML2/Binding/Redirect.pm and send me the microsoft CACert for testing

Binding/Redirect.pm don't need, because scripts broke before it calling.

Microsoft CAcert included in metadata azure.txt

ghost commented 3 years ago

azure.cer.txt

ghost commented 3 years ago

Some data from debugger. As you can see, as result one BIG certificate:

'entityid' => 'https://sts.windows.net/239f867f-feea-452e-a800-6859e696161c/', 'certs' => [ { 'signing' => '-----BEGIN CERTIFICATE----- MIIDhTCCAm2gAwIBAgIJAJZ1R0cT7w9TMA0GCSqGSIb3DQEBCwUAMFkxCzAJBgNV BAYTAlhYMRUwEwYDVQQHDAxEZWZhdWx0IENpdHkxHDAaBgNVBAoME0RlZmF1bHQg Q29tcGFueSBMdGQxFTATBgNVBAMMDDE3Mi4xOS4zLjEwNTAeFw0yMTAxMjgxMTUz MzhaFw0yMjAxMjMxMTUzMzhaMFkxCzAJBgNVBAYTAlhYMRUwEwYDVQQHDAxEZWZh dWx0IENpdHkxHDAaBgNVBAoME0RlZmF1bHQgQ29tcGFueSBMdGQxFTATBgNVBAMM DDE3Mi4xOS4zLjEwNTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAMwN QPIeuOwS6hDOgMA0cpViO/pwCiJyL034E2fN6fpGs2i1Q+jiIww250P2Wa13r36f gtcfeZpzxZttaOLHoXUsjDT58Gjq2wvVnmtdMJJRPpb65RQbUjazXnGlFHzSXEyF IKt29Jn/ZwR0/6L9ZrajwUKWmynOGx+zPd2dsktFLDM7FjnP1cQs//CGc1xUcixS nR1YNihCSbqU5xpVOFFJuaFXYMpMRKqZPL3dUpwEf0ZPixPU0rzw4R8Yl/HDZeI5 CKfrSmZv0Q1NUA/V9nPLUArx06FXcSfOmALPVXZUrNNRLr8pTJSYtGRVUol2WkNR peTwyHqdHcfsUNM+ZFECAwEAAaNQME4wHQYDVR0OBBYEFMY/cY30c1GAiTlfveCW yhRm0aYJMB8GA1UdIwQYMBaAFMY/cY30c1GAiTlfveCWyhRm0aYJMAwGA1UdEwQF MAMBAf8wDQYJKoZIhvcNAQELBQADggEBAKeJoEIze7KMglyzgCoQGwcTfc8gTpHP 2pCxo7VB0/kLRTsIaeNU1FA/HZzWHXqQriiYQ7SS7IpPiBca4QL0twm6Ks2hMU6j SXnp23/i8KphqJ/hckX6RrdK5NPbMxwls7poYM97cVnDpo8FDnQvCBY9t3+GmyaA 2VWl7tfwRHB6zW60Wr8nFf/w/VRZDuiZ4FUy4oekh0ShtjB7ae6t8tUgWscMp6bn oQ0cY/gzsdPALvR0ggUAPkcYujoPM9T+Gu08P+zgXXWPtFjgQIN5hMKGoaStPM2S RqjRPOsWZmknNOxXxsbvHlam/49ej429TiIB+AbKWTKOaXvo5QgYv6k=MIIDhTCC Am2gAwIBAgIJAJZ1R0cT7w9TMA0GCSqGSIb3DQEBCwUAMFkxCzAJBgNVBAYTAlhY MRUwEwYDVQQHDAxEZWZhdWx0IENpdHkxHDAaBgNVBAoME0RlZmF1bHQgQ29tcGFu eSBMdGQxFTATBgNVBAMMDDE3Mi4xOS4zLjEwNTAeFw0yMTAxMjgxMTUzMzhaFw0y MjAxMjMxMTUzMzhaMFkxCzAJBgNVBAYTAlhYMRUwEwYDVQQHDAxEZWZhdWx0IENp dHkxHDAaBgNVBAoME0RlZmF1bHQgQ29tcGFueSBMdGQxFTATBgNVBAMMDDE3Mi4x OS4zLjEwNTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAMwNQPIeuOwS 6hDOgMA0cpViO/pwCiJyL034E2fN6fpGs2i1Q+jiIww250P2Wa13r36fgtcfeZpz xZttaOLHoXUsjDT58Gjq2wvVnmtdMJJRPpb65RQbUjazXnGlFHzSXEyFIKt29Jn/ ZwR0/6L9ZrajwUKWmynOGx+zPd2dsktFLDM7FjnP1cQs//CGc1xUcixSnR1YNihC SbqU5xpVOFFJuaFXYMpMRKqZPL3dUpwEf0ZPixPU0rzw4R8Yl/HDZeI5CKfrSmZv 0Q1NUA/V9nPLUArx06FXcSfOmALPVXZUrNNRLr8pTJSYtGRVUol2WkNRpeTwyHqd HcfsUNM+ZFECAwEAAaNQME4wHQYDVR0OBBYEFMY/cY30c1GAiTlfveCWyhRm0aYJ MB8GA1UdIwQYMBaAFMY/cY30c1GAiTlfveCWyhRm0aYJMAwGA1UdEwQFMAMBAf8w DQYJKoZIhvcNAQELBQADggEBAKeJoEIze7KMglyzgCoQGwcTfc8gTpHP2pCxo7VB 0/kLRTsIaeNU1FA/HZzWHXqQriiYQ7SS7IpPiBca4QL0twm6Ks2hMU6jSXnp23/i 8KphqJ/hckX6RrdK5NPbMxwls7poYM97cVnDpo8FDnQvCBY9t3+GmyaA2VWl7tfw RHB6zW60Wr8nFf/w/VRZDuiZ4FUy4oekh0ShtjB7ae6t8tUgWscMp6bnoQ0cY/gz sdPALvR0ggUAPkcYujoPM9T+Gu08P+zgXXWPtFjgQIN5hMKGoaStPM2SRqjRPOsW ZmknNOxXxsbvHlam/49ej429TiIB+AbKWTKOaXvo5QgYv6k=MIIDhTCCAm2gAwIB AgIJAJZ1R0cT7w9TMA0GCSqGSIb3DQEBCwUAMFkxCzAJBgNVBAYTAlhYMRUwEwYD VQQHDAxEZWZhdWx0IENpdHkxHDAaBgNVBAoME0RlZmF1bHQgQ29tcGFueSBMdGQx FTATBgNVBAMMDDE3Mi4xOS4zLjEwNTAeFw0yMTAxMjgxMTUzMzhaFw0yMjAxMjMx MTUzMzhaMFkxCzAJBgNVBAYTAlhYMRUwEwYDVQQHDAxEZWZhdWx0IENpdHkxHDAa BgNVBAoME0RlZmF1bHQgQ29tcGFueSBMdGQxFTATBgNVBAMMDDE3Mi4xOS4zLjEw NTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAMwNQPIeuOwS6hDOgMA0 cpViO/pwCiJyL034E2fN6fpGs2i1Q+jiIww250P2Wa13r36fgtcfeZpzxZttaOLH oXUsjDT58Gjq2wvVnmtdMJJRPpb65RQbUjazXnGlFHzSXEyFIKt29Jn/ZwR0/6L9 ZrajwUKWmynOGx+zPd2dsktFLDM7FjnP1cQs//CGc1xUcixSnR1YNihCSbqU5xpV OFFJuaFXYMpMRKqZPL3dUpwEf0ZPixPU0rzw4R8Yl/HDZeI5CKfrSmZv0Q1NUA/V 9nPLUArx06FXcSfOmALPVXZUrNNRLr8pTJSYtGRVUol2WkNRpeTwyHqdHcfsUNM+ ZFECAwEAAaNQME4wHQYDVR0OBBYEFMY/cY30c1GAiTlfveCWyhRm0aYJMB8GA1Ud IwQYMBaAFMY/cY30c1GAiTlfveCWyhRm0aYJMAwGA1UdEwQFMAMBAf8wDQYJKoZI hvcNAQELBQADggEBAKeJoEIze7KMglyzgCoQGwcTfc8gTpHP2pCxo7VB0/kLRTsI aeNU1FA/HZzWHXqQriiYQ7SS7IpPiBca4QL0twm6Ks2hMU6jSXnp23/i8KphqJ/h ckX6RrdK5NPbMxwls7poYM97cVnDpo8FDnQvCBY9t3+GmyaA2VWl7tfwRHB6zW60 Wr8nFf/w/VRZDuiZ4FUy4oekh0ShtjB7ae6t8tUgWscMp6bnoQ0cY/gzsdPALvR0 ggUAPkcYujoPM9T+Gu08P+zgXXWPtFjgQIN5hMKGoaStPM2SRqjRPOsWZmknNOxX xsbvHlam/49ej429TiIB+AbKWTKOaXvo5QgYv6k=MIIDhTCCAm2gAwIBAgIJAJZ1 R0cT7w9TMA0GCSqGSIb3DQEBCwUAMFkxCzAJBgNVBAYTAlhYMRUwEwYDVQQHDAxE ZWZhdWx0IENpdHkxHDAaBgNVBAoME0RlZmF1bHQgQ29tcGFueSBMdGQxFTATBgNV BAMMDDE3Mi4xOS4zLjEwNTAeFw0yMTAxMjgxMTUzMzhaFw0yMjAxMjMxMTUzMzha MFkxCzAJBgNVBAYTAlhYMRUwEwYDVQQHDAxEZWZhdWx0IENpdHkxHDAaBgNVBAoM E0RlZmF1bHQgQ29tcGFueSBMdGQxFTATBgNVBAMMDDE3Mi4xOS4zLjEwNTCCASIw DQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAMwNQPIeuOwS6hDOgMA0cpViO/pw CiJyL034E2fN6fpGs2i1Q+jiIww250P2Wa13r36fgtcfeZpzxZttaOLHoXUsjDT5 8Gjq2wvVnmtdMJJRPpb65RQbUjazXnGlFHzSXEyFIKt29Jn/ZwR0/6L9ZrajwUKW mynOGx+zPd2dsktFLDM7FjnP1cQs//CGc1xUcixSnR1YNihCSbqU5xpVOFFJuaFX YMpMRKqZPL3dUpwEf0ZPixPU0rzw4R8Yl/HDZeI5CKfrSmZv0Q1NUA/V9nPLUArx 06FXcSfOmALPVXZUrNNRLr8pTJSYtGRVUol2WkNRpeTwyHqdHcfsUNM+ZFECAwEA AaNQME4wHQYDVR0OBBYEFMY/cY30c1GAiTlfveCWyhRm0aYJMB8GA1UdIwQYMBaA FMY/cY30c1GAiTlfveCWyhRm0aYJMAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQEL BQADggEBAKeJoEIze7KMglyzgCoQGwcTfc8gTpHP2pCxo7VB0/kLRTsIaeNU1FA/ HZzWHXqQriiYQ7SS7IpPiBca4QL0twm6Ks2hMU6jSXnp23/i8KphqJ/hckX6RrdK 5NPbMxwls7poYM97cVnDpo8FDnQvCBY9t3+GmyaA2VWl7tfwRHB6zW60Wr8nFf/w /VRZDuiZ4FUy4oekh0ShtjB7ae6t8tUgWscMp6bnoQ0cY/gzsdPALvR0ggUAPkcY ujoPM9T+Gu08P+zgXXWPtFjgQIN5hMKGoaStPM2SRqjRPOsWZmknNOxXxsbvHlam /49ej429TiIB+AbKWTKOaXvo5QgYv6k= -----END CERTIFICATE----- '

But should be like this: 'entityid' => 'https://sts.windows.net/239f867f-feea-452e-a800-6859e696161c/', 'certs' => [ { 'signing' => '-----BEGIN CERTIFICATE----- MIIDhTCCAm2gAwIBAgIJAJZ1R0cT7w9TMA0GCSqGSIb3DQEBCwUAMFkxCzAJBgNV BAYTAlhYMRUwEwYDVQQHDAxEZWZhdWx0IENpdHkxHDAaBgNVBAoME0RlZmF1bHQg Q29tcGFueSBMdGQxFTATBgNVBAMMDDE3Mi4xOS4zLjEwNTAeFw0yMTAxMjgxMTUz MzhaFw0yMjAxMjMxMTUzMzhaMFkxCzAJBgNVBAYTAlhYMRUwEwYDVQQHDAxEZWZh dWx0IENpdHkxHDAaBgNVBAoME0RlZmF1bHQgQ29tcGFueSBMdGQxFTATBgNVBAMM DDE3Mi4xOS4zLjEwNTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAMwN QPIeuOwS6hDOgMA0cpViO/pwCiJyL034E2fN6fpGs2i1Q+jiIww250P2Wa13r36f gtcfeZpzxZttaOLHoXUsjDT58Gjq2wvVnmtdMJJRPpb65RQbUjazXnGlFHzSXEyF IKt29Jn/ZwR0/6L9ZrajwUKWmynOGx+zPd2dsktFLDM7FjnP1cQs//CGc1xUcixS nR1YNihCSbqU5xpVOFFJuaFXYMpMRKqZPL3dUpwEf0ZPixPU0rzw4R8Yl/HDZeI5 CKfrSmZv0Q1NUA/V9nPLUArx06FXcSfOmALPVXZUrNNRLr8pTJSYtGRVUol2WkNR peTwyHqdHcfsUNM+ZFECAwEAAaNQME4wHQYDVR0OBBYEFMY/cY30c1GAiTlfveCW yhRm0aYJMB8GA1UdIwQYMBaAFMY/cY30c1GAiTlfveCWyhRm0aYJMAwGA1UdEwQF MAMBAf8wDQYJKoZIhvcNAQELBQADggEBAKeJoEIze7KMglyzgCoQGwcTfc8gTpHP 2pCxo7VB0/kLRTsIaeNU1FA/HZzWHXqQriiYQ7SS7IpPiBca4QL0twm6Ks2hMU6j SXnp23/i8KphqJ/hckX6RrdK5NPbMxwls7poYM97cVnDpo8FDnQvCBY9t3+GmyaA 2VWl7tfwRHB6zW60Wr8nFf/w/VRZDuiZ4FUy4oekh0ShtjB7ae6t8tUgWscMp6bn oQ0cY/gzsdPALvR0ggUAPkcYujoPM9T+Gu08P+zgXXWPtFjgQIN5hMKGoaStPM2S RqjRPOsWZmknNOxXxsbvHlam/49ej429TiIB+AbKWTKOaXvo5QgYv6k= -----END CERTIFICATE----- '

timlegge commented 3 years ago

Thanks - looks like in my testing metadata I changed one of the X509Certificate to X509Certificate1 for one of the certificate in the metadata.

Developer being unobservant...

Okay, I can fix it but it will be tonight.

In the mean time you can likely:

  1. tell azure to only send one signing cert (maybe) I did not realize it could have two active at the same time
  2. retrieve the metadata from the local web server with:
    1. one of the signing changed to signing1
    2. one of the X509Certificate change to X509Certificate1 (beginning and end tag)
    3. or delete one whole signing section
ghost commented 3 years ago

Yes, I made some hacks in my application: download xml from url, remove all unnecessary tags, and give this content to IdP load_from_xml()

And this works. But, you are active developer, so better to fix these bugs on your side.

Thanks!

timlegge commented 3 years ago

yes, I plan to fix Just wanted to make sure you are working - XML::XPath is a little painful and the problem is that the xpath findvalue is finding all the values not just the key it is working on.

Thanks for logging. It might take a night or two but I will sort it out

ghost commented 3 years ago

Thank you! It's not urgent case. Probably exists an alternative for XML::XPath ? Don't know. But thanks for your work anyway!

timlegge commented 3 years ago

Yea, I prefer XML::libXML::XPathContext

timlegge commented 3 years ago

@aaalexby this sorted the issue I believe. Works now with google which actually had multiple keys in IDPSSODescriptor and microsoft that simply had a lot of Keys

timlegge commented 3 years ago

@aaalexby let me know once you do some testing. I have more testing and cleanup to do

timlegge commented 3 years ago

@aaalexby I finally got some end to end testing against a Google IdP for this fix.

The result is that it requires one change in your application (in mine at least)

the cert in this is required to be an Array ref. In this case it is a file name to the certificate file but some of the other changes required it to be an Arrayref:

            cert =>  [$this->{sp_signing_cert}],

I'm not terribly happy with that solution so I will see if there is a better way around it that does not break existing code.

$redirect = Net::SAML2::Binding::Redirect->new(
                key => $this->{sp_signing_key},
                cert =>  [$this->{sp_signing_cert}],
                param => 'SAMLRequest',
                # The ssl_url destination for redirect
                url => $idp->sso_url('urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect'),
        #certs_as_string => $this->{certs_as_string},
            );
timlegge commented 3 years ago

I sorted the issue in the prior comment - need to take a look to see if it could help in the rest of the commit