cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daan Hoogland <daan.hoogl...@gmail.com>
Subject Re: Review Request 20117: Pilot patch for CS JIRA 6213
Date Mon, 14 Apr 2014 17:06:48 GMT
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.barve@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.hoogland@gmail.com>
> wrote:
>>
>> How about augmenting on loadtime?
>>
>> mobile bilingual spell checker used
>>
>> Op 14 apr. 2014 07:06 schreef "Mandar Barve" <mandar.barve@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.barve@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

Mime
View raw message