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.java >>> > 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/HypervDirectConnectResource.java >>> > 12fc39d >>> > >>> > >>> > plugins/storage/image/default/src/org/apache/cloudstack/storage/datastore/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
