Sent from my iPhone

Begin forwarded message:

> From: "Julian Hyde" <jhyde (AT) pentaho (DOT) com>
> Date: 21 June 2011 19:23:53 CEST
> To: "'Michele Rossi'" <michele.rossi (AT) gmail (DOT) com>
> Subject: RE: [Mondrian] Re: xmla security header processing
> Reply-To: <jhyde (AT) pentaho (DOT) com>
>


> again, to devs list please.
>
> From: Michele Rossi [mailto:michele.rossi (AT) gmail (DOT) com]
> Sent: Tuesday, June 21, 2011 8:52 AM
> To: jhyde (AT) pentaho (DOT) com
> Subject: Re: [Mondrian] Re: xmla security header processing
>
> hi Julian,
> I've implemented the first easy bit so that we can test whether the packChange process works.
> You can now optionally specify the return values for "discover datasources" in the web.xml configuration. If you do the DISCOVER_DATASOURCES rowset will not require a new OlapConnection.
>
> Could you please let me know if this change is ok and if you can read my packed change?
>
> Tomorrow I will start the more complex bit - the use of commons-dbcp to pool the OlapConnections.
>
>
> thanks,
> Michele
>
>
> On 20 June 2011 21:29, Julian Hyde <jhyde (AT) pentaho (DOT) com> wrote:
> The two should be equivalent. Let's go with the 'unpackChange' approach since it seems to be working.
>
> The only problem is with NonEmptyTest.java. It refused to overwrite a writable file -- which is sensible if you think about it.
>
> I think you should go with the version of NonEmptyTest.java in the .tar.gz file. If you have made changes to NonEmptyTest.java that you want to keep, you will have to manually apply them.
>
> Julian
>
> From: Michele Rossi [mailto:michele.rossi (AT) gmail (DOT) com]
> Sent: Monday, June 20, 2011 8:33 AM
> To: jhyde (AT) pentaho (DOT) com
>
> Subject: Re: [Mondrian] Re: xmla security header processing
>
> hi Julian,
> I've been able to apply the packed change but the patching continues to be elusive.
> The two approaches are equivalent right?
>
> I've included the output of the "unpackChange" and "patch" commands.
>
> Thanks!
> Michele
>
> mrossi@PI-mrossi-L-1 /cygdrive/c/Work/thirdparty/mondrian_perforce/open/mondrian
> $ unpackChange -c 14233 ../../../mondrian_scripts/patches_and_packed_changes/changelist14233.tar.gz
> File(s) not opened on this client.
> //open/mondrian/src/main/mondrian/tui/XmlaSupport.java#27 - file(s) up-to-date.
> Change 14233 belongs to client jhyde.marmite2.
> //open/mondrian/src/main/mondrian/xmla/RowsetDefinition.java#81 - file(s) up-to-date.
> Change 14233 belongs to client jhyde.marmite2.
> //open/mondrian/src/main/mondrian/xmla/XmlaConstants.java#13 - file(s) up-to-date.
> Change 14233 belongs to client jhyde.marmite2.
> //open/mondrian/src/main/mondrian/xmla/XmlaHandler.java#76 - file(s) up-to-date.
> Change 14233 belongs to client jhyde.marmite2.
> //open/mondrian/src/main/mondrian/xmla/XmlaRequest.java#11 - file(s) up-to-date.
> Change 14233 belongs to client jhyde.marmite2.
> //open/mondrian/src/main/mondrian/xmla/XmlaServlet.java#40 - file(s) up-to-date.
> Change 14233 belongs to client jhyde.marmite2.
> //open/mondrian/src/main/mondrian/xmla/XmlaUtil.java#31 - file(s) up-to-date.
> Change 14233 belongs to client jhyde.marmite2.
> //open/mondrian/src/main/mondrian/xmla/impl/DefaultSaxWriter.java#12 - file(s) up-to-date.
> Change 14233 belongs to client jhyde.marmite2.
> //open/mondrian/src/main/mondrian/xmla/impl/DefaultXmlaRequest.java#19 - file(s) up-to-date.
> Change 14233 belongs to client jhyde.marmite2.
> //open/mondrian/src/main/mondrian/xmla/impl/DefaultXmlaServlet.java#30 - file(s) up-to-date.
> Change 14233 belongs to client jhyde.marmite2.
> //open/mondrian/testsrc/main/mondrian/rolap/NonEmptyTest.java#145 - updating C:\Work\thirdparty\mondrian_perforce\open\mondrian\test
> src\main\mondrian\rolap\NonEmptyTest.java
> Can't clobber writable file C:\Work\thirdparty\mondrian_perforce\open\mondrian\testsrc\main\mondrian\rolap\NonEmptyTest.java
> Change 14233 belongs to client jhyde.marmite2.
> //open/mondrian/testsrc/main/mondrian/xmla/test/XmlaTest.java#23 - file(s) up-to-date.
> Change 14233 belongs to client jhyde.marmite2.
> New change number is 14233
>
>
>
>
> mrossi@PI-mrossi-L-1 /cygdrive/c/Work/thirdparty/mondrian_perforce/open/mondrian
> $ patch -p l < ../../../mondrian_scripts/patches_and_packed_changes/jhyde2..patch
> patch: **** strip count l is not a number
>
> mrossi@PI-mrossi-L-1 /cygdrive/c/Work/thirdparty/mondrian_perforce/open/mondrian
> $ patch -p 1 < ../../../mondrian_scripts/patches_and_packed_changes/jhyde2..patch
> patching file src/main/mondrian/tui/XmlaSupport.java
> Reversed (or previously applied) patch detected! Assume -R? [n] Y
> Apply anyway? [n] Y
> Skipping patch.
> 1 out of 1 hunk ignored -- saving rejects to file src/main/mondrian/tui/XmlaSupport.java.rej
> patching file src/main/mondrian/xmla/RowsetDefinition.java
> Reversed (or previously applied) patch detected! Assume -R? [n]
> Apply anyway? [n] y
> Hunk #1 FAILED at 1.
> Hunk #2 FAILED at 41.
> Hunk #3 succeeded at 6436 with fuzz 2 (offset 12 lines).
> 2 out of 3 hunks FAILED -- saving rejects to file src/main/mondrian/xmla/RowsetDefinition.java.rej
> patching file src/main/mondrian/xmla/XmlaConstants.java
> Reversed (or previously applied) patch detected! Assume -R? [n] Y
> Apply anyway? [n] y
> Hunk #1 succeeded at 9 with fuzz 2 (offset -38 lines).
> Hunk #2 FAILED at 26.
> 1 out of 2 hunks FAILED -- saving rejects to file src/main/mondrian/xmla/XmlaConstants.java.rej
> patching file src/main/mondrian/xmla/XmlaHandler.java
> Reversed (or previously applied) patch detected! Assume -R? [n]
>
>
>
> On 16 June 2011 23:11, Julian Hyde <jhyde (AT) pentaho (DOT) com> wrote:
> Two options for the patch file:
>
> 1. The patch file was the wrong format. I fixed it. See attached. Apply it using 'patch -p 1 < jhyde2.patch'.
>
> For this you will need to install cygwin, including the patch command.
>
> OR...
>
> 2. I have also attached a file generated by packChange. See attached. Apply using 'unpackChange changelist14233.tar.gz'.
>
> For this you will need to install cygwin, plus the packChange and unpackChange scripts from http://p4webhost.eigenbase.org:8080/open/util/bin/. Put them somewhere on cygwin's path, e.g. /usr/local/bin, and give them execute access 'chmod 755 ...'.
>
> To check out files for edit, use the user 'considerate_guest'. The password is 'consider8'.
>
> Julian
>
>
>
> From: Michele Rossi [mailto:michele.rossi (AT) gmail (DOT) com]
> Sent: Wednesday, June 15, 2011 6:47 AM
>
> To: jhyde (AT) pentaho (DOT) com; Mondrian developer mailing list
> Subject: Re: [Mondrian] Re: xmla security header processing
>
> hi Julian,
> sorry for the long delay, I've only just started working on this task again.
>
> I have been trying to apply your patch but unfortunately I get an error message, "Invalid patch file".
> I haven't found a way to apply your patch from the "p4" command line tool (I have googled for a considerable amount of time, no luck. I use the p4 eclipse plugin 2010.1/275861, I couldn't do with the March 2011 p4v client either).
>
> Could we consider switching to "perforce shelving" instead of patches?
> Or some other way to allow non-authorised committers to deal with the mondrian perforce in a more efficient way? For example we could create a special branch where everybody can commit too.
> Then you could decide what things you want to merge back to the main branch.
> It's just an idea.. I am obviously not too good with patch files.
>
> Back to the XMLA servlet:
>
> 1. it looks like we need very different connection creation policies: I need to create authenticated connections and I need to associate connections to sessions.
> If it's something that you don't want could we think about allowing the injection of different OlapConnectionFactory implementations?
> That way I could make an OlapConnectionFactory that caches connections and associates them with user sessions while the default behaviour would be what it is now.
>
> 2. I am happy to use the container-level session to store authentication credentials - that is something that you suggested and that I will implement asap
>
> 3. We have a problem with the first request that SimbaO2X sends without authentication.
> The request is "Discover Datasources".
> Are you happy if I allow users to provide their own hard-coded response to such request?
> It could go in the web.xml.
> When such configuration is not hard coded the system would behave as now and would try to obtain it from a OlapConnection object.
>
>
> many thanks,
> Michele
>
>
>
> On 26 May 2011 15:49, Michele Rossi <michele.rossi (AT) gmail (DOT) com> wrote:
> hi Julian,
> I've replied to some of your questions inline.
>
> thanks,
> Michele
>
> On 25 May 2011 00:43, Julian Hyde <jhyde (AT) pentaho (DOT) 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.
>
>
>
>


_______________________________________________
Mondrian mailing list
Mondrian (AT) pentaho (DOT) org
http://lists.pentaho.org/mailman/listinfo/mondrian