You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by Bertrand Delacretaz <bd...@apache.org> on 2015/12/29 09:56:59 UTC

How to whitelist legit usages of loginAdministrative?

Hi,

This is about SLING-5135, do people have ideas for identifying which
usages of loginAdministrative are acceptable?

I'll need this for SLING-5355 for example, which creates users and
sets access control.

IMO the proper way to keep track of this is to keep the explanation
why the programmer thinks it's ok to use loginAdministrative in the
code, next to the "get admin session" call.

Here's a suggested pattern that forces the caller to use a specific
wrapper class to get an admin session:

  // This code needs an admin session, for a valid reason
  // which is spelled out in the SlingAdminSession constructor
  Session s = new SlingAdminSession(repository, "setting access
control at Sling startup").getSession();

And we modify the existing loginAdministrative method to fail (when
isDisableLoginAdministrative is true) unless it is called from
SlingAdminSession, detected using
Thread.currentThread().getStackTrace().

This allows for removing all loginAdministrative calls from our code,
and easily checking that with grep.
And also auditing with grep where "new style" admin sessions are used.

We can then use a similar pattern for
ResourceResolverFactory.getAdministrativeResourceResolver(), if we are
still using this deprecated method.

WDYT?

-Bertrand

Re: How to whitelist legit usages of loginAdministrative?

Posted by Robert Munteanu <ro...@apache.org>.
On Wed, 2016-01-06 at 15:06 +0000, Marius Petria wrote:
> 
> 
> 
> 
> On 1/6/16, 4:42 PM, "Bertrand Delacretaz" <bd...@apache.org>
> wrote:
> 
> > 4. Adding sling bundles to the whitelist requires a serious review,
> > we
> > want to keep that very small
> > 
> > There are some legit uses of admin sessions, like SLING-5355.
> > Hiding
> > them behind pseudo service user is not useful IMO.
> > 
> > Does that work for you?
> 
> It works for me, but maybe we should update the jdocs (and also
> remove the @Deprecated annotation) on loginAdmin in [1] as it is not
> entirely correct. Or maybe that is too pedantic :).

Agreed. If we decide that we have scenarios where loginAdministrative
is OK, we should remove the deprecation notice and replace it with a
proper explanation regarding when it is OK to be used.

Robert

> 
> Marius
> 
> [1] https://github.com/apache/sling/blob/trunk/bundles/jcr/api/src/ma
> in/java/org/apache/sling/jcr/api/SlingRepository.java
>

Re: How to whitelist legit usages of loginAdministrative?

Posted by Marius Petria <mp...@adobe.com>.




On 1/6/16, 4:42 PM, "Bertrand Delacretaz" <bd...@apache.org> wrote:

>4. Adding sling bundles to the whitelist requires a serious review, we
>want to keep that very small
>
>There are some legit uses of admin sessions, like SLING-5355. Hiding
>them behind pseudo service user is not useful IMO.
>
>Does that work for you?

It works for me, but maybe we should update the jdocs (and also remove the @Deprecated annotation) on loginAdmin in [1] as it is not entirely correct. Or maybe that is too pedantic :).

Marius

[1] https://github.com/apache/sling/blob/trunk/bundles/jcr/api/src/main/java/org/apache/sling/jcr/api/SlingRepository.java

Re: How to whitelist legit usages of loginAdministrative?

Posted by Bertrand Delacretaz <bd...@apache.org>.
Hi,

On Wed, Jan 6, 2016 at 3:33 PM, Marius Petria <mp...@adobe.com> wrote:
> ...I think the spirit is to force every bundle to analyze its requirements and switch to loginService
> and only exceptionally use and admin session....

That's the idea, yes - here's the scenario as I see it:

1. We implement a mechanism to whitelist which bundles can use admin
sessions, SLING-5135

2. We define our initial whitelist, which should be quite small, and
activate the "admin sessions disabled except for whitelisted bundles"
feature by default.

3. If users have existing code that uses admin session they can add
them to the whitelist, at their own risk. That's very useful to handle
the transition and make sure they enable that blocking feature as
well.

4. Adding sling bundles to the whitelist requires a serious review, we
want to keep that very small

There are some legit uses of admin sessions, like SLING-5355. Hiding
them behind pseudo service user is not useful IMO.

Ideally as discussed in this thread we'd mark the individual lines of
code that create admin sessions to explain why, but if keep bundles
focused a per-bundle whitelist is reasonable.

Does that work for you?

-Bertrand

Re: How to whitelist legit usages of loginAdministrative?

Posted by Marius Petria <mp...@adobe.com>.




On 1/6/16, 3:51 PM, "Bertrand Delacretaz" <bd...@apache.org> wrote:

>Hi,
>
>On Tue, Dec 29, 2015 at 8:28 PM, Marius Petria <mp...@adobe.com> wrote:
>> ....An alternative way to spin this is to actually deprecate loginAdmin and keep the loginService as
>> the only login API for such things. An admin session should be obtain via loginService if the
>> service is mapped to “admin” user. We can have the white list of bundles and services that are
>> allowed to map to “admin" and that can be implemented at validator level....
>
>I think it's useful for existing bundles that use loginAdmin to
>continue to work without changes - in this respect Carsten's solution,
>to whitelist bundles which can do that, makes it easier to manage the
>transition, for existing code.
>
>-Bertrand


I think the spirit is to force every bundle to analyze its requirements and switch to loginService and only exceptionally use and admin session. If there is an easy config option to bypass it I think it will be used. In the end I am OK with using loginAdmin but I wanted to emphasize that it means we will not be able to properly deprecate it from SlingRepository and eventually remove it.

Marius

Re: How to whitelist legit usages of loginAdministrative?

Posted by Bertrand Delacretaz <bd...@apache.org>.
Hi,

On Tue, Dec 29, 2015 at 8:28 PM, Marius Petria <mp...@adobe.com> wrote:
> ....An alternative way to spin this is to actually deprecate loginAdmin and keep the loginService as
> the only login API for such things. An admin session should be obtain via loginService if the
> service is mapped to “admin” user. We can have the white list of bundles and services that are
> allowed to map to “admin" and that can be implemented at validator level....

I think it's useful for existing bundles that use loginAdmin to
continue to work without changes - in this respect Carsten's solution,
to whitelist bundles which can do that, makes it easier to manage the
transition, for existing code.

-Bertrand

Re: How to whitelist legit usages of loginAdministrative?

Posted by Marius Petria <mp...@adobe.com>.




On 12/29/15, 1:24 PM, "Bertrand Delacretaz" <bd...@apache.org> wrote:

>On Tue, Dec 29, 2015 at 11:29 AM, Carsten Ziegeler <cz...@apache.org> wrote:
>...
>>>   If "admin safe" mode is enabled, loginAdmin fails *unless* the code
>>>   that calls is is marked with the reason why it's needed.
>>
>> Don't want to be a pita, but that requirement is not in the issue :)..
>
>I said "IMO" ;-)
>
>Anyway we can use this discussion to clarify that requirement, and
>update the ticket later.


I think that is a general problem with service users and it should not be solved only for “admin sessions”. We could have a way to express the requirements of code, the requirements of an “admin session” is typically jcr:all on root and it is a particular example of this requirements language. @ServiceRequirement(permission=jcr:all, path=/)


>
>>... Why can't we simply use the same concept as for the service users?
>> The caller bundle needs to be in a list of allowed bundles...
>
>If we accept that the granularity is at the bundle level then yes,
>that would work, the SLING-5135 requirement then becomes
>
>>>   If "admin safe" mode is enabled, loginAdmin fails *unless* it's called from
>>>   a bundle that's in the list of allowed bundles.


An alternative way to spin this is to actually deprecate loginAdmin and keep the loginService as the only login API for such things. An admin session should be obtain via loginService if the service is mapped to “admin” user. We can have the white list of bundles and services that are allowed to map to “admin" and that can be implemented at validator level. 

Marius

Re: How to whitelist legit usages of loginAdministrative?

Posted by Carsten Ziegeler <cz...@apache.org>.
Bertrand Delacretaz wrote
> On Tue, Dec 29, 2015 at 11:29 AM, Carsten Ziegeler <cz...@apache.org> wrote:
> ...
>>>   If "admin safe" mode is enabled, loginAdmin fails *unless* the code
>>>   that calls is is marked with the reason why it's needed.
>>
>> Don't want to be a pita, but that requirement is not in the issue :)..
> 
> I said "IMO" ;-)

:)

> 
>> ... Why can't we simply use the same concept as for the service users?
>> The caller bundle needs to be in a list of allowed bundles...
> 
> If we accept that the granularity is at the bundle level then yes,
> that would work, the SLING-5135 requirement then becomes
> 

I think that's easy and sufficient. A bundle doesn't get more insecure
whether it
is doing a single loginAdmin call or several (of course the danger of
wrong code
is higher) - if you have one open door it doesn't matter if you have
more open doors or not.

For legitimate usages we can require a comment similar to Sonar's //
NOSONAR on the same line.

Carsten
 
-- 
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org

Re: How to whitelist legit usages of loginAdministrative?

Posted by Bertrand Delacretaz <bd...@apache.org>.
On Tue, Dec 29, 2015 at 11:29 AM, Carsten Ziegeler <cz...@apache.org> wrote:
...
>>   If "admin safe" mode is enabled, loginAdmin fails *unless* the code
>>   that calls is is marked with the reason why it's needed.
>
> Don't want to be a pita, but that requirement is not in the issue :)..

I said "IMO" ;-)

Anyway we can use this discussion to clarify that requirement, and
update the ticket later.

>... Why can't we simply use the same concept as for the service users?
> The caller bundle needs to be in a list of allowed bundles...

If we accept that the granularity is at the bundle level then yes,
that would work, the SLING-5135 requirement then becomes

>>   If "admin safe" mode is enabled, loginAdmin fails *unless* it's called from
>>   a bundle that's in the list of allowed bundles.

-Bertrand

Re: How to whitelist legit usages of loginAdministrative?

Posted by Carsten Ziegeler <cz...@apache.org>.
Bertrand Delacretaz wrote
> On Tue, Dec 29, 2015 at 10:43 AM, Carsten Ziegeler <cz...@apache.org> wrote:
>> ...If we simply
>> require a comment on the same line as the loginAdmin (just as an
>> example) a simple grep works ootb....
> 
> That's not sufficient, IMO the SLING-5135 requirement is:
> 
>   If "admin safe" mode is enabled, loginAdmin fails *unless* the code
>   that calls is is marked with the reason why it's needed.

Don't want to be a pita, but that requirement is not in the issue :)
Who has the control of what is allowed and what is not?
Is it the caller? then yes it must provide a good reason.
Is it the called service? Then how can it check the reason for being a
good reason?
We should clarify that question first.


Why can't we simply use the same concept as for the service users?
The caller bundle needs to be in a list of allowed bundles.

PS: The below approach adds new api (an annotation) as well :)

Regards
Carsten

> 
> An inline comment works for static code analysis but cannot cause
> existing code to fail, which we want.
> 
> You have a good point about keeping the code compatible with older
> modules - if we don't want any API changes we might use a method
> annotation to indicate the reason:
> 
>   @RequiresLoginAdmin("setting ACL at startup")
>   void doSometing() {
>     Session s = repository.loginAdministrative()
>   }
> 
> We'd need to go up the stack trace to grab the annotation in the
> loginAdmin code, but as we require the direct caller to have the
> annotation it shouldn't be too costly.
> 
> WDYT?
> 
> -Bertrand
> 


 
-- 
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org

Re: How to whitelist legit usages of loginAdministrative?

Posted by Bertrand Delacretaz <bd...@apache.org>.
On Tue, Dec 29, 2015 at 10:43 AM, Carsten Ziegeler <cz...@apache.org> wrote:
> ...If we simply
> require a comment on the same line as the loginAdmin (just as an
> example) a simple grep works ootb....

That's not sufficient, IMO the SLING-5135 requirement is:

  If "admin safe" mode is enabled, loginAdmin fails *unless* the code
  that calls is is marked with the reason why it's needed.

An inline comment works for static code analysis but cannot cause
existing code to fail, which we want.

You have a good point about keeping the code compatible with older
modules - if we don't want any API changes we might use a method
annotation to indicate the reason:

  @RequiresLoginAdmin("setting ACL at startup")
  void doSometing() {
    Session s = repository.loginAdministrative()
  }

We'd need to go up the stack trace to grab the annotation in the
loginAdmin code, but as we require the direct caller to have the
annotation it shouldn't be too costly.

WDYT?

-Bertrand

Re: How to whitelist legit usages of loginAdministrative?

Posted by Carsten Ziegeler <cz...@apache.org>.
I think that adding new api in whatever form is not a good idea: this
makes the code unusable with older api versions and binds it to the
latest and greatest repository api/implementation.
Adding a new api because of a tooling problem (simple grep not working)
while breaking compatibility is something we should avoid. If we simply
require a comment on the same line as the loginAdmin (just as an
example) a simple grep works ootb.

Carsten
-- 
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org

Re: How to whitelist legit usages of loginAdministrative?

Posted by Carsten Ziegeler <cz...@apache.org>.
Bertrand Delacretaz wrote
> Hi,
> 
> On Tue, Dec 29, 2015 at 10:10 AM, Carsten Ziegeler <cz...@apache.org> wrote:
>> I don't understand what makes grepping for SlingAdminSession easier than
>> grepping for loginAdministrative?
> 
> Grepping for loginAdministrative returns all existing legacy
> occurences, you have no way of knowing of they have been validated as
> having good reasons.

Hmm, ok
> 
> Grepping SlingAdminSession returns only calls that have been added
> after introducing this SLING-5135 whitelisting mechanism. And its
> constructor requires providing a reason for getting an admin session,
> so the justification is "local" to the code, no need to search
> elsewhere.
> 
Well, this looks nice, but the reason has no real value, I can put there
"This code needs it", "Workaround" etc. So having this in the API is not
really of value.
But how does SlingAdminSession work? How is it protected from being
called all over the place?

Carsten

 
-- 
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org

Re: How to whitelist legit usages of loginAdministrative?

Posted by Bertrand Delacretaz <bd...@apache.org>.
Hi,

On Tue, Dec 29, 2015 at 10:10 AM, Carsten Ziegeler <cz...@apache.org> wrote:
> I don't understand what makes grepping for SlingAdminSession easier than
> grepping for loginAdministrative?

Grepping for loginAdministrative returns all existing legacy
occurences, you have no way of knowing of they have been validated as
having good reasons.

Grepping SlingAdminSession returns only calls that have been added
after introducing this SLING-5135 whitelisting mechanism. And its
constructor requires providing a reason for getting an admin session,
so the justification is "local" to the code, no need to search
elsewhere.

-Bertrand

Re: How to whitelist legit usages of loginAdministrative?

Posted by Carsten Ziegeler <cz...@apache.org>.
Hi,

I don't understand what makes grepping for SlingAdminSession easier than
grepping for loginAdministrative?

Carsten

Bertrand Delacretaz wrote
> Hi,
> 
> This is about SLING-5135, do people have ideas for identifying which
> usages of loginAdministrative are acceptable?
> 
> I'll need this for SLING-5355 for example, which creates users and
> sets access control.
> 
> IMO the proper way to keep track of this is to keep the explanation
> why the programmer thinks it's ok to use loginAdministrative in the
> code, next to the "get admin session" call.
> 
> Here's a suggested pattern that forces the caller to use a specific
> wrapper class to get an admin session:
> 
>   // This code needs an admin session, for a valid reason
>   // which is spelled out in the SlingAdminSession constructor
>   Session s = new SlingAdminSession(repository, "setting access
> control at Sling startup").getSession();
> 
> And we modify the existing loginAdministrative method to fail (when
> isDisableLoginAdministrative is true) unless it is called from
> SlingAdminSession, detected using
> Thread.currentThread().getStackTrace().
> 
> This allows for removing all loginAdministrative calls from our code,
> and easily checking that with grep.
> And also auditing with grep where "new style" admin sessions are used.
> 
> We can then use a similar pattern for
> ResourceResolverFactory.getAdministrativeResourceResolver(), if we are
> still using this deprecated method.
> 
> WDYT?
> 
> -Bertrand
> 


 
-- 
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org