cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mandar Barve <mandar.ba...@sungardas.com>
Subject Re: Review Request 20117: Pilot patch for CS JIRA 6213
Date Mon, 14 Apr 2014 05:05:59 GMT
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
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message