Some queries on getMetadata in SAML2SPLogic

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Some queries on getMetadata in SAML2SPLogic

Colm O hEigeartaigh
I have a few minor queries relating to getMetadata in SAML2SPLogic:

 a) You can't get the metadata for a service via the REST API using the
admin credentials due to the logic in SAML2SPLogic, e.g.
@PreAuthorize("hasRole('" + StandardEntitlement.ANONYMOUS + "')")

Should this be changed? It seems a bit odd to get a 403 when just
downloading the metadata using the admin credentials.

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?

Colm.


--
Colm O hEigeartaigh

Talend Community Coder
http://coders.talend.com
Reply | Threaded
Open this post in threaded view
|

Re: Some queries on getMetadata in SAML2SPLogic

ilgrosso
Administrator
On 11/08/2017 15:50, Colm O hEigeartaigh wrote:
> I have a few minor queries relating to getMetadata in SAML2SPLogic:
>
>   a) You can't get the metadata for a service via the REST API using the
> admin credentials due to the logic in SAML2SPLogic, e.g.
> @PreAuthorize("hasRole('" + StandardEntitlement.ANONYMOUS + "')")
>
> Should this be changed? It seems a bit odd to get a 403 when just
> downloading the metadata using the admin credentials.

Agree. Maybe it should just be changed to

@PreAuthorize("isAuthenticated()")

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

Regards.

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

Reply | Threaded
Open this post in threaded view
|

Re: Some queries on getMetadata in SAML2SPLogic

Colm O hEigeartaigh
On Fri, Aug 11, 2017 at 2:59 PM, Francesco Chicchiriccò <[hidden email]
> 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.


>
> Regards.
>
> --
> 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/
>
>


--
Colm O hEigeartaigh

Talend Community Coder
http://coders.talend.com
Reply | Threaded
Open this post in threaded view
|

Re: Some queries on getMetadata in SAML2SPLogic

ilgrosso
Administrator
Hi,
I have committed the changes discussed below as

https://github.com/apache/syncope/commit/e99766a441260bb47dbac13efe069682dd4a442d
https://github.com/apache/syncope/commit/f912d90c2aa23c055cfb2e143e865f02025154bd

Regards.

On 11/08/2017 18:02, Colm O hEigeartaigh wrote:

> On Fri, Aug 11, 2017 at 2:59 PM, Francesco Chicchiriccò <[hidden email]> 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/

Reply | Threaded
Open this post in threaded view
|

Re: Some queries on getMetadata in SAML2SPLogic

Colm O hEigeartaigh
Excellent, thanks!

On Mon, Aug 14, 2017 at 4:13 PM, Francesco Chicchiriccò <[hidden email]
> 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ò <
>> [hidden email]> 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/
>
>


--
Colm O hEigeartaigh

Talend Community Coder
http://coders.talend.com
Reply | Threaded
Open this post in threaded view
|

Re: Some queries on getMetadata in SAML2SPLogic

ilgrosso
Administrator
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 <[hidden email]> wrote:

> Excellent, thanks!
>
> On Mon, Aug 14, 2017 at 4:13 PM, Francesco Chicchiriccò <[hidden email]
> > 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ò <
> >> [hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: Some queries on getMetadata in SAML2SPLogic

Colm O hEigeartaigh
Maybe we could revert but also explicitly allow the admin role?

Colm.

On Wed, Aug 16, 2017 at 8:26 AM, Francesco Chicchiriccò <[hidden email]
> 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 <[hidden email]> wrote:
> > Excellent, thanks!
> >
> > On Mon, Aug 14, 2017 at 4:13 PM, Francesco Chicchiriccò <
> [hidden email]
> > > 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ò <
> > >> [hidden email]> 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.
>



--
Colm O hEigeartaigh

Talend Community Coder
http://coders.talend.com
Reply | Threaded
Open this post in threaded view
|

Re: Some queries on getMetadata in SAML2SPLogic

ilgrosso
Administrator
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ò <[hidden email]> 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 <[hidden email]> wrote:
>>> Excellent, thanks!
>>>
>>> On Mon, Aug 14, 2017 at 4:13 PM, Francesco Chicchiriccò <
>> [hidden email]
>>>> 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ò <
>>>>> [hidden email]> 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/

Reply | Threaded
Open this post in threaded view
|

Re: Some queries on getMetadata in SAML2SPLogic

Colm O hEigeartaigh
No reason, other than it seems odd that you can download the metadata via
REST using the anonymous creds but not the admin creds.

Colm.

On Wed, Aug 16, 2017 at 12:32 PM, Francesco Chicchiriccò <
[hidden email]> wrote:

> 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ò <
>> [hidden email]> 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 <[hidden email]> wrote:
>>>
>>>> Excellent, thanks!
>>>>
>>>> On Mon, Aug 14, 2017 at 4:13 PM, Francesco Chicchiriccò <
>>>>
>>> [hidden email]
>>>
>>>> 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ò <
>>>>>> [hidden email]> 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/
>
>


--
Colm O hEigeartaigh

Talend Community Coder
http://coders.talend.com