Daan,Alena, 1. The pilot patch I posted already does this of checking the command level flag first and only for "flagged" commands check the list of "sensitive" parameters. I was wondering if that code itself, building list of sensitive parameters as a command can have more than one of these and then stripping them one by one can lead to additional overhead. To reduce that overhead, I was thinking the REGEX that cleanString util function uses can be generated at application load time walking the list of all only "sensitive" (flagged) command classes followed by the "sensitive" param lists for such classes. At run time when commands are executed all you need to do is for the already flagged "sensitive" commands alone use the pre-built REGEX to filter sensitive data.
2. Yes I can look into Guava and performance tests. Thanks, Mandar On Mon, Apr 14, 2014 at 11:10 PM, Daan Hoogland <daan.hoogl...@gmail.com>wrote: > Guava claims to use the apache 2.0 license on it's site. so that would > not be a problem. > > On Mon, Apr 14, 2014 at 7:33 PM, Alena Prokharchyk > <alena.prokharc...@citrix.com> wrote: > > 1. Agree with Daan. Putting the flag on the command level as a gate would > > eliminate unneeded check for non-sensitive command¹s parameters. > > Especially considering that majority of the commands are not sensitive. > > > > 2. I would really like some performance test to run on a top of the fix, > > Mandar. As a result, I would like to see the difference between ³before > > and after² ways. Also have you considered using Guava CharMatcher > > > https://code.google.com/p/guava-libraries/wiki/StringsExplained#CharMatcher > > ? > > > > Daan, I¹m not really sure if the Guava libraries can be shipped under > > Apache license, please advise on this part. > > > > Thanks, > > Alena. > > > > On 4/14/14, 10:06 AM, "Daan Hoogland" <daan.hoogl...@gmail.com> wrote: > > > >>you got my drift. we could also replace the regex with a tree of flags > >>to search that contains flags or method names. Not sure how that > >>impacts performance. > >> > >>On Mon, Apr 14, 2014 at 11:14 AM, Mandar Barve > >><mandar.ba...@sungardas.com> wrote: > >>> Do you mean at init walk the list of "sensitive" classes somehow, > >>>followed > >>> by "sensitive" Params and build the REGEX to be used at run time? > >>>Basically > >>> split the existing logic into 2 parts? That sounds like a good idea. I > >>>will > >>> need to find out how to do it but sounds doable. > >>> > >>> Thanks, > >>> Mandar > >>> > >>> > >>> On Mon, Apr 14, 2014 at 10:45 AM, Daan Hoogland > >>><daan.hoogl...@gmail.com> > >>> wrote: > >>>> > >>>> How about augmenting on loadtime? > >>>> > >>>> mobile bilingual spell checker used > >>>> > >>>> Op 14 apr. 2014 07:06 schreef "Mandar Barve" > >>>><mandar.ba...@sungardas.com>: > >>>>> > >>>>> This solution for which I have posted a pilot patch has following > >>>>> potential > >>>>> drawbacks: > >>>>> > >>>>> 1. For a sensitive API we need to load all "Param/Parameter" > >>>>>annotations > >>>>> iteratively. This can be time consuming. > >>>>> 2. We also have to iterate multiple times in the cleanString utility > >>>>> function ensuring every identified sensitive keyword is stripped. > >>>>> 3. This adds multiple iterations in the code path for stripping > >>>>>sensitive > >>>>> data. > >>>>> > >>>>> Other potential solution to think about could be: > >>>>> 1. Augment the list of "hard coded" keywords with what we know as the > >>>>> additional sensitive keywords (by carefully going through various > >>>>> response > >>>>> key words, which will be required either ways). Hopefully this won't > >>>>>come > >>>>> out to be too big a list. > >>>>> 2. Device a scheme of tagging sensitive API request/response > >>>>>parameters > >>>>> with a well known prefix or a suffix. The filter REGEX can be > >>>>>augmented > >>>>> further to look for such sub strings. This can remove the need for > >>>>> iterative code. > >>>>> > >>>>> Thanks, > >>>>> Mandar > >>>>> > >>>>> > >>>>> On Tue, Apr 8, 2014 at 1:32 PM, Mandar Barve > >>>>> <mandar.ba...@sungard.com>wrote: > >>>>> > >>>>> > > >>>>> > ----------------------------------------------------------- > >>>>> > This is an automatically generated e-mail. To reply, visit: > >>>>> > https://reviews.apache.org/r/20117/ > >>>>> > ----------------------------------------------------------- > >>>>> > > >>>>> > Review request for cloudstack and Alena Prokharchyk. > >>>>> > > >>>>> > > >>>>> > Repository: cloudstack-git > >>>>> > > >>>>> > > >>>>> > Description > >>>>> > ------- > >>>>> > > >>>>> > Hi Alena, > >>>>> > I am attaching a pilot patch for the problem we are trying to > >>>>> > solve. > >>>>> > Please let me know your thoughts, comments, suggestions on the > >>>>>approach > >>>>> > and > >>>>> > code. We can make widespread code changes after agreeing on the > >>>>> > approach > >>>>> > and any other details. > >>>>> > > >>>>> > Problem: When stripping sensitive parameters from the response log > >>>>> > string, > >>>>> > the strip logic should be generic. Current cleanString code strips > >>>>>few > >>>>> > hardcoded keywords from the response string. > >>>>> > > >>>>> > Approach: As discussed in the context of CS JIRA 4406 I have > >>>>>modified > >>>>> > @Parameter annotation applied to fields of command classes and > >>>>>@Param > >>>>> > annotation applied to fields of response classes. > >>>>> > > >>>>> > 1. Annotations modified to add a new flag that conveys sensitivity > >>>>>of > >>>>> > the > >>>>> > parameter for log, default set to false. > >>>>> > 2. cleanString utility function is modified to process an array of > >>>>> > strings > >>>>> > passed to it so it can strip all. > >>>>> > 3. To keep this backward compatible (and also don't know the code > >>>>> > change > >>>>> > implications at other places at this time) made sure old > cleanString > >>>>> > code > >>>>> > will continue to strip hardcoded keywords when zero sized filter > >>>>>array > >>>>> > is > >>>>> > passed. This can change if we think so and when we think so. This > >>>>> > change I > >>>>> > am putting is minimal focussed to solve current problem. > >>>>> > 4. In ApiServer code where we are loading APICommand annotation to > >>>>> > check > >>>>> > if the command response carried sensitive data, additional code is > >>>>> > added to > >>>>> > load response class signature Param and SerializedName annotations > >>>>>to > >>>>> > get > >>>>> > the name of the field that is flagged to carry sensitive > information > >>>>> > 5. A list of such names is built and passed to cleanString to strip > >>>>> > 6. All code areas that got affected by cleanString signature change > >>>>>are > >>>>> > modified to pass zero sized filter arrays to cleanString > >>>>> > 7. pom.xml is modified for server project to include gson > dependency > >>>>> > 8.StringUtil unit test code modified to use new signature for > >>>>> > cleanString. > >>>>> > (New tests will need to be added in the final patch for testing the > >>>>>new > >>>>> > functions of cleanString) > >>>>> > > >>>>> > On final note this addresses handling the audit logging of response > >>>>> > strings alone. I haven't looked into audit logging of request > >>>>>strings > >>>>> > and > >>>>> > what will need to be done there. > >>>>> > > >>>>> > This pilot patch is tested for listUsers command response. The code > >>>>> > strips > >>>>> > apikey, secretkey and additional parameter userid (only meant for > >>>>> > testing) > >>>>> > as they are tagged to be sensitive. > >>>>> > > >>>>> > Thanks, > >>>>> > Mandar > >>>>> > > >>>>> > > >>>>> > Diffs > >>>>> > ----- > >>>>> > > >>>>> > api/src/com/cloud/serializer/Param.java 3e6f852 > >>>>> > api/src/org/apache/cloudstack/api/Parameter.java 7ee6897 > >>>>> > > >>>>> > > >>>>> > > > >>>>>api/src/org/apache/cloudstack/api/command/user/vm/ResetVMPasswordCmd.ja > >>>>>va > >>>>> > d15ea47 > >>>>> > api/src/org/apache/cloudstack/api/response/UserResponse.java > >>>>>40e1561 > >>>>> > core/src/com/cloud/agent/transport/Request.java b5890d9 > >>>>> > > >>>>> > > >>>>> > > > >>>>>plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/Hyp > >>>>>ervDirectConnectResource.java > >>>>> > 12fc39d > >>>>> > > >>>>> > > >>>>> > > > >>>>>plugins/storage/image/default/src/org/apache/cloudstack/storage/datasto > >>>>>re/lifecycle/CloudStackImageStoreLifeCycleImpl.java > >>>>> > 7675e94 > >>>>> > server/pom.xml 6e60fc4 > >>>>> > server/src/com/cloud/api/ApiServer.java 42ac8b7 > >>>>> > server/src/com/cloud/api/ApiServlet.java e78bf38 > >>>>> > server/src/com/cloud/api/query/dao/ImageStoreJoinDaoImpl.java > >>>>>f1f873c > >>>>> > server/src/com/cloud/api/query/dao/StoragePoolJoinDaoImpl.java > >>>>> > 1d89b19 > >>>>> > utils/src/com/cloud/utils/StringUtils.java 1600488 > >>>>> > utils/test/com/cloud/utils/StringUtilsTest.java 5a90300 > >>>>> > > >>>>> > Diff: https://reviews.apache.org/r/20117/diff/ > >>>>> > > >>>>> > > >>>>> > Testing > >>>>> > ------- > >>>>> > > >>>>> > Tested the strip logic in the pilot patch for listUsers command > >>>>> > response. > >>>>> > > >>>>> > > >>>>> > Thanks, > >>>>> > > >>>>> > Mandar Barve > >>>>> > > >>>>> > > >>> > >>> > >> > >> > >> > >>-- > >>Daan > > > > > > -- > Daan >
