You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Mandar Barve <ma...@sungard.com> on 2014/04/08 10:02:06 UTC
Review Request 20117: Pilot patch for CS JIRA 6213
-----------------------------------------------------------
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
Re: Review Request 20117: Pilot patch for CS JIRA 6213
Posted by Daan Hoogland <da...@gmail.com>.
Ok, so you mean generating an or including all names of annotated
methods/params? or do you mean to include package and class names? It
can not work if you don't. A name maybe used different in different
classes.
On Tue, Apr 15, 2014 at 8:25 AM, Mandar Barve
<ma...@sungardas.com> wrote:
> Oh all I meant was stuffing the existing search string that cleanString uses
> with all identified "sensitive" keywords. And I guess if it starts getting
> GIANT it won't make sense.
>
>
>
>
> On Tue, Apr 15, 2014 at 11:36 AM, Daan Hoogland <da...@gmail.com>
> wrote:
>>
>> On Tue, Apr 15, 2014 at 6:01 AM, Mandar Barve
>> <ma...@sungardas.com> wrote:
>> > 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.
>>
>> I am not sure what you mean by this. The REGEX can get quite complex,
>> do you envision to create a giant set of or'ed method names? It can be
>> done but I doubt that building a REGEX is the best thing at load time.
>> At that time more efficient lists or arrays can be build (or a tree
>> :P)
>> Any way tests will tell us.
>>
>> --
>> Daan
>
>
--
Daan
Re: Review Request 20117: Pilot patch for CS JIRA 6213
Posted by Mandar Barve <ma...@sungardas.com>.
Oh all I meant was stuffing the existing search string that cleanString
uses with all identified "sensitive" keywords. And I guess if it starts
getting GIANT it won't make sense.
On Tue, Apr 15, 2014 at 11:36 AM, Daan Hoogland <da...@gmail.com>wrote:
> On Tue, Apr 15, 2014 at 6:01 AM, Mandar Barve
> <ma...@sungardas.com> wrote:
> > 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.
>
> I am not sure what you mean by this. The REGEX can get quite complex,
> do you envision to create a giant set of or'ed method names? It can be
> done but I doubt that building a REGEX is the best thing at load time.
> At that time more efficient lists or arrays can be build (or a tree
> :P)
> Any way tests will tell us.
>
> --
> Daan
>
Re: Review Request 20117: Pilot patch for CS JIRA 6213
Posted by Daan Hoogland <da...@gmail.com>.
On Tue, Apr 15, 2014 at 6:01 AM, Mandar Barve
<ma...@sungardas.com> wrote:
> 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.
I am not sure what you mean by this. The REGEX can get quite complex,
do you envision to create a giant set of or'ed method names? It can be
done but I doubt that building a REGEX is the best thing at load time.
At that time more efficient lists or arrays can be build (or a tree
:P)
Any way tests will tell us.
--
Daan
Re: Review Request 20117: Pilot patch for CS JIRA 6213
Posted by Mandar Barve <ma...@sungardas.com>.
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 <da...@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
> <Al...@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" <da...@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
> >><ma...@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
> >>><da...@gmail.com>
> >>> wrote:
> >>>>
> >>>> How about augmenting on loadtime?
> >>>>
> >>>> mobile bilingual spell checker used
> >>>>
> >>>> Op 14 apr. 2014 07:06 schreef "Mandar Barve"
> >>>><ma...@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
> >>>>> <ma...@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
>
Re: Review Request 20117: Pilot patch for CS JIRA 6213
Posted by Daan Hoogland <da...@gmail.com>.
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
<Al...@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" <da...@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
>><ma...@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
>>><da...@gmail.com>
>>> wrote:
>>>>
>>>> How about augmenting on loadtime?
>>>>
>>>> mobile bilingual spell checker used
>>>>
>>>> Op 14 apr. 2014 07:06 schreef "Mandar Barve"
>>>><ma...@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
>>>>> <ma...@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
Re: Review Request 20117: Pilot patch for CS JIRA 6213
Posted by Alena Prokharchyk <Al...@citrix.com>.
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" <da...@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
><ma...@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
>><da...@gmail.com>
>> wrote:
>>>
>>> How about augmenting on loadtime?
>>>
>>> mobile bilingual spell checker used
>>>
>>> Op 14 apr. 2014 07:06 schreef "Mandar Barve"
>>><ma...@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
>>>> <ma...@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
Re: Review Request 20117: Pilot patch for CS JIRA 6213
Posted by Daan Hoogland <da...@gmail.com>.
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
<ma...@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 <da...@gmail.com>
> wrote:
>>
>> How about augmenting on loadtime?
>>
>> mobile bilingual spell checker used
>>
>> Op 14 apr. 2014 07:06 schreef "Mandar Barve" <ma...@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
>>> <ma...@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
Re: Review Request 20117: Pilot patch for CS JIRA 6213
Posted by Mandar Barve <ma...@sungardas.com>.
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 <da...@gmail.com>wrote:
> How about augmenting on loadtime?
>
> mobile bilingual spell checker used
> Op 14 apr. 2014 07:06 schreef "Mandar Barve" <ma...@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
>> >
>> >
>>
>
Re: Review Request 20117: Pilot patch for CS JIRA 6213
Posted by Daan Hoogland <da...@gmail.com>.
How about augmenting on loadtime?
mobile bilingual spell checker used
Op 14 apr. 2014 07:06 schreef "Mandar Barve" <ma...@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
> >
> >
>
Re: Review Request 20117: Pilot patch for CS JIRA 6213
Posted by Mandar Barve <ma...@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 <ma...@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
>
>