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.

Sign up for GitHub

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

Do not crash when entity-selection-profile attribute has invalid JSON #286

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Do not crash when entity-selection-profile attribute has invalid JSON #286

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f733b10
Do not crash when entity-selection-profile attribute has invalid JSON
enriquepablo Nov 27, 2024
b392203
warn when trust info has profiles with same name. Also allow extra me…
enriquepablo Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions src/pyff/samlmd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1047,14 +1047,25 @@ def discojson_sp_attr(e):
if b64_trustinfos is None:
return None

entityID = e.get('entityID', None)
if entityID is None:
return None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would an entry have no entityID?
Isn't this check part of a validation step already, while ingesting input?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you are right, I'll remove that


sp = {}
sp['entityID'] = e.get('entityID', None)
sp['entityID'] = entityID
sp['profiles'] = {}

for b64_trustinfo in b64_trustinfos:
str_trustinfo = b64decode(b64_trustinfo.encode('ascii'))
trustinfo = json.loads(str_trustinfo.decode('utf8'))
sp['profiles'].update(trustinfo['profiles'])
try:
str_trustinfo = b64decode(b64_trustinfo.encode('ascii'))
trustinfo = json.loads(str_trustinfo.decode('utf8'))
sp['profiles'].update(trustinfo['profiles'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that a profile entry can overwrite a previous entry.
Should we have a warning when that happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overwriting a previous entry would be an error in the side of the SP (publishing 2 profiles with the same name), and this warning would be seen by the aggregator, so this would seem to put some responsibility on the aggregator over the correctness of the SP metadata...
I'll add the warning anyway, it can always be ignored.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to put some responsibility on the aggregator over the correctness of the SP metadata

I understand that, but at least the aggregator-operators can be aware and maybe contact the SP-operators to resolve this, instead of hiding the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also the possibility to have trust info in both JSON in an entity attribute and as XML in a TrustInfo element. At this point pyFF will produce discojson_sp with repeated entities, that will be merged by thiss-mdq. So not all equally named trust profiles will be caught here.

Copy link
Member

@c00kiemon5ter c00kiemon5ter Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are issues and questions that should go back to the writers of the specification. They should guide us on the behaviour that is intended when such conflicts occur.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, at one point there was talk of removing the possibility of having trust info in XML, and allowing it only JSON in the entity attribute.
I have addressed both concerns above. I have also added the possibility to have extra_md in JSON trust info.

Copy link

@alexstuart alexstuart Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Ivan, there should be a meeting of the REFEDS spec working group on Thursday, and I'll bring your point up. We have explicitly said in the current draft (version 6):

  • There is only one instance of an Entity Selection Profile entity attribute in an entity. Behaviour when more than one is present is not defined by this profile.
  • Composition with other sources of filtering (such as SeamlessAccess button) is not defined by this profile

so will obviously need to revist those.


except Exception as e:
log.warning(f"Invalid entity-selection-profile attribute for {entityID}: {e}")

if not sp['profiles']:
return None

return sp

Expand Down
112 changes: 112 additions & 0 deletions src/pyff/test/data/metadata/test-sp-trustinfo-in-attr.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,118 @@ fMou5aW0mZ+QgJNKOrxY5vFxUq6pn3OiYbBu3m1C9ajbU/nx2evzt4+qUwTfHFb+
ZgXpOtmxRekFzVvGZ18BSPJKwAAqqZ11X7skT/NwEAhbgplVPv9WkDmDzqNvHqQJ
nyRgD2ZqUPU9nEOjGy0gI07dciVcYZQ+CiZeSECIWgQwjDEBDuwMCVAZA6gfdz6C
KJuN+RUSKPEcxPxle1MiB4MU0ei5X4xUbvLWKn9Ok7TOXg2BpnMAv6eON1wVo0Aa
D265cqy6Le/toVg=</ds:X509Certificate>
</ds:X509Data>
</ds:KeyInfo>
<md:EncryptionMethod Algorithm="http://www.w3.org/2009/xmlenc11#aes128-gcm"/>
<md:EncryptionMethod Algorithm="http://www.w3.org/2009/xmlenc11#aes192-gcm"/>
<md:EncryptionMethod Algorithm="http://www.w3.org/2009/xmlenc11#aes256-gcm"/>
<md:EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#aes128-cbc"/>
<md:EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#aes192-cbc"/>
<md:EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#aes256-cbc"/>
<md:EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#tripledes-cbc"/>
<md:EncryptionMethod Algorithm="http://www.w3.org/2009/xmlenc11#rsa-oaep"/>
<md:EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#rsa-oaep-mgf1p"/>
</md:KeyDescriptor>
<md:AssertionConsumerService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" Location="https://cpauth.icos-cp.eu/saml/SAML2/POST" index="1" isDefault="true"/>
<md:AttributeConsumingService index="1">
<md:ServiceName xml:lang="en">ICOS Carbon Portal SAML service</md:ServiceName>
<md:ServiceName xml:lang="sv">ICOS Kolportalens SAML tjänst</md:ServiceName>
<md:RequestedAttribute FriendlyName="givenName" Name="urn:oid:2.5.4.42" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri" isRequired="true"/>
<md:RequestedAttribute FriendlyName="sn" Name="urn:oid:2.5.4.4" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri" isRequired="true"/>
<md:RequestedAttribute FriendlyName="mail" Name="urn:oid:0.9.2342.19200300.100.1.3" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri" isRequired="true"/>
</md:AttributeConsumingService>
</md:SPSSODescriptor>
<md:Organization>
<md:OrganizationName xml:lang="en">ICOS Carbon Portal</md:OrganizationName>
<md:OrganizationName xml:lang="sv">ICOS Kolportalen</md:OrganizationName>
<md:OrganizationDisplayName xml:lang="en">Carbon Portal</md:OrganizationDisplayName>
<md:OrganizationDisplayName xml:lang="sv">Kolportalen</md:OrganizationDisplayName>
<md:OrganizationURL xml:lang="en">https://www.icos-cp.eu/</md:OrganizationURL>
<md:OrganizationURL xml:lang="sv">https://www.icos-cp.eu/</md:OrganizationURL>
</md:Organization>
<md:ContactPerson contactType="technical">
<md:GivenName>Oleg</md:GivenName>
<md:SurName>Mirzov</md:SurName>
<md:EmailAddress>mailto:[email protected]</md:EmailAddress>
</md:ContactPerson>
<md:ContactPerson contactType="administrative">
<md:GivenName>Alex</md:GivenName>
<md:SurName>Vermeulen</md:SurName>
<md:EmailAddress>mailto:[email protected]</md:EmailAddress>
</md:ContactPerson>
</md:EntityDescriptor>
<md:EntityDescriptor entityID="https://example.org/shibboleth">
<md:Extensions>
<mdrpi:RegistrationInfo registrationAuthority="http://www.swamid.se/" registrationInstant="2015-02-11T11:09:51Z">
<mdrpi:RegistrationPolicy xml:lang="en">http://swamid.se/policy/mdrps</mdrpi:RegistrationPolicy>
</mdrpi:RegistrationInfo>
<alg:DigestMethod Algorithm="http://www.w3.org/2001/04/xmlenc#sha512"/>
<alg:DigestMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#sha384"/>
<alg:DigestMethod Algorithm="http://www.w3.org/2001/04/xmlenc#sha256"/>
<alg:DigestMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#sha224"/>
<alg:DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
<alg:SigningMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha512"/>
<alg:SigningMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha384"/>
<alg:SigningMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha256"/>
<alg:SigningMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha224"/>
<alg:SigningMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha512"/>
<alg:SigningMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha384"/>
<alg:SigningMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"/>
<alg:SigningMethod Algorithm="http://www.w3.org/2009/xmldsig11#dsa-sha256"/>
<alg:SigningMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha1"/>
<alg:SigningMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>
<alg:SigningMethod Algorithm="http://www.w3.org/2000/09/xmldsig#dsa-sha1"/>
<mdattr:EntityAttributes>
<saml:Attribute NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri" Name="http://macedir.org/entity-category">
<saml:AttributeValue>http://www.geant.net/uri/dataprotection-code-of-conduct/v1</saml:AttributeValue>
</saml:Attribute>
<saml:Attribute NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri" Name="https://refeds.org/entity-selection-profile">
<saml:AttributeValue>invalidValueForAttribute</saml:AttributeValue>
</saml:Attribute>
</mdattr:EntityAttributes>
</md:Extensions>
<md:SPSSODescriptor protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
<md:Extensions>
<init:RequestInitiator Binding="urn:oasis:names:tc:SAML:profiles:SSO:request-init" Location="https://cpauth.icos-cp.eu/saml/login"/>
<mdui:UIInfo>
<mdui:DisplayName xml:lang="en">Carbon Portal authentication service</mdui:DisplayName>
<mdui:DisplayName xml:lang="sv">Kolportalens autentiseringstjänst</mdui:DisplayName>
<mdui:Description xml:lang="en">Single Sign On for services of ICOS Carbon Portal. Maintained by the Carbon Portal team at Physical Geography department (nateko.lu.se).</mdui:Description>
<mdui:Description xml:lang="sv">Single Sign On tjänst för ICOS Kolportalen. Hanteras av Carbon Portal teamet på INES (nateko.lu.se).</mdui:Description>
<mdui:PrivacyStatementURL xml:lang="en">https://cpauth.icos-cp.eu/saml/privacyStatement</mdui:PrivacyStatementURL>
<mdui:InformationURL xml:lang="en">https://www.icos-cp.eu/</mdui:InformationURL>
<mdui:InformationURL xml:lang="sv">https://www.icos-cp.eu/</mdui:InformationURL>
<mdui:PrivacyStatementURL xml:lang="sv">https://cpauth.icos-cp.eu/saml/privacyStatement</mdui:PrivacyStatementURL>
</mdui:UIInfo>
</md:Extensions>
<md:KeyDescriptor>
<ds:KeyInfo>
<ds:KeyName>cpauth.icos-cp.eu</ds:KeyName>
<ds:X509Data>
<ds:X509SubjectName>CN=cpauth.icos-cp.eu</ds:X509SubjectName>
<ds:X509Certificate>MIIEJzCCAw+gAwIBAgIJANC3VWNs7fbTMA0GCSqGSIb3DQEBCwUAMIGpMQswCQYD
VQQGEwJTRTERMA8GA1UECAwIU2vDg8KlbmUxDTALBgNVBAcMBEx1bmQxGzAZBgNV
BAoMEklDT1MgQ2FyYm9uIFBvcnRhbDEfMB0GA1UECwwWQXV0aGVudGljYXRpb24g
U2VydmljZTEaMBgGA1UEAwwRY3BhdXRoLmljb3MtY3AuZXUxHjAcBgkqhkiG9w0B
CQEWD2luZm9AaWNvcy1jcC5ldTAeFw0xNTAyMDUxMjI0MzZaFw0yNTAyMDIxMjI0
MzZaMIGpMQswCQYDVQQGEwJTRTERMA8GA1UECAwIU2vDg8KlbmUxDTALBgNVBAcM
BEx1bmQxGzAZBgNVBAoMEklDT1MgQ2FyYm9uIFBvcnRhbDEfMB0GA1UECwwWQXV0
aGVudGljYXRpb24gU2VydmljZTEaMBgGA1UEAwwRY3BhdXRoLmljb3MtY3AuZXUx
HjAcBgkqhkiG9w0BCQEWD2luZm9AaWNvcy1jcC5ldTCCASIwDQYJKoZIhvcNAQEB
BQADggEPADCCAQoCggEBAM2QN1jaZJeuPAH+4sVMZKk7vg4JIbUuTMKk0+KIAg5M
XiVsRiEUjY+LtIncrvA/kf2CIySI0WkbwZMjcDd03hNj4kLWhuyxfOCwDO6DsUbG
MbyI6HIYWXJp5ljfEEFgtMqT3dDtD5vwq8h4Zy20ukxOoIokKczrAvn4JjkMsj6Z
0CEAFBC29o4E8PWQbUBgvt6Z+2ao+RHMLD7nZVBx98Occ9KfnYnDDd9Oi1XFe009
zaSbcqY2RpN8I9hcW/KQf3KnGW5xZ5dr4rhGklCkYr+h0W3xKu+hin8bk91t1Dkr
gaKl/N7M3Oof3k+7ZBlwaV97es5InWCeNgDxCGkBRNsCAwEAAaNQME4wHQYDVR0O
BBYEFDcD7MVudooGaNRYqXBYqQi3VzGxMB8GA1UdIwQYMBaAFDcD7MVudooGaNRY
qXBYqQi3VzGxMAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQELBQADggEBABS02eZS
weXGMJ2fEIy2JH0VhCbjuX/rz+8Hfh9LjzNb3QwKHuwP83yvPqRulV9FYmvOoK8T
fMou5aW0mZ+QgJNKOrxY5vFxUq6pn3OiYbBu3m1C9ajbU/nx2evzt4+qUwTfHFb+
ZgXpOtmxRekFzVvGZ18BSPJKwAAqqZ11X7skT/NwEAhbgplVPv9WkDmDzqNvHqQJ
nyRgD2ZqUPU9nEOjGy0gI07dciVcYZQ+CiZeSECIWgQwjDEBDuwMCVAZA6gfdz6C
KJuN+RUSKPEcxPxle1MiB4MU0ei5X4xUbvLWKn9Ok7TOXg2BpnMAv6eON1wVo0Aa
D265cqy6Le/toVg=</ds:X509Certificate>
</ds:X509Data>
</ds:KeyInfo>
Expand Down
Loading