[Mondrian] Re: xmla security header processing

[Michele Rossi]

hi Julian,
I've replied to some of your questions inline.

thanks,
Michele

On 25 May 2011 00:43, Julian Hyde <jhyde at pentaho.com> wrote:

>  Michele,
>
> As promised, I've reviewed your changes.
>
> All in all, the change looks useful. I can see that we need to introduce
> the notion of 'sessions' due to the way Simba O2X sends its requests. But I
> am concerned at how many places username & password are now present in the
> API, and the result is so confusing it's difficult to say that there are not
> security holes.
>
> If you can address my concerns, I will commit your changes. I've changed
> quite a lot of the code; see the patch attached. Can you use that patch as
> starting point when fixing the issues I raise below. And when you have made
> changes, please generate a similar patch using 'p4 diff -du' from the
> command line.
>
> Detailed comments...
>
> 0. Would it be simpler if the servlet looked for the <Security> tag and
> session information and copied these into the HTTP headers? Then we could
> use HTTP athentication and session lifecycle management. Just an idea.
>

My main problem is to create an authenticated OlapConnection.
Hence I need the credentials to be processed in the code that creates the
connection and not by the container.


>
> 1. MondrianServerImpl.getConnection is not used. Removed it.
>
> 2. It is a bad idea to pool connections. If you want to pool connections,
> use a connection pool. It is an even worse idea to pool connections based on
> session ID -- that implies that you are trying to make sessions stateful,
> and it is much better if the XMLA servlet is stateless.
>
> So, I removed ConnectionHolder and the "connections" member.
>

The problem is that Simba will create something like 15 connections before
you can even start configuring a Pivot in Excel.

In general I believe that creating a new connection for each request is  an
anti-pattern as it leads to a substantial waste of resources.

In my case in particular this approach wouldn't work at all as I cache the
metadata objects at the connection level.
So before displaying a Pivot in Excel I'd have to populate the whole
metadata cache N times, once for each connection I create.

Connection pooling is the technique normally used by more traditional web
apps that require access to a database for this problem and it can be
implemented either at the application level (e.g. commons-dbcp) and or at
the container level (tomcat can handle pool of connections to databases) .

I am aware that my implementation didn't clean up un-used connections.
I didn't want to submit too much code at once and risk wasting time if you
didn't like all the rest of the work.


>
> I see why we need to save username & password so that they can be retrieved
> by subsequent requests in the same session. But your implementation never
> removed entries from that table. I added code to remove the entry when we
> see an 'end of session'. It would be nice if we could hook into HTTP session
> management (see #0 above).
>

We can definitely save the credentials in the Http Session. The new version
of Simba supports Cookies too which means that this approach could work.
I will start thinking about this approach.


>
> 3. In removing the "connections" member, I may have broken the mechanism by
> which subsequent requests in the same XMLA session inherit the same
> password. (Difficult to tell without unit tests.) If so, I apologize.
>
> But it is not obvious how session context is conveyed along with the
> XmlaRequest. Would it make sense to replace 'String
> XmlaRequest.getSessionId()' (and getUsername() and getPassword()) with a
> 'SessionInfo XmlaRequest.getSessionInfo()' method that contains all session
> context?
>

Yes definitely - although the end result is very similar this approach is
probably more flexible.


>
> 4. Please convince me that you are not opening up a security hole, as
> follows. The context contains a role name. It also may contain username and
> password, if the client happens to be using basic authentication. Suppose
> that a malicious client specified a valid username and password in its HTTP
> headers, but also specified a role that had greater privileges than the
> username and password allowed. Would that client be able to gain greater
> privileges than it deserved?
>

I don't think the Xmla Servlet supports basic authentication - you could
enable basic authentication at the container level but I really don't know
much about this.
The credentials I am interested in are provided in the SOAP payload by
SimbaO2X, they don't come from HTTP headers.

Also how are clients currently specifying their "role" ?
What stops them from choosing anything they like as their role?
(I will also start doing my own research on these points)


> It needs to be clear and foolproof in the code whether the client is
> trusted. If trusted, it can adopt whichever role it choses. If not, it must
> take the role assigned by olap4j after authentication.
>

I don't fully understand the significance of the role - it's not something I
use in my driver but I know that mondrian uses it. Is it a standard JDBC
concept or something mondrian specific?
I will find out more about it.


> 5. What does your comment "we'd need to inject the HttpServletRequest into
> this method" mean?
>

What I meant was that if we wanted to make the host name that made the
request (IP of the client machine) available in the context, we would need
to have access to the HttpServletRequest.
I need that information for licensing reasons.


>
> 6. There is a magic id for un-authenticated session. Is this a security
> hole?
>

Unfortunately Simba fires off the very first XMLA "DISCOVER" request
(Discover Datasources) without providing any authentication credentials.
It's as if Simba expected the server side to know about its data sources
without opening a database connection.
We could approach this problem in other ways, for example by providing the
list of data sources in a different way.
I will think about it.

In any case an un-authenticated connection is not a security hole per-se.
The behaviour depends on the particular implementation of Olap4j.
Some drivers might refuse to execute queries on OlapConnections created
without credentials.
Other might not need authenticated connections at all.


>
> 7. Would it make sense for the servlet to refuse basic authentication
> requests if they were not over HTTPS? (And not just the first request, which
> carries username/password. Subsequent requests carry an authenticated
> session ID, so they must be protected also.)
>

Again I need to go back and study  the relation between the xmla servlet and
basic authentication as I don't understand it yet.


> 8. I fixed XmlaBasicTest. Maybe other tests are broken also. Can you please
> ensure that the tests run clean before you re-submit.
>

Will do.

>
> 9. Add unit test for basic authentication. (It's hard for me to review this
> code if it is not used in the test suite.)
>

Ok maybe by Basic Authentication you mean the SOAP authentication provided
by Excel / Simba.
I will look into it as discussed above.


>
> 10. Is the security model assuming that session ids are hard to guess?
>

Yes that is always expected by session IDs - you want to prevent an attacker
from guessing a valid ID.
So I can make the session ID generation algo a bit better.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.pentaho.org/pipermail/mondrian/attachments/20110526/ad06e553/attachment.html