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
