syncope-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Francesco Chicchiriccò <ilgro...@apache.org>
Subject Re: Some queries on getMetadata in SAML2SPLogic
Date Wed, 16 Aug 2017 11:32:13 GMT
On 16/08/2017 13:06, Colm O hEigeartaigh wrote:
> Maybe we could revert but also explicitly allow the admin role?

Why? The idea is that metadata should be downloaded via the URL managed 
by the agent (e.g. /syncope-console/saml2sp/metadata), not via REST.

Regards.

> On Wed, Aug 16, 2017 at 8:26 AM, Francesco Chicchiriccò <ilgrosso@apache.org>
wrote:
>> Hi,
>> FYI I have now remembered why we used to have
>>
>> @PreAuthorize("hasRole('" + StandardEntitlement.ANONYMOUS + "')")
>>
>> on SAML2SPLogic#getMetadata: the reason was that metadata were supposed to
>> be get through the SAML2SPAgent, which acts as HTTP interface for all SAML
>> 2.0 operations - this is also the reason why downloading metadata via Admin
>> Console used to work even with the entitlement set as above.
>>
>> I wonder if we should revert such change...
>>
>> Regards.
>>
>> On 2017-08-14 18:13, Colm O hEigeartaigh <coheigea@apache.org> wrote:
>>> Excellent, thanks!
>>>
>>> On Mon, Aug 14, 2017 at 4:13 PM, Francesco Chicchiriccò <
>> ilgrosso@apache.org
>>>> wrote:
>>>> Hi,
>>>> I have committed the changes discussed below as
>>>>
>>>> https://github.com/apache/syncope/commit/e99766a441260bb47db
>>>> ac13efe069682dd4a442d
>>>> https://github.com/apache/syncope/commit/f912d90c2aa23c055cf
>>>> b2e143e865f02025154bd
>>>>
>>>> Regards.
>>>>
>>>> On 11/08/2017 18:02, Colm O hEigeartaigh wrote:
>>>>
>>>>> On Fri, Aug 11, 2017 at 2:59 PM, Francesco Chicchiriccò <
>>>>> ilgrosso@apache.org> wrote:
>>>>>
>>>>> Agree. Maybe it should just be changed to
>>>>>> @PreAuthorize("isAuthenticated()")
>>>>>>
>>>>> +1.
>>>>>
>>>>>
>>>>> b) The urlContext not validated at all. For example, you can pass
>> through
>>>>>>> something like  "../../root" which is added to the metadata,
e.g.
>>>>>>> Location="
>>>>>>> http://localhost:9080/syncope/../../root/assertion-consumer".
>>>>>>>
>>>>>>> Should we implement some kind of validation rules on what is
acceptable here?
>>>>>>>
>>>>>>> What do you have in mind here? Just forbid '../'? What could
be the issue(s) with the current implementation?
>>>>>>>
>>>>> Well it makes me a bit uneasy that  a user can essentially insert
>>>>> arbitrary
>>>>> URLs into a metadata document issued by Syncope. I have three suggestions:
>>>>> a) Restrict the acceptable values. Do we need to allow arbitrary contexts
here...?
>>>>> b) Disallow ".." in the values
>>>>> c) Use Commons UrlValidator to make sure that the generated assertion
>>>>> consumer URL is a valid URL.
>>>>>
>>>>> Colm.

-- 
Francesco Chicchiriccò

Tirasa - Open Source Excellence
http://www.tirasa.net/

Member at The Apache Software Foundation
Syncope, Cocoon, Olingo, CXF, OpenJPA, PonyMail
http://home.apache.org/~ilgrosso/


Mime
View raw message