IdentityPython / pyFF

SAML metadata aggregator
https://pyff.io/
Other
50 stars 37 forks source link

Bare alias calls fail due to leading forward-slash in store lookup #105

Closed mrvanes closed 5 years ago

mrvanes commented 7 years ago

If member is a source (alias) MDRepository._lookup sends the member to store._lookup as is, wich turns out to contain a leading forward slash if the member is the only/first part in the query (e.g. /foobar.json). This results in an empty set, because /foobar can not be found in the metadata set as a key, only foobar exists. I've fixed this by having MDRepository._lookup send the store.lookup call using the stripped version of member like this @827 in mdrepo.py

        return self.store.lookup(member.strip("/"))

I have no idea if this is intended functionality or the right place to fix this, but it solves a lot of my problems and questionmarks around the pyFF query syntax.

leifj commented 7 years ago

I'll be happy to review a PR with that fix in it. Also - can you provide an example of how this turns up in deployment that would be very helpful!

mrvanes commented 7 years ago

The deployment is a simple as can be: a git checkout, pip install . in a python virtualenv and then pyffd started on localhost:8080, using an md file containing:

...
- when update:
    - load:
        - edugain_tiny.xml
    - select as coco: '!//md:EntityDescriptor[md:Extensions/mdattr:EntityAttributes/saml:Attribute/saml:AttributeValue="http://www.geant.net/uri/dataprotection-code-of-conduct/v1"]'
    - stats
    - break
...

(edugain_tiny.xml is a (test) subset of edugain to speed up testing)

When I try any request with /coco as first part in path (e.g. http://localhost:8080/coco.xml) all requests fail or result in empty set until I "fix" mdrepo.py.

Without fix:

[29/May/2017:14:01:32]  ROOT default args: ('coco.html',), kwargs: {}
[29/May/2017:14:01:32]  MDServer pfx=None, path=/coco.html, content_type=None
[29/May/2017:14:01:32]  _d(/coco.html,True)
[29/May/2017:14:01:32]  request path: /coco.html, headers: {'Remote-Addr': '127.0.0.1', 'Accept-Language': 'en-US,en;q=0.8,nl;q=0.6', 'Accept-Encoding': 'gzip, deflate, sdch, br', 'Host': 'localhost:8080', 'Accept': 'text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8', 'Upgrade-Insecure-Requests': '1', 'Dnt': '1', 'Connection': 'keep-alive', 'Pragma': 'no-cache', 'Cache-Control': 'no-cache', 'User-Agent': 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36'}
[29/May/2017:14:01:32]  calling store lookup /coco
[29/May/2017:14:01:32]  memory store lookup: /coco
[29/May/2017:14:01:32]  trying main index lookup /coco: 
127.0.0.1 - - [29/May/2017:14:01:32] "GET /coco.html HTTP/1.1" 404 7619 "" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36"

With fix:

[29/May/2017:14:02:07]  ROOT default args: ('coco.html',), kwargs: {}
[29/May/2017:14:02:07]  MDServer pfx=None, path=/coco.html, content_type=None
[29/May/2017:14:02:07]  _d(/coco.html,True)
[29/May/2017:14:02:07]  request path: /coco.html, headers: {'Remote-Addr': '127.0.0.1', 'Accept-Language': 'en-US,en;q=0.8,nl;q=0.6', 'Accept-Encoding': 'gzip, deflate, sdch, br', 'Host': 'localhost:8080', 'Accept': 'text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8', 'Upgrade-Insecure-Requests': '1', 'Dnt': '1', 'Connection': 'keep-alive', 'Pragma': 'no-cache', 'Cache-Control': 'no-cache', 'User-Agent': 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36'}
[29/May/2017:14:02:07]  calling store lookup /coco
[29/May/2017:14:02:07]  memory store lookup: coco
[29/May/2017:14:02:07]  trying main index lookup coco: 
[29/May/2017:14:02:07]  main index: {'coco': ['https://login.aaiedu.hr/edugain/saml2/idp2/metadata.php', 'https://wiki.edugain.org/shibboleth'], 'edugain_tiny.xml': ['https://login.aaiedu.hr/edugain/saml2/idp/metadata.php', 'https://login.aaiedu.hr/edugain/saml2/idp2/metadata.php', 'https://egiswamid.egi.kth.se/shibboleth', 'https://box-idp.nordu.net/simplesaml/module.php/saml/sp/metadata.php/default-sp', 'https://wiki.edugain.org/shibboleth', 'http://test-adfs.geant.net/adfs/services/trust']}: 
[29/May/2017:14:02:07]  entities list coco: ['https://login.aaiedu.hr/edugain/saml2/idp2/metadata.php', 'https://wiki.edugain.org/shibboleth']
127.0.0.1 - - [29/May/2017:14:02:07] "GET /coco.html HTTP/1.1" 200 8047 "" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36"

I'd be more than happy to create a PR, but my problem is that I don't grok the code enough to understand if my fix is the best place to solve this?

My change is as simple as this:

mdrepo.py@827
-        return self.store.lookup(member)
+        return self.store.lookup(member.strip("/"))
leifj commented 7 years ago

The reson I ask for a PR is that this triggers automatic tests etc which is critical to integrating a fix. Also note that the example pipelines all do "select as /foo ..." . I don't know which is more intuitive and I agree that if possible the slash should be optional.

mrvanes commented 7 years ago

I used the inline documentation in builtins.py for my reference. @568 it says:

.. code-block:: yaml

    - select as foo-2.0: "!//md:EntityDescriptor[md:IDPSSODescriptor]""

would allow you to use /foo-2.0.json to refer to the JSON-version of all IdPs in the current repository.

So that explains my frustration ;)

leifj commented 7 years ago

Your frustration is totally understandable. I just want to make sure both /foo and foo work in the select statement.

mrvanes commented 5 years ago

Can be closed as the alias is now correctly documented as being a path with a leading /