[Mondrian] Re: xmla security header processing

Julian Hyde jhyde at pentaho.com
Tue May 24 18:43:52 EDT 2011


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.
 
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.
 
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).
 
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?
 
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?
 
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.
 
5. What does your comment "we'd need to inject the HttpServletRequest into
this method" mean?
 
6. There is a magic id for un-authenticated session. Is this a security
hole?
 
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.)
 
8. I fixed XmlaBasicTest. Maybe other tests are broken also. Can you please
ensure that the tests run clean before you re-submit.
 
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.)
 
10. Is the security model assuming that session ids are hard to guess?

Julian
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.pentaho.org/pipermail/mondrian/attachments/20110524/c4488d16/attachment.html 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jhyde.patch
Type: application/octet-stream
Size: 34688 bytes
Desc: not available
Url : http://lists.pentaho.org/pipermail/mondrian/attachments/20110524/c4488d16/attachment.obj 


More information about the Mondrian mailing list