CLOUDSTACK-5583: vmopsSnapshot plug-in (XenServer) does not return an error when it should
Hi, I tried to reproduce the issue but couldn't get this to fail for insufficient space. I then injected an exception trying to list files from a non existent path (added this code in the "try" block). This landed me into the exception handling code. It raised correct exception saying "file not found" which was captured in the management server vmops log file. It was not displayed by the GUI. GUI just reported Error (Are we looking for GUI displaying error code?). The plugin code returns "0" immediately after the line of code that raises exception but I think this applies only for successful execution of the plugin code that reverts the snapshot. If any exception is raised (e.g. in the reported case here insufficient space) then the code should return appropriate error message to the caller as I found. In exception handling path return "0" wouldn't execute. I don't see any problem here. Let me know if I am missing anything. Thanks, Mandar
Re: CLOUDSTACK-5583: vmopsSnapshot plug-in (XenServer) does not return an error when it should
I tested this with CS 4.3. Thanks, Mandar On Tue, Mar 4, 2014 at 9:09 PM, Mike Tutkowski wrote: > Hi, > > Can you tell me what release you tested this with? I noticed the problem > while developing on CloudStack 4.3. > > Thanks! > > > On Tue, Mar 4, 2014 at 3:43 AM, Mandar Barve wrote: > >> Hi, >> I tried to reproduce the issue but couldn't get this to fail for >> insufficient space. I then injected an exception trying to list files from >> a non existent path (added this code in the "try" block). This landed me >> into the exception handling code. It raised correct exception saying "file >> not found" which was captured in the management server vmops log file. It >> was not displayed by the GUI. GUI just reported Error (Are we looking for >> GUI displaying error code?). The plugin code returns "0" immediately after >> the line of code that raises exception but I think this applies only for >> successful execution of the plugin code that reverts the snapshot. >> >>If any exception is raised (e.g. in the reported case here >> insufficient space) then the code should return appropriate error message >> to the caller as I found. In exception handling path return "0" wouldn't >> execute. >> >> I don't see any problem here. Let me know if I am missing anything. >> >> Thanks, >> Mandar >> > > > > -- > *Mike Tutkowski* > *Senior CloudStack Developer, SolidFire Inc.* > e: mike.tutkow...@solidfire.com > o: 303.746.7302 > Advancing the way the world uses the > cloud<http://solidfire.com/solution/overview/?video=play> > *(tm)* >
Re: Review Request 16385: Fix for CloudStack JIRA 4406
We surely need a way to make this generic since cleanString looks for specific keywords to filter. I will take a look at this. Using @Parameter may have its own limitations like running through the entire list of parameters per API before deciding which ones to exclude. But let me take a look. I believe we can mark 4406 resolved. Thanks, Mandar On Sat, Mar 8, 2014 at 3:46 AM, Daan Hoogland wrote: > Mandar, you want to take it? > > On Fri, Mar 7, 2014 at 11:12 PM, Alena Prokharchyk > wrote: > > And here is the Jira ticket: > > > > https://issues.apache.org/jira/browse/CLOUDSTACK-6213 > > > > "Add new field to API @Parameter indicating if the param should be > skipped > > from logs" > > > > -Alena. > > > > On 3/7/14, 1:47 PM, "Daan Hoogland" wrote: > > > >>no problem, glad we agree. > >> > >>On Fri, Mar 7, 2014 at 8:38 PM, Alena Prokharchyk > >> wrote: > >>> Ok, got it, somehow missed the "hardcoded" parameters part. In this > case > >>> true is fine as the parameter in @ApiCommand just triggers the > >>>validation. > >>> We only have to fix one part - instead of hardcoding the parameter(s) > to > >>> hide, we have to come up with the new parameter in @Parameter to > trigger > >>> the exclusion from the logs. > >>> > >>> Thank you, > >>> Alena. > >>> > >>> On 3/7/14, 11:32 AM, "Daan Hoogland" wrote: > >>> > Alena, I can see I am not being clear because what you say is the > sensible way and apart from the parameter level exactly what happens. > > The parameter thing is an enhancement that we can make on top of this. > At the moment it only obfuscate a set of parameters with a fixed set > of names. We will have to have a new discussion of what the desirable > default is however. I say security first. but let's not have that > discussion in this thread. > > Hope this clarifies, > Daan > > On Fri, Mar 7, 2014 at 8:21 PM, Alena Prokharchyk > wrote: > > Daan, if the default comes as true for the command, I assume that the > >user > > won¹t see the command logged at all? Unless he overrides it. > > I assume sensitive=³true² means not ³analyze the command² but rather > > ³don¹t log the command². That doesn¹t seem right to me. > > > > True would seem right to me if the parameter is defined on both > > parameter/command level (which is not how it works today). Then > >parameter > > in @ApiCommand annotation will just trigger the analyze for sensitive > > parameters, and the parameter in the @Parameter will tell whether to > >log > > the parameter itself. > > > > > > -Alena. > > > > On 3/7/14, 10:51 AM, "Daan Hoogland" > wrote: > > > >>On Fri, Mar 7, 2014 at 7:31 PM, Alena Prokharchyk > >> wrote: > >>> And the defaults should be false, > >> > >> > >>I don't agree, The true case does nothing if no fields are recognized > >>as sensitive, but it the flase case skips sensitive data containing > >>log messages. The only consquence of true as default is a performance > >>penalty that we were paying in the old case anyhow. > >> > >>-- > >>Daan > > > > > > -- > Daan > >>> > >> > >> > >> > >>-- > >>Daan > > > > > > -- > Daan > >
Re: CLOUDSTACK-5583: vmopsSnapshot plug-in (XenServer) does not return an error when it should
Hi Mike, Did you get a chance to look at this? Thanks, Mandar On Wed, Mar 5, 2014 at 10:12 AM, Mandar Barve wrote: > I tested this with CS 4.3. > > Thanks, > Mandar > > > On Tue, Mar 4, 2014 at 9:09 PM, Mike Tutkowski < > mike.tutkow...@solidfire.com> wrote: > >> Hi, >> >> Can you tell me what release you tested this with? I noticed the problem >> while developing on CloudStack 4.3. >> >> Thanks! >> >> >> On Tue, Mar 4, 2014 at 3:43 AM, Mandar Barve wrote: >> >>> Hi, >>> I tried to reproduce the issue but couldn't get this to fail for >>> insufficient space. I then injected an exception trying to list files from >>> a non existent path (added this code in the "try" block). This landed me >>> into the exception handling code. It raised correct exception saying "file >>> not found" which was captured in the management server vmops log file. It >>> was not displayed by the GUI. GUI just reported Error (Are we looking for >>> GUI displaying error code?). The plugin code returns "0" immediately after >>> the line of code that raises exception but I think this applies only for >>> successful execution of the plugin code that reverts the snapshot. >>> >>>If any exception is raised (e.g. in the reported case here >>> insufficient space) then the code should return appropriate error message >>> to the caller as I found. In exception handling path return "0" wouldn't >>> execute. >>> >>> I don't see any problem here. Let me know if I am missing anything. >>> >>> Thanks, >>> Mandar >>> >> >> >> >> -- >> *Mike Tutkowski* >> *Senior CloudStack Developer, SolidFire Inc.* >> e: mike.tutkow...@solidfire.com >> o: 303.746.7302 >> Advancing the way the world uses the >> cloud<http://solidfire.com/solution/overview/?video=play> >> *(tm)* >> > >
Re: Review Request 16385: Fix for CloudStack JIRA 4406
Guess in addition to the command level flag that we have Parameter walk will need to be done only for the already identified "sensitive" responses as discussed on the thread so this may be fine. Thanks, Mandar On Mon, Mar 10, 2014 at 9:34 AM, Mandar Barve wrote: > We surely need a way to make this generic since cleanString looks for > specific keywords to filter. I will take a look at this. Using @Parameter > may have its own limitations like running through the entire list of > parameters per API before deciding which ones to exclude. But let me take a > look. > > I believe we can mark 4406 resolved. > > Thanks, > Mandar > > > On Sat, Mar 8, 2014 at 3:46 AM, Daan Hoogland wrote: > >> Mandar, you want to take it? >> >> On Fri, Mar 7, 2014 at 11:12 PM, Alena Prokharchyk >> wrote: >> > And here is the Jira ticket: >> > >> > https://issues.apache.org/jira/browse/CLOUDSTACK-6213 >> > >> > "Add new field to API @Parameter indicating if the param should be >> skipped >> > from logs" >> > >> > -Alena. >> > >> > On 3/7/14, 1:47 PM, "Daan Hoogland" wrote: >> > >> >>no problem, glad we agree. >> >> >> >>On Fri, Mar 7, 2014 at 8:38 PM, Alena Prokharchyk >> >> wrote: >> >>> Ok, got it, somehow missed the "hardcoded" parameters part. In this >> case >> >>> true is fine as the parameter in @ApiCommand just triggers the >> >>>validation. >> >>> We only have to fix one part - instead of hardcoding the parameter(s) >> to >> >>> hide, we have to come up with the new parameter in @Parameter to >> trigger >> >>> the exclusion from the logs. >> >>> >> >>> Thank you, >> >>> Alena. >> >>> >> >>> On 3/7/14, 11:32 AM, "Daan Hoogland" wrote: >> >>> >> >>>>Alena, I can see I am not being clear because what you say is the >> >>>>sensible way and apart from the parameter level exactly what happens. >> >>>> >> >>>>The parameter thing is an enhancement that we can make on top of this. >> >>>>At the moment it only obfuscate a set of parameters with a fixed set >> >>>>of names. We will have to have a new discussion of what the desirable >> >>>>default is however. I say security first. but let's not have that >> >>>>discussion in this thread. >> >>>> >> >>>>Hope this clarifies, >> >>>>Daan >> >>>> >> >>>>On Fri, Mar 7, 2014 at 8:21 PM, Alena Prokharchyk >> >>>> wrote: >> >>>>> Daan, if the default comes as true for the command, I assume that >> the >> >>>>>user >> >>>>> won¹t see the command logged at all? Unless he overrides it. >> >>>>> I assume sensitive=³true² means not ³analyze the command² but rather >> >>>>> ³don¹t log the command². That doesn¹t seem right to me. >> >>>>> >> >>>>> True would seem right to me if the parameter is defined on both >> >>>>> parameter/command level (which is not how it works today). Then >> >>>>>parameter >> >>>>> in @ApiCommand annotation will just trigger the analyze for >> sensitive >> >>>>> parameters, and the parameter in the @Parameter will tell whether to >> >>>>>log >> >>>>> the parameter itself. >> >>>>> >> >>>>> >> >>>>> -Alena. >> >>>>> >> >>>>> On 3/7/14, 10:51 AM, "Daan Hoogland" >> wrote: >> >>>>> >> >>>>>>On Fri, Mar 7, 2014 at 7:31 PM, Alena Prokharchyk >> >>>>>> wrote: >> >>>>>>> And the defaults should be false, >> >>>>>> >> >>>>>> >> >>>>>>I don't agree, The true case does nothing if no fields are >> recognized >> >>>>>>as sensitive, but it the flase case skips sensitive data containing >> >>>>>>log messages. The only consquence of true as default is a >> performance >> >>>>>>penalty that we were paying in the old case anyhow. >> >>>>>> >> >>>>>>-- >> >>>>>>Daan >> >>>>> >> >>>> >> >>>> >> >>>> >> >>>>-- >> >>>>Daan >> >>> >> >> >> >> >> >> >> >>-- >> >>Daan >> > >> >> >> >> -- >> Daan >> >> >
Re: CLOUDSTACK-5583: vmopsSnapshot plug-in (XenServer) does not return an error when it should
I tried to reproduce the issue the way you mentioned with few changes. I don't have iSCSI SAN on my setup. I connected a 2 GB disk that I presented as storage tagged NFS primary to CS. I created a 1GB disk offering and then deployed a VM on this new primary. Took a couple of snapshots like you mentioned and when tried to revert to one of them I did see an error in vmops log that said revert_memory_snapshot returned NULL. Exception was thrown with async job status result code 530 and text response as "Failed to revert VM snapshot". I think this exception came later. The vmops snapshot plugin code itself may not have landed into exception handling path. I need to double check this. Is this what you are referring to? Could you attach snippets of SMlog and mops.log when the failure happened to the JIRA? Thanks, Mandar On Tue, Mar 11, 2014 at 3:25 AM, Mike Tutkowski < mike.tutkow...@solidfire.com> wrote: > Here is the comment I just added in JIRA for this ticket. Thanks! > > Hi, > > Here is how I reproduced it: > > I created an iSCSI volume on my SAN that is only 2 GB. > > I created a XenServer SR based on this SAN volume. > > I created Primary Storage in CloudStack based on this XenServer SR. > > I created a Disk Offering that was storage tagged to use this Primary > Storage. It will lead to the creation of a 1 GB volume when executed and > attached to a VM for the first time. > > I executed the Disk Offering to create a CloudStack volume and attached > this volume to a VM. > > I took two hypervisor snapshots of the VM, then reverted to the first > hypervisor snapshot. > > I looked at the SR that should contain my CloudStack volume and its > hypervisor snapshots. I saw two snapshots, but no active VDI. I should see > two hypervisor snapshots and an active VDI. > > Thanks! > > > On Mon, Mar 10, 2014 at 9:27 AM, Mike Tutkowski < > mike.tutkow...@solidfire.com> wrote: > >> I did look at it, but haven't had a chance to try to repo. >> >> I should be able to try to repo it today. >> >> Thanks! >> >> >> On Sun, Mar 9, 2014 at 10:05 PM, Mandar Barve >> wrote: >> >>> Hi Mike, >>> Did you get a chance to look at this? >>> >>> Thanks, >>> Mandar >>> >>> >>> On Wed, Mar 5, 2014 at 10:12 AM, Mandar Barve >>> wrote: >>> >>>> I tested this with CS 4.3. >>>> >>>> Thanks, >>>> Mandar >>>> >>>> >>>> On Tue, Mar 4, 2014 at 9:09 PM, Mike Tutkowski < >>>> mike.tutkow...@solidfire.com> wrote: >>>> >>>>> Hi, >>>>> >>>>> Can you tell me what release you tested this with? I noticed the >>>>> problem while developing on CloudStack 4.3. >>>>> >>>>> Thanks! >>>>> >>>>> >>>>> On Tue, Mar 4, 2014 at 3:43 AM, Mandar Barve >>>> > wrote: >>>>> >>>>>> Hi, >>>>>> I tried to reproduce the issue but couldn't get this to fail >>>>>> for insufficient space. I then injected an exception trying to list files >>>>>> from a non existent path (added this code in the "try" block). This >>>>>> landed >>>>>> me into the exception handling code. It raised correct exception saying >>>>>> "file not found" which was captured in the management server vmops log >>>>>> file. It was not displayed by the GUI. GUI just reported Error (Are we >>>>>> looking for GUI displaying error code?). The plugin code returns "0" >>>>>> immediately after the line of code that raises exception but I think this >>>>>> applies only for successful execution of the plugin code that reverts the >>>>>> snapshot. >>>>>> >>>>>>If any exception is raised (e.g. in the reported case here >>>>>> insufficient space) then the code should return appropriate error message >>>>>> to the caller as I found. In exception handling path return "0" wouldn't >>>>>> execute. >>>>>> >>>>>> I don't see any problem here. Let me know if I am missing anything. >>>>>> >>>>>> Thanks, >>>>>> Mandar >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> *Mike Tutkowski* >>>>> *Senior CloudStack Developer, SolidFire Inc.* >>>>> e: mike.tutkow...@solidfire.com >>>>> o: 303.746.7302 >>>>> Advancing the way the world uses the >>>>> cloud<http://solidfire.com/solution/overview/?video=play> >>>>> *(tm)* >>>>> >>>> >>>> >>> >> >> >> -- >> *Mike Tutkowski* >> *Senior CloudStack Developer, SolidFire Inc.* >> e: mike.tutkow...@solidfire.com >> o: 303.746.7302 >> Advancing the way the world uses the >> cloud<http://solidfire.com/solution/overview/?video=play> >> *(tm)* >> > > > > -- > *Mike Tutkowski* > *Senior CloudStack Developer, SolidFire Inc.* > e: mike.tutkow...@solidfire.com > o: 303.746.7302 > Advancing the way the world uses the > cloud<http://solidfire.com/solution/overview/?video=play> > *(tm)* >
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
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 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
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 wrote: > How about augmenting on loadtime? > > mobile bilingual spell checker used > Op 14 apr. 2014 07:06 schreef "Mandar Barve" : > >> 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 > >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 look
Re: Review Request 20117: Pilot patch for CS JIRA 6213
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 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 > 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" 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 > >> 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 > >>> > >>> wrote: > >>>> > >>>> How about augmenting on loadtime? > >>>> > >>>> mobile bilingual spell checker used > >>>> > >>>> Op 14 apr. 2014 07:06 schreef "Mandar Barve" > >>>>: > >>>>> > >>>>> 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 > >>>>
Re: Review Request 20117: Pilot patch for CS JIRA 6213
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 wrote: > On Tue, Apr 15, 2014 at 6:01 AM, Mandar Barve > 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: CLOUDSTACK-5583: vmopsSnapshot plug-in (XenServer) does not return an error when it should
I followed the steps you mentioned on the bug using a software iSCSI target in a VM and could reproduce the problem. I do see INSUFFICIENT_SPACE exception being thrown when "xe snapshot-revert" is called by the vmopsSnapshot plug in. When this happens the plugin doesn't throw any exception or return any error. The problem looks like this xe command is called via os.system module by the python plugin. xe is a different program and any error/exception thrown by this won't get propagated to the caller. To fix this os.system can be replaced by subprocess.call with a check for the return code. I tried this and this will return a non zero error code to the management server. It may still not return the child process's exception code. Let me know what you think. Thanks, Mandar On Fri, Mar 14, 2014 at 11:55 AM, Mandar Barve wrote: > I tried to reproduce the issue the way you mentioned with few changes. I > don't have iSCSI SAN on my setup. I connected a 2 GB disk that I presented > as storage tagged NFS primary to CS. I created a 1GB disk offering and then > deployed a VM on this new primary. Took a couple of snapshots like you > mentioned and when tried to revert to one of them I did see an error in > vmops log that said revert_memory_snapshot returned NULL. Exception was > thrown with async job status result code 530 and text response as "Failed > to revert VM snapshot". I think this exception came later. The vmops > snapshot plugin code itself may not have landed into exception handling > path. I need to double check this. > > Is this what you are referring to? Could you attach snippets of SMlog and > mops.log when the failure happened to the JIRA? > > Thanks, > Mandar > > > On Tue, Mar 11, 2014 at 3:25 AM, Mike Tutkowski < > mike.tutkow...@solidfire.com> wrote: > >> Here is the comment I just added in JIRA for this ticket. Thanks! >> >> Hi, >> >> Here is how I reproduced it: >> >> I created an iSCSI volume on my SAN that is only 2 GB. >> >> I created a XenServer SR based on this SAN volume. >> >> I created Primary Storage in CloudStack based on this XenServer SR. >> >> I created a Disk Offering that was storage tagged to use this Primary >> Storage. It will lead to the creation of a 1 GB volume when executed and >> attached to a VM for the first time. >> >> I executed the Disk Offering to create a CloudStack volume and attached >> this volume to a VM. >> >> I took two hypervisor snapshots of the VM, then reverted to the first >> hypervisor snapshot. >> >> I looked at the SR that should contain my CloudStack volume and its >> hypervisor snapshots. I saw two snapshots, but no active VDI. I should see >> two hypervisor snapshots and an active VDI. >> >> Thanks! >> >> >> On Mon, Mar 10, 2014 at 9:27 AM, Mike Tutkowski < >> mike.tutkow...@solidfire.com> wrote: >> >>> I did look at it, but haven't had a chance to try to repo. >>> >>> I should be able to try to repo it today. >>> >>> Thanks! >>> >>> >>> On Sun, Mar 9, 2014 at 10:05 PM, Mandar Barve >>> wrote: >>> >>>> Hi Mike, >>>> Did you get a chance to look at this? >>>> >>>> Thanks, >>>> Mandar >>>> >>>> >>>> On Wed, Mar 5, 2014 at 10:12 AM, Mandar Barve >>> > wrote: >>>> >>>>> I tested this with CS 4.3. >>>>> >>>>> Thanks, >>>>> Mandar >>>>> >>>>> >>>>> On Tue, Mar 4, 2014 at 9:09 PM, Mike Tutkowski < >>>>> mike.tutkow...@solidfire.com> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> Can you tell me what release you tested this with? I noticed the >>>>>> problem while developing on CloudStack 4.3. >>>>>> >>>>>> Thanks! >>>>>> >>>>>> >>>>>> On Tue, Mar 4, 2014 at 3:43 AM, Mandar Barve < >>>>>> mandar.ba...@sungard.com> wrote: >>>>>> >>>>>>> Hi, >>>>>>> I tried to reproduce the issue but couldn't get this to fail >>>>>>> for insufficient space. I then injected an exception trying to list >>>>>>> files >>>>>>> from a non existent path (added this code in the "try" block). This >>>>>>> landed >>>>>>> me into the exception handling code.
JIRA 3061
Hi all,
Problem: JIRA says list hosts API response didn't return CPU used parameter
value. This bug is reported against version 4.0.2.
I could not reproduce this problem with CS version 4.2.
I used CloudMonkey CLI to fire API commands to the management server. With
a basic zone created that has 1 pod, cluster and couple of system vms
connected to the management server using CloudMonkey CLI sent the list
hosts API command and the JSON response output could be captured in the log
file. JSON response and the CLI output shows "cpuused". The value seen here
could be matched against the portal reported host statistics value for CPU
used.
CLI output:
> list hosts
count = 1
host:
id = df4fe805-a320-4417-b8be-22dd0b86561e
name = devcloud
capabilities = xen-3.0-x86_32p , hvm
clusterid = b3b80638-1fc5-4d13-aafc-28ff5155c681
clustername = test000
clustertype = CloudManaged
cpuallocated = 0%
cpunumber = 2
cpuspeed = 2486
*cpuused = 0.22%*
cpuwithoverprovisioning = 4972.0
created = 2013-10-07T18:57:58+0530
disconnected = 2013-10-15T11:24:19+0530
events = PingTimeout; AgentConnected; HostDown; ShutdownRequested;
AgentDisconnected; ManagementServerDown; Remove; Ping; StartAgentRebalance
hahost = False
hypervisor = XenServer
ipaddress = 192.168.56.10
islocalstorageactive = False
lastpinged = 1970-01-16T20:20:29+0530
JSON response log:
2013-10-15 11:48:32,724 - requester.py:45 - [DEBUG] START Request
2013-10-15 11:48:32,724 - requester.py:45 - [DEBUG] Requesting
command=listHosts, args={}
2013-10-15 11:48:32,725 - requester.py:45 - [DEBUG] Request sent:
http://localhost:8080/client/api?apiKey=c9uPXphFfiQS5589hVp245hWrqcg1yxcVNg9h1xJES34j8uAtvKj0EP6h8jlSC5_VlajL1a2TaXuYFGoON0DMg&command=listHosts&response=json&signature=hKQ5hI0XFpAzNPJYJ7ivR53%2FzJU%3D
2013-10-15 11:48:32,820 - requester.py:45 - [DEBUG] Response received: {
"listhostsresponse" : { "count":1 ,"host" : [
{"id":"df4fe805-a320-4417-b8be-22dd0b86561e","name":"devcloud","state":"Up","disconnected":"2013-10-15T11:24:19+0530","type":"Routing","ipaddress":"192.168.56.10","zoneid":"7b015b74-f00f-4216-b523-efc2e32c6bc5","zonename":"DevCloud0","podid":"c58e91d0-ad57-4d09-a485-f0decab857b4","podname":"test00","version":"4.2.0","hypervisor":"XenServer","cpunumber":2,"cpuspeed":2486,"cpuallocated":"0%",
*"cpuused":"0.22%"*,"cpuwithoverprovisioning":"4972.0","networkkbsread":57462,"networkkbswrite":38105,"memorytotal":251632,"memoryallocated":0,"memoryused":546428,"capabilities":"xen-3.0-x86_32p
,
hvm","lastpinged":"1970-01-16T20:20:29+0530","managementserverid":8796750265493,"clusterid":"b3b80638-1fc5-4d13-aafc-28ff5155c681","clustername":"test000","clustertype":"CloudManaged","islocalstorageactive":false,"created":"2013-10-07T18:57:58+0530","events":"PingTimeout;
AgentConnected; HostDown; ShutdownRequested; AgentDisconnected;
ManagementServerDown; Remove; Ping;
StartAgentRebalance","resourcestate":"Enabled","hahost":false} ] } }
2013-10-15 11:48:32,821 - requester.py:45 - [DEBUG] END Request
Can this be closed? I have updated the JIRA with same comment.
Thanks,
Mandar
JIRA 285
Hi all, I could not reproduce this issue with vesion 4.0.2. I tried creating hourly snapshot schedule with a keep value of 4. I could see 4 snapshots retained. Then as mentioned in the bug I deleted the schedule and recreated with same parameters except changing the keep value to 3. After this I could see only 3 snapshots retained. Has this been resolved? Can it be closed? I have updated the JIRA with my comments. Thanks, Mandar
Re: JIRA 285
Thanks Daan for your inputs. It looks like I don't have permissions to assign or change bug state. What should I do to get these? Thanks, Mandar On Tue, Nov 26, 2013 at 2:29 PM, Daan Hoogland wrote: > Mandar, > > As you have updated the ticket, the reporter should have been notified > of your findings. I think resolving it is the proper way to go, and if > the reporter doesn't close or reopen it withing a month or so, close > it. > > regards > > On Mon, Nov 25, 2013 at 9:10 AM, Mandar Barve > wrote: > > Hi all, > > I could not reproduce this issue with vesion 4.0.2. I tried creating > > hourly snapshot schedule with a keep value of 4. I could see 4 snapshots > > retained. Then as mentioned in the bug I deleted the schedule and > recreated > > with same parameters except changing the keep value to 3. After this I > > could see only 3 snapshots retained. > > > > Has this been resolved? Can it be closed? I have updated the JIRA with my > > comments. > > > > Thanks, > > Mandar > >
Re: JIRA 3061
Thanks Daan for your inputs. It looks like I don't have permissions to
assign or change bug state. What should I do to get these?
Thanks,
Mandar
On Tue, Nov 26, 2013 at 2:25 PM, Daan Hoogland wrote:
> H Mandar,
>
> You can contact the reporter directly but that would only be to ask to
> verify in the newer version. This can be a hassle for small plants. So
> you could put to resolved with the right fix-version.
>
> regards,
>
>
> On Mon, Nov 25, 2013 at 9:00 AM, Mandar Barve
> wrote:
> > Hi all,
> >
> > Problem: JIRA says list hosts API response didn't return CPU used
> parameter
> > value. This bug is reported against version 4.0.2.
> >
> > I could not reproduce this problem with CS version 4.2.
> >
> > I used CloudMonkey CLI to fire API commands to the management server.
> With
> > a basic zone created that has 1 pod, cluster and couple of system vms
> > connected to the management server using CloudMonkey CLI sent the list
> > hosts API command and the JSON response output could be captured in the
> log
> > file. JSON response and the CLI output shows "cpuused". The value seen
> here
> > could be matched against the portal reported host statistics value for
> CPU
> > used.
> >
> > CLI output:
> >
> >> list hosts
> > count = 1
> > host:
> > id = df4fe805-a320-4417-b8be-22dd0b86561e
> > name = devcloud
> > capabilities = xen-3.0-x86_32p , hvm
> > clusterid = b3b80638-1fc5-4d13-aafc-28ff5155c681
> > clustername = test000
> > clustertype = CloudManaged
> > cpuallocated = 0%
> > cpunumber = 2
> > cpuspeed = 2486
> > *cpuused = 0.22%*
> > cpuwithoverprovisioning = 4972.0
> > created = 2013-10-07T18:57:58+0530
> > disconnected = 2013-10-15T11:24:19+0530
> > events = PingTimeout; AgentConnected; HostDown; ShutdownRequested;
> > AgentDisconnected; ManagementServerDown; Remove; Ping;
> StartAgentRebalance
> > hahost = False
> > hypervisor = XenServer
> > ipaddress = 192.168.56.10
> > islocalstorageactive = False
> > lastpinged = 1970-01-16T20:20:29+0530
> >
> > JSON response log:
> >
> > 2013-10-15 11:48:32,724 - requester.py:45 - [DEBUG] START
> Request
> >
> > 2013-10-15 11:48:32,724 - requester.py:45 - [DEBUG] Requesting
> > command=listHosts, args={}
> > 2013-10-15 11:48:32,725 - requester.py:45 - [DEBUG] Request sent:
> >
> http://localhost:8080/client/api?apiKey=c9uPXphFfiQS5589hVp245hWrqcg1yxcVNg9h1xJES34j8uAtvKj0EP6h8jlSC5_VlajL1a2TaXuYFGoON0DMg&command=listHosts&response=json&signature=hKQ5hI0XFpAzNPJYJ7ivR53%2FzJU%3D
> > 2013-10-15 11:48:32,820 - requester.py:45 - [DEBUG] Response received: {
> > "listhostsresponse" : { "count":1 ,"host" : [
> >
>
> {"id":"df4fe805-a320-4417-b8be-22dd0b86561e","name":"devcloud","state":"Up","disconnected":"2013-10-15T11:24:19+0530","type":"Routing","ipaddress":"192.168.56.10","zoneid":"7b015b74-f00f-4216-b523-efc2e32c6bc5","zonename":"DevCloud0","podid":"c58e91d0-ad57-4d09-a485-f0decab857b4","podname":"test00","version":"4.2.0","hypervisor":"XenServer","cpunumber":2,"cpuspeed":2486,"cpuallocated":"0%",
> >
> *"cpuused":"0.22%"*,"cpuwithoverprovisioning":"4972.0","networkkbsread":57462,"networkkbswrite":38105,"memorytotal":251632,"memoryallocated":0,"memoryused":546428,"capabilities":"xen-3.0-x86_32p
> > ,
> >
> hvm","lastpinged":"1970-01-16T20:20:29+0530","managementserverid":8796750265493,"clusterid":"b3b80638-1fc5-4d13-aafc-28ff5155c681","clustername":"test000","clustertype":"CloudManaged","islocalstorageactive":false,"created":"2013-10-07T18:57:58+0530","events":"PingTimeout;
> > AgentConnected; HostDown; ShutdownRequested; AgentDisconnected;
> > ManagementServerDown; Remove; Ping;
> > StartAgentRebalance","resourcestate":"Enabled","hahost":false} ] } }
> > 2013-10-15 11:48:32,821 - requester.py:45 - [DEBUG] END Request
> >
> >
> > Can this be closed? I have updated the JIRA with same comment.
> >
> > Thanks,
> > Mandar
>
>
Re: JIRA 3061
Works.
Thanks,
Mandar
On Wed, Nov 27, 2013 at 3:13 PM, Daan Hoogland wrote:
> Can you try again, I put you in the contributers group but i am not
> sure if this gives you enough rights
>
> thanks,
> Daan
>
> On Wed, Nov 27, 2013 at 6:56 AM, Mandar Barve
> wrote:
> > Thanks Daan for your inputs. It looks like I don't have permissions to
> > assign or change bug state. What should I do to get these?
> >
> > Thanks,
> > Mandar
> >
> >
> > On Tue, Nov 26, 2013 at 2:25 PM, Daan Hoogland >wrote:
> >
> >> H Mandar,
> >>
> >> You can contact the reporter directly but that would only be to ask to
> >> verify in the newer version. This can be a hassle for small plants. So
> >> you could put to resolved with the right fix-version.
> >>
> >> regards,
> >>
> >>
> >> On Mon, Nov 25, 2013 at 9:00 AM, Mandar Barve >
> >> wrote:
> >> > Hi all,
> >> >
> >> > Problem: JIRA says list hosts API response didn't return CPU used
> >> parameter
> >> > value. This bug is reported against version 4.0.2.
> >> >
> >> > I could not reproduce this problem with CS version 4.2.
> >> >
> >> > I used CloudMonkey CLI to fire API commands to the management server.
> >> With
> >> > a basic zone created that has 1 pod, cluster and couple of system vms
> >> > connected to the management server using CloudMonkey CLI sent the list
> >> > hosts API command and the JSON response output could be captured in
> the
> >> log
> >> > file. JSON response and the CLI output shows "cpuused". The value seen
> >> here
> >> > could be matched against the portal reported host statistics value for
> >> CPU
> >> > used.
> >> >
> >> > CLI output:
> >> >
> >> >> list hosts
> >> > count = 1
> >> > host:
> >> > id = df4fe805-a320-4417-b8be-22dd0b86561e
> >> > name = devcloud
> >> > capabilities = xen-3.0-x86_32p , hvm
> >> > clusterid = b3b80638-1fc5-4d13-aafc-28ff5155c681
> >> > clustername = test000
> >> > clustertype = CloudManaged
> >> > cpuallocated = 0%
> >> > cpunumber = 2
> >> > cpuspeed = 2486
> >> > *cpuused = 0.22%*
> >> > cpuwithoverprovisioning = 4972.0
> >> > created = 2013-10-07T18:57:58+0530
> >> > disconnected = 2013-10-15T11:24:19+0530
> >> > events = PingTimeout; AgentConnected; HostDown; ShutdownRequested;
> >> > AgentDisconnected; ManagementServerDown; Remove; Ping;
> >> StartAgentRebalance
> >> > hahost = False
> >> > hypervisor = XenServer
> >> > ipaddress = 192.168.56.10
> >> > islocalstorageactive = False
> >> > lastpinged = 1970-01-16T20:20:29+0530
> >> >
> >> > JSON response log:
> >> >
> >> > 2013-10-15 11:48:32,724 - requester.py:45 - [DEBUG] START
> >> Request
> >> >
> >> > 2013-10-15 11:48:32,724 - requester.py:45 - [DEBUG] Requesting
> >> > command=listHosts, args={}
> >> > 2013-10-15 11:48:32,725 - requester.py:45 - [DEBUG] Request sent:
> >> >
> >>
> http://localhost:8080/client/api?apiKey=c9uPXphFfiQS5589hVp245hWrqcg1yxcVNg9h1xJES34j8uAtvKj0EP6h8jlSC5_VlajL1a2TaXuYFGoON0DMg&command=listHosts&response=json&signature=hKQ5hI0XFpAzNPJYJ7ivR53%2FzJU%3D
> >> > 2013-10-15 11:48:32,820 - requester.py:45 - [DEBUG] Response
> received: {
> >> > "listhostsresponse" : { "count":1 ,"host" : [
> >> >
> >>
>
> {"id":"df4fe805-a320-4417-b8be-22dd0b86561e","name":"devcloud","state":"Up","disconnected":"2013-10-15T11:24:19+0530","type":"Routing","ipaddress":"192.168.56.10","zoneid":"7b015b74-f00f-4216-b523-efc2e32c6bc5","zonename":"DevCloud0","podid":"c58e91d0-ad57-4d09-a485-f0decab857b4","podname":"test00","version":"4.2.0","hypervisor":"XenServer","cpunumber":2,"cpuspeed":2486,"cpuallocated":"0%",
> >> >
> >>
> *"cpuused":"0.22%"*,"cpuwithoverprovisioning":"4972.0","networkkbsread":57462,"networkkbswrite":38105,"memorytotal":251632,"memoryallocated":0,"memoryused":546428,"capabilities":"xen-3.0-x86_32p
> >> > ,
> >> >
> >>
> hvm","lastpinged":"1970-01-16T20:20:29+0530","managementserverid":8796750265493,"clusterid":"b3b80638-1fc5-4d13-aafc-28ff5155c681","clustername":"test000","clustertype":"CloudManaged","islocalstorageactive":false,"created":"2013-10-07T18:57:58+0530","events":"PingTimeout;
> >> > AgentConnected; HostDown; ShutdownRequested; AgentDisconnected;
> >> > ManagementServerDown; Remove; Ping;
> >> > StartAgentRebalance","resourcestate":"Enabled","hahost":false} ] } }
> >> > 2013-10-15 11:48:32,821 - requester.py:45 - [DEBUG] END
> Request
> >> >
> >> >
> >> > Can this be closed? I have updated the JIRA with same comment.
> >> >
> >> > Thanks,
> >> > Mandar
> >>
> >>
>
>
[PROPOSAL] JIRA 4406: Remove cleanString() call for every API to improve performance - especially of the list APIs
Hi All, JIRA 4406 expects removal of cleanString() call for performance improvements. This is called when building audit trail for command responses and used for removing sensitive data (passwords, secret keys) from the log buffer. All the API responses do not carry such sensitive information so pattern matching done by cleanString against all API response strings can be costly. I propose following for a solution: Approach #1 * Check for all the cmds that carry sensitive data each time the logging function is called to strip sensitive information * Pros: Easy to implement, with little code impact * Cons: Won't scale well since any new additional command will mean code modification required where checks are made Approach #2 * Modify BaseCmd class to add flags that will store cmd/response sensitivity * At init these flags will be set to false indicating no cmd req/resp carries sensitive data * any child api cmd class that will carry sensitive data in the req/resp should set the respective flags * before calling any logging function the flag should be checked and cleanString should be called only for cmds with flags set Pro: This approach will scale well as new cmds get added and no additional changes should be required. Con: Big change upfront as it will touch all API cmd classes that carry sensitive information along with BaseCmd class. NOTE: changes should be simple and straightforward though spread across multiple classes. In my opinion we should implement approach #2. I have tested this approach for couple of sensitive commands list Users and list Accounts. It looks to work fine. Please let me know if you have concerns, suggestions. Thanks, Mandar
Re: [PROPOSAL] JIRA 4406: Remove cleanString() call for every API to improve performance - especially of the list APIs
Daan, The child classes will "know" (since the API request/response parameters are known) at load time if they are to carry sensitive data and they can set the flags at class load time/construction like you mentioned. Flag check will be at the time of logging. Did you mean to suggest the same or am I missing something? Thanks, Mandar On Thu, Nov 28, 2013 at 5:22 PM, Daan Hoogland wrote: > On Thu, Nov 28, 2013 at 10:38 AM, Mandar Barve > wrote: > > > > In my opinion we should implement approach #2. I have tested this > approach > > for couple of sensitive commands list Users and list Accounts. It looks > to > > work fine. > > > H Mandar, I agree but can some automation be done i.e. recognize > sensitive names of fields [pP]assword or varieties thereof? This can > be done at construction or even classloadtime and shouldn't impact > performance very much. > > otherwise #2 is more elegant and i mean this as an extension of #2 > >
Re: [PROPOSAL] JIRA 4406: Remove cleanString() call for every API to improve performance - especially of the list APIs
Hi Daan, I see your point in not bothering API implementers when it comes to checking or setting the flag. (In my implementation I am proposing flag check using base class ref and set in API child class. Flags and get/set methods are members of the base class.) If the base class has to do the check and set at load time as you suggest here are some things to consider: 1. API command class is loaded by its name at run time. 2. At this time code doesn't know if the API command is carrying sensitive data or not. 3. I did not find any annotation in the API command classes for return types that indicate password/secret key that could be used as a simple match string to help decide sensitivity of the class at load time, which means class needs to be modified to set such flag perhaps in its constructor or when it executes the command as suggested earlier. In a nutshell API command class modification is needed. I also agree with your second point that with introduction of another member variable and expectation of it being set by the child class, if there is a developer oversight purpose can get lost. I think by making a base class wrapper method e.g. InformCommandSensitivity abstract we should be able to enforce this at compile time. All API command classes will have to implement this method leading to setting/resetting of the flag as needed. Like you said I don't know the future security implications if any for such code. Considering all these I would like to go ahead with the approach I suggested. Thanks, Mandar On Thu, Nov 28, 2013 at 8:07 PM, Daan Hoogland wrote: > Almost, I would like to see some construct that would allow for child > classes not to bother and let the baseclass do the checking and set > the flag at load time so api implemeters don't have to worry about > this. It is kind of a challenge but using reflection it should be > possible. > > As I understand your proposal every implementer of an api has to make > the consideration considering every field of the class. It will work > but automated processing of the api class could prevent oversights by > developers. > > On the other hand we will run into privacy and security related > situations not foreseeable right now. I am not sure if what I am > saying will pay off. > > > > On Thu, Nov 28, 2013 at 3:24 PM, Mandar Barve > wrote: > > Daan, > > The child classes will "know" (since the API request/response > > parameters are known) at load time if they are to carry sensitive data > and > > they can set the flags at class load time/construction like you > mentioned. > > Flag check will be at the time of logging. > > > > Did you mean to suggest the same or am I missing something? > > > > Thanks, > > Mandar > > > > > > On Thu, Nov 28, 2013 at 5:22 PM, Daan Hoogland >wrote: > > > >> On Thu, Nov 28, 2013 at 10:38 AM, Mandar Barve < > mandar.ba...@sungard.com> > >> wrote: > >> > > >> > In my opinion we should implement approach #2. I have tested this > >> approach > >> > for couple of sensitive commands list Users and list Accounts. It > looks > >> to > >> > work fine. > >> > >> > >> H Mandar, I agree but can some automation be done i.e. recognize > >> sensitive names of fields [pP]assword or varieties thereof? This can > >> be done at construction or even classloadtime and shouldn't impact > >> performance very much. > >> > >> otherwise #2 is more elegant and i mean this as an extension of #2 > >> > >> > >
Re: [PROPOSAL] JIRA 4406: Remove cleanString() call for every API to improve performance - especially of the list APIs
Thanks Daan. Thanks, Mandar On Fri, Nov 29, 2013 at 5:16 PM, Daan Hoogland wrote: > Mandar, > > I agree, go ahead. Let me know if you need any help. > > On Fri, Nov 29, 2013 at 12:40 PM, Mandar Barve > wrote: > > Hi Daan, > >I see your point in not bothering API implementers when it comes to > > checking or setting the flag. (In my implementation I am proposing flag > > check using base class ref and set in API child class. Flags and get/set > > methods are members of the base class.) > > > > If the base class has to do the check and set at load time as you suggest > > here are some things to consider: > > > > 1. API command class is loaded by its name at run time. > > 2. At this time code doesn't know if the API command is carrying > sensitive > > data or not. > > 3. I did not find any annotation in the API command classes for return > > types that indicate password/secret key that could be used as a simple > > match string to help decide sensitivity of the class at load time, which > > means class needs to be modified to set such flag perhaps in its > > constructor or when it executes the command as suggested earlier. In a > > nutshell API command class modification is needed. > > > > I also agree with your second point that with introduction of another > > member variable and expectation of it being set by the child class, if > > there is a developer oversight purpose can get lost. I think by making a > > base class wrapper method e.g. InformCommandSensitivity abstract we > should > > be able to enforce this at compile time. All API command classes will > have > > to implement this method leading to setting/resetting of the flag as > needed. > > > > Like you said I don't know the future security implications if any for > such > > code. > > > > Considering all these I would like to go ahead with the approach I > > suggested. > > > > Thanks, > > Mandar > > > > > > On Thu, Nov 28, 2013 at 8:07 PM, Daan Hoogland >wrote: > > > >> Almost, I would like to see some construct that would allow for child > >> classes not to bother and let the baseclass do the checking and set > >> the flag at load time so api implemeters don't have to worry about > >> this. It is kind of a challenge but using reflection it should be > >> possible. > >> > >> As I understand your proposal every implementer of an api has to make > >> the consideration considering every field of the class. It will work > >> but automated processing of the api class could prevent oversights by > >> developers. > >> > >> On the other hand we will run into privacy and security related > >> situations not foreseeable right now. I am not sure if what I am > >> saying will pay off. > >> > >> > >> > >> On Thu, Nov 28, 2013 at 3:24 PM, Mandar Barve > > >> wrote: > >> > Daan, > >> > The child classes will "know" (since the API request/response > >> > parameters are known) at load time if they are to carry sensitive data > >> and > >> > they can set the flags at class load time/construction like you > >> mentioned. > >> > Flag check will be at the time of logging. > >> > > >> > Did you mean to suggest the same or am I missing something? > >> > > >> > Thanks, > >> > Mandar > >> > > >> > > >> > On Thu, Nov 28, 2013 at 5:22 PM, Daan Hoogland < > daan.hoogl...@gmail.com > >> >wrote: > >> > > >> >> On Thu, Nov 28, 2013 at 10:38 AM, Mandar Barve < > >> mandar.ba...@sungard.com> > >> >> wrote: > >> >> > > >> >> > In my opinion we should implement approach #2. I have tested this > >> >> approach > >> >> > for couple of sensitive commands list Users and list Accounts. It > >> looks > >> >> to > >> >> > work fine. > >> >> > >> >> > >> >> H Mandar, I agree but can some automation be done i.e. recognize > >> >> sensitive names of fields [pP]assword or varieties thereof? This can > >> >> be done at construction or even classloadtime and shouldn't impact > >> >> performance very much. > >> >> > >> >> otherwise #2 is more elegant and i mean this as an extension of #2 > >> >> > >> >> > >> > >> > >
Review Request 16385: Fix for CloudStack JIRA 4406
/AddBigSwitchVnsDeviceCmd.java a30059d plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/DeleteBigSwitchVnsDeviceCmd.java 4af45b2 plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/ListBigSwitchVnsDevicesCmd.java 6e4ee75 plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/api/command/CreateServiceInstanceCmd.java 50457d8 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/AddNiciraNvpDeviceCmd.java 7842d37 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/DeleteNiciraNvpDeviceCmd.java 374b0fe plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/ListNiciraNvpDeviceNetworksCmd.java 6d2dc05 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/ListNiciraNvpDevicesCmd.java 78b2ad8 plugins/network-elements/palo-alto/src/com/cloud/api/commands/AddPaloAltoFirewallCmd.java 7aba9c2 plugins/network-elements/palo-alto/src/com/cloud/api/commands/ConfigurePaloAltoFirewallCmd.java 80f02ad plugins/network-elements/palo-alto/src/com/cloud/api/commands/DeletePaloAltoFirewallCmd.java 4f147eb plugins/network-elements/palo-alto/src/com/cloud/api/commands/ListPaloAltoFirewallNetworksCmd.java d1b7425 plugins/network-elements/palo-alto/src/com/cloud/api/commands/ListPaloAltoFirewallsCmd.java ad4be72 plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/api/commands/AddSspCmd.java 085f873 plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/api/commands/DeleteSspCmd.java e23f642 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPConfigCmd.java db6d7dd plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPRemoveCmd.java 535a545 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapAddConfigurationCmd.java 5686374 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java 100ffe6 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapDeleteConfigurationCmd.java b45bce5 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java 89cec65 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListConfigurationCmd.java b50970f plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListUsersCmd.java 5c65ac4 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapUserSearchCmd.java e2b050d server/src/com/cloud/api/ApiServer.java 03361a4 server/test/com/cloud/api/ApiDispatcherTest.java 7314a57 Diff: https://reviews.apache.org/r/16385/diff/ Testing --- Using CloudMonkey following commands have been tested to make sure secret key/password is stripped from the response list users list accounts list virtualmachines create user update user create sshkeypair Thanks, Mandar Barve
Re: Review Request 16385: Fix for CloudStack JIRA 4406
> On Dec. 19, 2013, 3:58 p.m., daan Hoogland wrote: > > api/src/org/apache/cloudstack/api/BaseCmd.java, line 427 > > <https://reviews.apache.org/r/16385/diff/1/?file=400859#file400859line427> > > > > please make sure a clear and extensive javadoc is present on why and > > how this abstract method should be implemented by api devs. Hi Daan, I couldn't find a suitable predefined annotation that could be added for abstract methods or methods in general. I also didn't find any annotated methods as reference. Here is what I could do 1. Add a new annotation type e.g. Implementation that has name, description, implementation, usage properties and won't be visible in API doc by default but will be available at RUNTIME 2. I will apply this annotation to the new method as follow: @Implementation(name="cmdHandlesCriticalData", description="cmdHandlesCriticalData method must be implemented for all APIs. This method declares if i t handles requests and/or responses that carry sensitive data such as passwords, secret keys.", implementation= "Method implementation should call cmdReqIsCritical and/or cmdRespIsCritical based on if the API carries such sensitive information in its request and/or response. If command doesn't carry any sensitive infor mation then this method's implementation can be empty and method need not be called.", usage="If API does handle sensitive data then this method should be called either from the API command constructor or before processing the command from the execute method") Please let me know what you think. Thanks, Mandar - Mandar --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16385/#review30699 --- On Dec. 19, 2013, 1:45 p.m., Mandar Barve wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16385/ > --- > > (Updated Dec. 19, 2013, 1:45 p.m.) > > > Review request for cloudstack and daan Hoogland. > > > Bugs: CLOUDSTACK-4406 > https://issues.apache.org/jira/browse/CLOUDSTACK-4406 > > > Repository: cloudstack-git > > > Description > --- > > JIRA 4406 expects removal of cleanString() call for performance > improvements. This is called when building audit trail for command responses > and used for removing sensitive data (passwords, secret keys) from the log > buffer. All the API responses do not carry such sensitive information so > pattern matching done by cleanString against all API response strings can be > costly. > > I propose following for a solution: > > * Modify BaseCmd class to add flags that will store cmd/response sensitivity > * At init these flags will be set to false indicating no cmd req/resp carries > sensitive data > * any child api cmd class that will carry sensitive data in the req/resp > should set the respective flags > * before calling any logging function the flag should be checked and > cleanString should be called only for cmds with flags set > > Pro: This approach will scale well as new cmds get added and no additional > changes should be required. > Con: Big change upfront as it will touch all API cmd classes that carry > sensitive information along with BaseCmd class. > > NOTE: changes should be simple and straightforward though spread across > multiple classes. > > > Diffs > - > > api/src/com/cloud/api/commands/ListRecurringSnapshotScheduleCmd.java > d34c09c > api/src/org/apache/cloudstack/api/BaseCmd.java 0cfb950 > api/src/org/apache/cloudstack/api/BaseListTemplateOrIsoPermissionsCmd.java > 48c1e02 > > api/src/org/apache/cloudstack/api/command/admin/account/CreateAccountCmd.java > c5a2d1a > > api/src/org/apache/cloudstack/api/command/admin/account/DeleteAccountCmd.java > 7c1b206 > > api/src/org/apache/cloudstack/api/command/admin/account/DisableAccountCmd.java > 6fdbefe > > api/src/org/apache/cloudstack/api/command/admin/account/EnableAccountCmd.java > 59d6acd > api/src/org/apache/cloudstack/api/command/admin/account/LockAccountCmd.java > 93ec1be > > api/src/org/apache/cloudstack/api/command/admin/account/UpdateAccountCmd.java > a8cf63f > api/src/org/apache/cloudstack/api/command/admin/alert/GenerateAlertCmd.java > 620c5ed > > api/src/org/apache/cloudstack/api/command/admin/autoscale/CreateCounterCmd.java > 6c4b81b >
Re: Review Request 16385: Fix for CloudStack JIRA 4406
Sounds good. I will post updated patch. Thanks, Mandar On Mon, Dec 23, 2013 at 8:14 PM, Daan Hoogland wrote: > H Mandar, > > why not just put > > /** > * cmdHandlesCriticalData method must be implemented for all APIs. > This method declares if it handles requests and/or responses that carry > sensitive data such as passwords, secret keys. > * Method implementation should call cmdReqIsCritical and/or > cmdRespIsCritical based on if the API carries such sensitive information in > its request and/or response. If command doesn't carry any sensitive > information then this method's implementation can be empty and method need > not be called. > * If API does handle sensitive data then this method should be called > either from the API command constructor or before processing the command > from the execute method > */ > abstract public void cmdHandlesCriticalData(); > > in BaseCmd.java? > > regards, > Daan > > > On Mon, Dec 23, 2013 at 12:05 PM, Mandar Barve > wrote: > >>This is an automatically generated e-mail. To reply, visit: >> https://reviews.apache.org/r/16385/ >> >> On December 19th, 2013, 3:58 p.m. UTC, *daan Hoogland* wrote: >> >> >> api/src/org/apache/cloudstack/api/BaseCmd.java<https://reviews.apache.org/r/16385/diff/1/?file=400859#file400859line427> >> (Diff >> revision 1) >> >> 427 >> >> abstract public void cmdHandlesCriticalData(); >> >> please make sure a clear and extensive javadoc is present on why and how >> this abstract method should be implemented by api devs. >> >> Hi Daan, >> I couldn't find a suitable predefined annotation that could be added for >> abstract methods or methods in general. I also didn't find any annotated >> methods as reference. Here is what I could do >> 1. Add a new annotation type e.g. Implementation that has name, description, >> implementation, usage properties and won't be visible in API doc by default >> but will be available at RUNTIME >> 2. I will apply this annotation to the new method as follow: >> @Implementation(name="cmdHandlesCriticalData", >> description="cmdHandlesCriticalData method must be >> implemented for all APIs. This method declares if i >> t handles requests and/or responses that carry sensitive data such as >> passwords, secret keys.", >> implementation= "Method implementation should call >> cmdReqIsCritical and/or cmdRespIsCritical based on >> if the API carries such sensitive information in its request and/or >> response. If command doesn't carry any sensitive infor >> mation then this method's implementation can be empty and method need not be >> called.", >> usage="If API does handle sensitive data then this >> method should be called either from the API command >> constructor or before processing the command from the execute method") >> >> Please let me know what you think. >> >> Thanks, >> Mandar >> >> >> - Mandar >> >> On December 19th, 2013, 1:45 p.m. UTC, Mandar Barve wrote: >> Review request for cloudstack and daan Hoogland. >> By Mandar Barve. >> >> *Updated Dec. 19, 2013, 1:45 p.m.* >> *Bugs: * >> CLOUDSTACK-4406<https://issues.apache.org/jira/browse/CLOUDSTACK-4406> >> *Repository: * cloudstack-git >> Description >> >> JIRA 4406 expects removal of cleanString() call for performance >> improvements. This is called when building audit trail for command responses >> and used for removing sensitive data (passwords, secret keys) from the log >> buffer. All the API responses do not carry such sensitive information so >> pattern matching done by cleanString against all API response strings can be >> costly. >> >> I propose following for a solution: >> >> * Modify BaseCmd class to add flags that will store cmd/response sensitivity >> * At init these flags will be set to false indicating no cmd req/resp >> carries sensitive data >> * any child api cmd class that will carry sensitive data in the req/resp >> should set the respective flags >> * before calling any logging function the flag should be checked and >> cleanString should be called only for cmds with flags set >> >> Pro: This approach will scale well as new cmds get added and no additional >> changes should be required. >> Con: Big change upfront as it will touch all API cmd clas
Re: Review Request 16385: Fix for CloudStack JIRA 4406
/ListUcsProfileCmd.java cc49cef plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/AddBigSwitchVnsDeviceCmd.java a30059d plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/DeleteBigSwitchVnsDeviceCmd.java 4af45b2 plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/ListBigSwitchVnsDevicesCmd.java 6e4ee75 plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/api/command/CreateServiceInstanceCmd.java 50457d8 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/AddNiciraNvpDeviceCmd.java 7842d37 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/DeleteNiciraNvpDeviceCmd.java 374b0fe plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/ListNiciraNvpDeviceNetworksCmd.java 6d2dc05 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/ListNiciraNvpDevicesCmd.java 78b2ad8 plugins/network-elements/palo-alto/src/com/cloud/api/commands/AddPaloAltoFirewallCmd.java 7aba9c2 plugins/network-elements/palo-alto/src/com/cloud/api/commands/ConfigurePaloAltoFirewallCmd.java 80f02ad plugins/network-elements/palo-alto/src/com/cloud/api/commands/DeletePaloAltoFirewallCmd.java 4f147eb plugins/network-elements/palo-alto/src/com/cloud/api/commands/ListPaloAltoFirewallNetworksCmd.java d1b7425 plugins/network-elements/palo-alto/src/com/cloud/api/commands/ListPaloAltoFirewallsCmd.java ad4be72 plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/api/commands/AddSspCmd.java 085f873 plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/api/commands/DeleteSspCmd.java e23f642 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPConfigCmd.java db6d7dd plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPRemoveCmd.java 535a545 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapAddConfigurationCmd.java 5686374 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java 100ffe6 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapDeleteConfigurationCmd.java b45bce5 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java 89cec65 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListConfigurationCmd.java b50970f plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListUsersCmd.java 5c65ac4 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapUserSearchCmd.java e2b050d server/src/com/cloud/api/ApiServer.java 03361a4 server/test/com/cloud/api/ApiDispatcherTest.java 7314a57 Diff: https://reviews.apache.org/r/16385/diff/ Testing --- Using CloudMonkey following commands have been tested to make sure secret key/password is stripped from the response list users list accounts list virtualmachines create user update user create sshkeypair Thanks, Mandar Barve
Re: Review Request 16385: Fix for CloudStack JIRA 4406
cc49cef plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/AddBigSwitchVnsDeviceCmd.java a30059d plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/DeleteBigSwitchVnsDeviceCmd.java 4af45b2 plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/ListBigSwitchVnsDevicesCmd.java 6e4ee75 plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/api/command/CreateServiceInstanceCmd.java 50457d8 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/AddNiciraNvpDeviceCmd.java 7842d37 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/DeleteNiciraNvpDeviceCmd.java 374b0fe plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/ListNiciraNvpDeviceNetworksCmd.java 6d2dc05 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/ListNiciraNvpDevicesCmd.java 78b2ad8 plugins/network-elements/palo-alto/src/com/cloud/api/commands/AddPaloAltoFirewallCmd.java 7aba9c2 plugins/network-elements/palo-alto/src/com/cloud/api/commands/ConfigurePaloAltoFirewallCmd.java 80f02ad plugins/network-elements/palo-alto/src/com/cloud/api/commands/DeletePaloAltoFirewallCmd.java 4f147eb plugins/network-elements/palo-alto/src/com/cloud/api/commands/ListPaloAltoFirewallNetworksCmd.java d1b7425 plugins/network-elements/palo-alto/src/com/cloud/api/commands/ListPaloAltoFirewallsCmd.java ad4be72 plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/api/commands/AddSspCmd.java 085f873 plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/api/commands/DeleteSspCmd.java e23f642 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPConfigCmd.java db6d7dd plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPRemoveCmd.java 535a545 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapAddConfigurationCmd.java 5686374 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java 100ffe6 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapDeleteConfigurationCmd.java b45bce5 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java 89cec65 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListConfigurationCmd.java b50970f plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListUsersCmd.java 5c65ac4 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapUserSearchCmd.java e2b050d server/src/com/cloud/api/ApiServer.java 03361a4 server/test/com/cloud/api/ApiDispatcherTest.java 7314a57 Diff: https://reviews.apache.org/r/16385/diff/ Testing --- Using CloudMonkey following commands have been tested to make sure secret key/password is stripped from the response list users list accounts list virtualmachines create user update user create sshkeypair Thanks, Mandar Barve
Re: Review Request 16385: Fix for CloudStack JIRA 4406
> On Dec. 23, 2013, 5:58 p.m., Nitin Mehta wrote: > > api/src/org/apache/cloudstack/api/BaseCmd.java, line 415 > > <https://reviews.apache.org/r/16385/diff/1/?file=400859#file400859line415> > > > > Can you please create names which are more intuitive such as > > cmdRequestContainsSensitiveInfo and also better names for getters and > > setters ? Nitin, I wanted to keep names short at the same time convey adequate meaning hence I chose those names. But I see your point, I could create following names. Here the thought is to have intuitive names plus try to follow getter/setter existing naming convention. Let me know if you have concerns. Member variables can be named as: responseHasSensitiveInfo requestHasSensitiveInfo The getter/setters can be named as: getRequestHasSensitiveInfo setRequestHasSensitiveInfo getResponseHasSensitiveInfo setResponseHasSensitiveInfo Thanks, Mandar > On Dec. 23, 2013, 5:58 p.m., Nitin Mehta wrote: > > api/src/org/apache/cloudstack/api/BaseListTemplateOrIsoPermissionsCmd.java, > > line 53 > > <https://reviews.apache.org/r/16385/diff/1/?file=400860#file400860line53> > > > > You shouldn't have to override for every cmd. By default its false and > > the cmds having sensitive information can have methods returning true. Also > > they do not need to be set in execute. This is static information, doesn't > > change per command so why this needs to be set ? Nitin, You are right. This was discussed in the earlier discussion thread. You should really have to modify only commands that carry sensitive information. The problem with that approach as stated earlier is API developer can forget to declare command/response sensitivity by implementing a method that sets the flags, returns true etc. The wrapper abstract method was introduced essentially to ensure new APIs as they get introduced will give compiler error if this wrapper is not implemented enforcing the developer to declare such sensitivity upfront. Hope that addresses your concern. Thanks, Mandar - Mandar --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16385/#review30828 --- On Dec. 23, 2013, 6:13 p.m., Mandar Barve wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16385/ > --- > > (Updated Dec. 23, 2013, 6:13 p.m.) > > > Review request for cloudstack and daan Hoogland. > > > Bugs: CLOUDSTACK-4406 > https://issues.apache.org/jira/browse/CLOUDSTACK-4406 > > > Repository: cloudstack-git > > > Description > --- > > JIRA 4406 expects removal of cleanString() call for performance > improvements. This is called when building audit trail for command responses > and used for removing sensitive data (passwords, secret keys) from the log > buffer. All the API responses do not carry such sensitive information so > pattern matching done by cleanString against all API response strings can be > costly. > > I propose following for a solution: > > * Modify BaseCmd class to add flags that will store cmd/response sensitivity > * At init these flags will be set to false indicating no cmd req/resp carries > sensitive data > * any child api cmd class that will carry sensitive data in the req/resp > should set the respective flags > * before calling any logging function the flag should be checked and > cleanString should be called only for cmds with flags set > > Pro: This approach will scale well as new cmds get added and no additional > changes should be required. > Con: Big change upfront as it will touch all API cmd classes that carry > sensitive information along with BaseCmd class. > > NOTE: changes should be simple and straightforward though spread across > multiple classes. > > > Diffs > - > > api/src/com/cloud/api/commands/ListRecurringSnapshotScheduleCmd.java > d34c09c > api/src/org/apache/cloudstack/api/BaseCmd.java 0cfb950 > api/src/org/apache/cloudstack/api/BaseListTemplateOrIsoPermissionsCmd.java > 48c1e02 > > api/src/org/apache/cloudstack/api/command/admin/account/CreateAccountCmd.java > c5a2d1a > > api/src/org/apache/cloudstack/api/command/admin/account/DeleteAccountCmd.java > 7c1b206 > > api/src/org/apache/cloudstack/api/command/admin/account/DisableAccountCmd.java > 6fdbefe > > api/src/org/apache/cloudstack/api/command/admin/account/EnableAccountCmd.java > 59d6acd > api/sr
Re: Review Request 16385: Fix for CloudStack JIRA 4406
> On Dec. 23, 2013, 5:58 p.m., Nitin Mehta wrote: > > api/src/org/apache/cloudstack/api/BaseListTemplateOrIsoPermissionsCmd.java, > > line 53 > > <https://reviews.apache.org/r/16385/diff/1/?file=400860#file400860line53> > > > > You shouldn't have to override for every cmd. By default its false and > > the cmds having sensitive information can have methods returning true. Also > > they do not need to be set in execute. This is static information, doesn't > > change per command so why this needs to be set ? > > Mandar Barve wrote: > Nitin, > You are right. This was discussed in the earlier discussion thread. > You should really have to modify only commands that carry sensitive > information. The problem with that approach as stated earlier is API > developer can forget to declare command/response sensitivity by implementing > a method that sets the flags, returns true etc. The wrapper abstract method > was introduced essentially to ensure new APIs as they get introduced will > give compiler error if this wrapper is not implemented enforcing the > developer to declare such sensitivity upfront. > Hope that addresses your concern. > > Thanks, > Mandar > > Nitin Mehta wrote: > Thanks Mandar. I see your point and was thinking on the same lines as > well. I appreciate your thinking for future API devs. But I have the > following concerns > 1. I probably think that this information should be static for the Cmd > class and doesnt have to be set on every execute invocation > 2. For few commands having sensitive information we are writing > boilerplate code in all the api's, this is not en elegant way of enforcing > every API developer to look into this. I would rather want this to be dealt > through an annotation (if it doesnt exist lets create one in the public > @interface APICommand and keep the default value to true that it contains > sensitive information) Nitin, I see us going back to PROPOSAL discussion which is fine but IMO its happening little late. I am new to this process of development in CloudStack and would want to take this opportunity to understand how this thing works. As I understood it I tried to: - reproduce and understand the issue, come up with a solution, - ran a PoC making sure the proposed solution will work, will scale etc. - Put down a proposal providing multiple solution approaches discussing pros/cons and shared with the team inviting comments. - Addressed all the concerns related to the proposal until I saw no more concerns raised over this. - went through an entire exercise of manually changing each command file carefully going through API doc with the proposed change. I truly appreciate all the comments and also understand sometimes important things may need to be addressed even if they are late. Is there any norm in the community to close a "PROPOSAL/DISCUSS" phase? Are we supposed to get "VOTE" on the proposed solution before moving to implementation? This didn't look like the case for every discussion from my reading of wiki. Now coming to your comments on the PROPOSAL. You are suggesting making declarative changes (static)to API Commands e.g. to APICommand annotation or a new annotation. Something like this can surely make the change look more elegant in the sense the change itself will potentially be limited to one/two lines per file (ensuring all annotations for all commands are changed to the new one) and won't need a call from execute. The checking code will need to load the annotation to check the flag status in the annotation meaning a reflective code. Daan had earlier proposed using reflection with string match but also had raised security concerns over using reflection. Leaving that aside, to ensure every API does its job of declaring sensitivity upfront we should really be able to enforce it at compile time like mentioned before. I don't see a way to enforce annotation implementation by all sub classes at compile time. IF such method doesn't exist then we will be leaving use of this annotat ion to the mercy of the API developer who can forget to do so. In such case your default true values can come into play but then essentially losing the whole purpose where a command that is not sensitive will still need to go through a cleanString call. Assuming we apply this annotation to all known API commands to date close to 437 files will need to change and that is truly a boiler plate change. If we rely on using default "false" e.g. and modify only sensitive classes then also it can come to around 50 files or little more I believe with a hole left open where newly added commands can go without annotation with unintended results as mentioned above.
Re: Review Request 16385: Fix for CloudStack JIRA 4406
Daan, I don't get the idea behind making the methods static. Making getter/setters static will lead to instance vars losing their meaning and we need each instance to let us know its sensitivity. I assume you are not suggesting changing the abstract method into static. Can you please explain? Thanks, Mandar On Wed, Jan 8, 2014 at 8:24 PM, daan Hoogland wrote: > > > > On Dec. 23, 2013, 5:58 p.m., Nitin Mehta wrote: > > > > api/src/org/apache/cloudstack/api/BaseListTemplateOrIsoPermissionsCmd.java, > line 53 > > > < > https://reviews.apache.org/r/16385/diff/1/?file=400860#file400860line53> > > > > > > You shouldn't have to override for every cmd. By default its false > and the cmds having sensitive information can have methods returning true. > Also they do not need to be set in execute. This is static information, > doesn't change per command so why this needs to be set ? > > > > Mandar Barve wrote: > > Nitin, > > You are right. This was discussed in the earlier discussion > thread. You should really have to modify only commands that carry sensitive > information. The problem with that approach as stated earlier is API > developer can forget to declare command/response sensitivity by > implementing a method that sets the flags, returns true etc. The wrapper > abstract method was introduced essentially to ensure new APIs as they get > introduced will give compiler error if this wrapper is not implemented > enforcing the developer to declare such sensitivity upfront. > > Hope that addresses your concern. > > > > Thanks, > > Mandar > > > > Nitin Mehta wrote: > > Thanks Mandar. I see your point and was thinking on the same lines > as well. I appreciate your thinking for future API devs. But I have the > following concerns > > 1. I probably think that this information should be static for the > Cmd class and doesnt have to be set on every execute invocation > > 2. For few commands having sensitive information we are writing > boilerplate code in all the api's, this is not en elegant way of enforcing > every API developer to look into this. I would rather want this to be dealt > through an annotation (if it doesnt exist lets create one in the public > @interface APICommand and keep the default value to true that it contains > sensitive information) > > > > Mandar Barve wrote: > > Nitin, > > I see us going back to PROPOSAL discussion which is fine but > IMO its happening little late. > > > > I am new to this process of development in CloudStack and would > want to take this opportunity to understand how this thing works. As I > understood it I tried to: > > - reproduce and understand the issue, come up with a solution, > > - ran a PoC making sure the proposed solution will work, will scale > etc. > > - Put down a proposal providing multiple solution approaches > discussing pros/cons and shared with the team inviting comments. > > - Addressed all the concerns related to the proposal until I saw no > more concerns raised over this. > > - went through an entire exercise of manually changing each command > file carefully going through API doc with the proposed change. > > > > I truly appreciate all the comments and also understand sometimes > important things may need to be addressed even if they are late. Is there > any norm in the community to close a "PROPOSAL/DISCUSS" phase? Are we > supposed to get "VOTE" on the proposed solution before moving to > implementation? This didn't look like the case for every discussion from my > reading of wiki. > > > > Now coming to your comments on the PROPOSAL. You are suggesting > making declarative changes (static)to API Commands e.g. to APICommand > annotation or a new annotation. Something like this can surely make the > change look more elegant in the sense the change itself will potentially be > limited to one/two lines per file (ensuring all annotations for all > commands are changed to the new one) and won't need a call from execute. > The checking code will need to load the annotation to check the flag status > in the annotation meaning a reflective code. Daan had earlier proposed > using reflection with string match but also had raised security concerns > over using reflection. Leaving that aside, to ensure every API does its job > of declaring sensitivity upfront we should really be able to enforce it at > compile time like mentioned before. I don't see a way to enforce annotation > implementation by all sub classes at compile time. IF such method doesn
Re: Review Request 16385: Fix for CloudStack JIRA 4406
Daan, I have been busy with few other things. Will need to get back to this and update, hopefully before EoW. Thanks, Mandar On Fri, Jan 31, 2014 at 3:03 PM, daan Hoogland wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16385/#review33306 > --- > > > Mandar, still feel like picking this up. As Nithin didn't reply anymore I > will test and commit if you update. > > - daan Hoogland > > > On Dec. 23, 2013, 6:13 p.m., Mandar Barve wrote: > > > > --- > > This is an automatically generated e-mail. To reply, visit: > > https://reviews.apache.org/r/16385/ > > --- > > > > (Updated Dec. 23, 2013, 6:13 p.m.) > > > > > > Review request for cloudstack and daan Hoogland. > > > > > > Bugs: CLOUDSTACK-4406 > > https://issues.apache.org/jira/browse/CLOUDSTACK-4406 > > > > > > Repository: cloudstack-git > > > > > > Description > > --- > > > > JIRA 4406 expects removal of cleanString() call for performance > improvements. This is called when building audit trail for command > responses and used for removing sensitive data (passwords, secret keys) > from the log buffer. All the API responses do not carry such sensitive > information so pattern matching done by cleanString against all API > response strings can be costly. > > > > I propose following for a solution: > > > > * Modify BaseCmd class to add flags that will store cmd/response > sensitivity > > * At init these flags will be set to false indicating no cmd req/resp > carries sensitive data > > * any child api cmd class that will carry sensitive data in the req/resp > should set the respective flags > > * before calling any logging function the flag should be checked and > cleanString should be called only for cmds with flags set > > > > Pro: This approach will scale well as new cmds get added and no > additional changes should be required. > > Con: Big change upfront as it will touch all API cmd classes that carry > sensitive information along with BaseCmd class. > > > > NOTE: changes should be simple and straightforward though spread across > multiple classes. > > > > > > Diffs > > - > > > > api/src/com/cloud/api/commands/ListRecurringSnapshotScheduleCmd.java > d34c09c > > api/src/org/apache/cloudstack/api/BaseCmd.java 0cfb950 > > > api/src/org/apache/cloudstack/api/BaseListTemplateOrIsoPermissionsCmd.java > 48c1e02 > > > api/src/org/apache/cloudstack/api/command/admin/account/CreateAccountCmd.java > c5a2d1a > > > api/src/org/apache/cloudstack/api/command/admin/account/DeleteAccountCmd.java > 7c1b206 > > > api/src/org/apache/cloudstack/api/command/admin/account/DisableAccountCmd.java > 6fdbefe > > > api/src/org/apache/cloudstack/api/command/admin/account/EnableAccountCmd.java > 59d6acd > > > api/src/org/apache/cloudstack/api/command/admin/account/LockAccountCmd.java > 93ec1be > > > api/src/org/apache/cloudstack/api/command/admin/account/UpdateAccountCmd.java > a8cf63f > > > api/src/org/apache/cloudstack/api/command/admin/alert/GenerateAlertCmd.java > 620c5ed > > > api/src/org/apache/cloudstack/api/command/admin/autoscale/CreateCounterCmd.java > 6c4b81b > > > api/src/org/apache/cloudstack/api/command/admin/autoscale/DeleteCounterCmd.java > 50477f5 > > > api/src/org/apache/cloudstack/api/command/admin/cluster/AddClusterCmd.java > d0e7380 > > > api/src/org/apache/cloudstack/api/command/admin/cluster/DeleteClusterCmd.java > e1bc585 > > > api/src/org/apache/cloudstack/api/command/admin/cluster/ListClustersCmd.java > 8640f37 > > > api/src/org/apache/cloudstack/api/command/admin/cluster/UpdateClusterCmd.java > b13f81a > > > api/src/org/apache/cloudstack/api/command/admin/config/ListCfgsByCmd.java > 517807d > > > api/src/org/apache/cloudstack/api/command/admin/config/ListDeploymentPlannersCmd.java > 1d9d2d9 > > > api/src/org/apache/cloudstack/api/command/admin/config/ListHypervisorCapabilitiesCmd.java > 16adf66 > > > api/src/org/apache/cloudstack/api/command/admin/config/UpdateCfgCmd.java > 9bc9b3c > > > api/src/org/apache/cloudstack/api/command/admin/config/UpdateHypervisorCapabilitiesCmd.java > 5cb5f9c > > > api/src/org/apache/cloudstack/api/command/admin/domain/CreateDomainCmd.java > 4737555 &
Re: Review Request 16385: Fix for CloudStack JIRA 4406
/ListUcsBladeCmd.java 41c7cc1 plugins/hypervisors/ucs/src/org/apache/cloudstack/api/ListUcsManagerCmd.java 767682f plugins/hypervisors/ucs/src/org/apache/cloudstack/api/ListUcsProfileCmd.java cc49cef plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/AddBigSwitchVnsDeviceCmd.java a30059d plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/DeleteBigSwitchVnsDeviceCmd.java 4af45b2 plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/ListBigSwitchVnsDevicesCmd.java 6e4ee75 plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/api/command/CreateServiceInstanceCmd.java 50457d8 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/AddNiciraNvpDeviceCmd.java 7842d37 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/DeleteNiciraNvpDeviceCmd.java 374b0fe plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/ListNiciraNvpDeviceNetworksCmd.java 6d2dc05 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/ListNiciraNvpDevicesCmd.java 78b2ad8 plugins/network-elements/palo-alto/src/com/cloud/api/commands/AddPaloAltoFirewallCmd.java 7aba9c2 plugins/network-elements/palo-alto/src/com/cloud/api/commands/ConfigurePaloAltoFirewallCmd.java 80f02ad plugins/network-elements/palo-alto/src/com/cloud/api/commands/DeletePaloAltoFirewallCmd.java 4f147eb plugins/network-elements/palo-alto/src/com/cloud/api/commands/ListPaloAltoFirewallNetworksCmd.java d1b7425 plugins/network-elements/palo-alto/src/com/cloud/api/commands/ListPaloAltoFirewallsCmd.java ad4be72 plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/api/commands/AddSspCmd.java 085f873 plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/api/commands/DeleteSspCmd.java e23f642 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPConfigCmd.java db6d7dd plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPRemoveCmd.java 535a545 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapAddConfigurationCmd.java 5686374 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java 100ffe6 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapDeleteConfigurationCmd.java b45bce5 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java 89cec65 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListConfigurationCmd.java b50970f plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListUsersCmd.java 5c65ac4 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapUserSearchCmd.java e2b050d server/src/com/cloud/api/ApiServer.java 03361a4 server/test/com/cloud/api/ApiDispatcherTest.java 7314a57 Diff: https://reviews.apache.org/r/16385/diff/ Testing --- Using CloudMonkey following commands have been tested to make sure secret key/password is stripped from the response list users list accounts list virtualmachines create user update user create sshkeypair Thanks, Mandar Barve
Re: Review Request 16385: Fix for CloudStack JIRA 4406
41c7cc1 plugins/hypervisors/ucs/src/org/apache/cloudstack/api/ListUcsManagerCmd.java 767682f plugins/hypervisors/ucs/src/org/apache/cloudstack/api/ListUcsProfileCmd.java cc49cef plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/AddBigSwitchVnsDeviceCmd.java a30059d plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/DeleteBigSwitchVnsDeviceCmd.java 4af45b2 plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/ListBigSwitchVnsDevicesCmd.java 6e4ee75 plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/api/command/CreateServiceInstanceCmd.java 50457d8 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/AddNiciraNvpDeviceCmd.java 7842d37 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/DeleteNiciraNvpDeviceCmd.java 374b0fe plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/ListNiciraNvpDeviceNetworksCmd.java 6d2dc05 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/ListNiciraNvpDevicesCmd.java 78b2ad8 plugins/network-elements/palo-alto/src/com/cloud/api/commands/AddPaloAltoFirewallCmd.java 7aba9c2 plugins/network-elements/palo-alto/src/com/cloud/api/commands/ConfigurePaloAltoFirewallCmd.java 80f02ad plugins/network-elements/palo-alto/src/com/cloud/api/commands/DeletePaloAltoFirewallCmd.java 4f147eb plugins/network-elements/palo-alto/src/com/cloud/api/commands/ListPaloAltoFirewallNetworksCmd.java d1b7425 plugins/network-elements/palo-alto/src/com/cloud/api/commands/ListPaloAltoFirewallsCmd.java ad4be72 plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/api/commands/AddSspCmd.java 085f873 plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/api/commands/DeleteSspCmd.java e23f642 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPConfigCmd.java db6d7dd plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPRemoveCmd.java 535a545 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapAddConfigurationCmd.java 5686374 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java 100ffe6 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapDeleteConfigurationCmd.java b45bce5 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java 89cec65 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListConfigurationCmd.java b50970f plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListUsersCmd.java 5c65ac4 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapUserSearchCmd.java e2b050d server/src/com/cloud/api/ApiServer.java 03361a4 server/test/com/cloud/api/ApiDispatcherTest.java 7314a57 Diff: https://reviews.apache.org/r/16385/diff/ Testing --- Using CloudMonkey following commands have been tested to make sure secret key/password is stripped from the response list users list accounts list virtualmachines create user update user create sshkeypair Thanks, Mandar Barve
Re: Review Request 16385: Fix for CloudStack JIRA 4406
Daan, I am still failing to understand the use of static vars and setter methods. If we do that then those vars will essentially become class vars and not instance vars. Don't we want each API class to have a diff instance var telling us if its sensitive or not? Am I missing something? Thanks, Mandar On Mon, Feb 10, 2014 at 1:00 AM, daan Hoogland wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16385/#review34038 > --- > > > H Mandar, some little issues applying, but mostly: the methods for setting > are not static yet and the scope of the variables themselves should be > static as well. > > > git am ~/Downloads/CS4406_02M.patch > Applying: Squashed commits for CloudStack-4406 > /Users/daan/cloudstack/cloudstack/.git/rebase-apply/patch:484: trailing > whitespace. > * cmdHandlesCriticalData method must be implemented for all APIs. This > /Users/daan/cloudstack/cloudstack/.git/rebase-apply/patch:485: trailing > whitespace. > * method declares if it handles requests and/or responses that carry > /Users/daan/cloudstack/cloudstack/.git/rebase-apply/patch:486: trailing > whitespace. > * sensitive data such as passwords, secret keys. > /Users/daan/cloudstack/cloudstack/.git/rebase-apply/patch:488: trailing > whitespace. > * Method implementation should call cmdReqIsCritical and/or > /Users/daan/cloudstack/cloudstack/.git/rebase-apply/patch:490: trailing > whitespace. > * in its request and/or response. If command doesn't carry any > sensitive > error: patch failed: > api/src/org/apache/cloudstack/api/command/admin/storage/AddS3Cmd.java:83 > error: > api/src/org/apache/cloudstack/api/command/admin/storage/AddS3Cmd.java: > patch does not apply > error: > api/src/org/apache/cloudstack/api/command/admin/storage/PrepareSecondaryStorageForMigrationCmd.java: > does not exist in index > error: patch failed: > api/src/org/apache/cloudstack/api/command/user/autoscale/ListAutoScaleVmProfilesCmd.java:71 > error: > api/src/org/apache/cloudstack/api/command/user/autoscale/ListAutoScaleVmProfilesCmd.java: > patch does not apply > error: patch failed: > plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPConfigCmd.java:170 > error: > plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPConfigCmd.java: > patch does not apply > error: patch failed: server/src/com/cloud/api/ApiServer.java:376 > error: server/src/com/cloud/api/ApiServer.java: patch does not apply > Patch failed at 0001 Squashed commits for CloudStack-4406 > The copy of the patch that failed is found in: >/Users/daan/cloudstack/cloudstack/.git/rebase-apply/patch > > - daan Hoogland > > > On Feb. 7, 2014, 10:30 a.m., Mandar Barve wrote: > > > > --- > > This is an automatically generated e-mail. To reply, visit: > > https://reviews.apache.org/r/16385/ > > --- > > > > (Updated Feb. 7, 2014, 10:30 a.m.) > > > > > > Review request for cloudstack and daan Hoogland. > > > > > > Bugs: CLOUDSTACK-4406 > > https://issues.apache.org/jira/browse/CLOUDSTACK-4406 > > > > > > Repository: cloudstack-git > > > > > > Description > > --- > > > > JIRA 4406 expects removal of cleanString() call for performance > improvements. This is called when building audit trail for command > responses and used for removing sensitive data (passwords, secret keys) > from the log buffer. All the API responses do not carry such sensitive > information so pattern matching done by cleanString against all API > response strings can be costly. > > > > I propose following for a solution: > > > > * Modify BaseCmd class to add flags that will store cmd/response > sensitivity > > * At init these flags will be set to false indicating no cmd req/resp > carries sensitive data > > * any child api cmd class that will carry sensitive data in the req/resp > should set the respective flags > > * before calling any logging function the flag should be checked and > cleanString should be called only for cmds with flags set > > > > Pro: This approach will scale well as new cmds get added and no > additional changes should be required. > > Con: Big change upfront as it will touch all API cmd classes that carry > sensitive information along with BaseCmd class. > > > > NOTE: changes should be simple and straightforward though spread ac
Re: Review Request 16385: Fix for CloudStack JIRA 4406
I think I get what you are saying. We should be using annotation per API class declaring sensitivity at class level. Using static methods returning predefined values is similar to this and annotation looks more elegant as what Nitin had suggested. At run time we will need to load this annotation and check the flag values mentioned in the annotation. My only contention then and now for this approach was on its scalability since each new class as it gets added will need to have this annotation/static method added and if developer forgets to do this we will be left with a hole. Looks like we are okay with that which is fine as long as we understand and agree with it. This change is going to be a big change across files and will take time. Thanks, Mandar On Tue, Feb 11, 2014 at 2:17 PM, Daan Hoogland wrote: > You are right in your analysis but about the methods you are drawing > the wrong conclusion. > > We want each class to have its own values, not each command object. > Any BlaCmd should have exactly the same values so it makes sense to > make them static on the class object. > > About the variables; I see how my remarks about those is confusing. We > don't need them. As the methods are static and per class they can > return either true or false. without vars containing those values. > > hope this clariifies > > On Tue, Feb 11, 2014 at 8:54 AM, Mandar Barve > wrote: > > Daan, > > I am still failing to understand the use of static vars and setter > > methods. If we do that then those vars will essentially become class vars > > and not instance vars. Don't we want each API class to have a diff > instance > > var telling us if its sensitive or not? Am I missing something? > > > > Thanks, > > Mandar > > > > > > On Mon, Feb 10, 2014 at 1:00 AM, daan Hoogland > > wrote: > >> > >> > >> --- > >> > >> This is an automatically generated e-mail. To reply, visit: > >> https://reviews.apache.org/r/16385/#review34038 > >> --- > >> > >> > >> > >> H Mandar, some little issues applying, but mostly: the methods for > setting > >> are not static yet and the scope of the variables themselves should be > >> static as well. > >> > >> > git am ~/Downloads/CS4406_02M.patch > >> Applying: Squashed commits for CloudStack-4406 > >> /Users/daan/cloudstack/cloudstack/.git/rebase-apply/patch:484: trailing > >> whitespace. > >> * cmdHandlesCriticalData method must be implemented for all APIs. > >> This > >> /Users/daan/cloudstack/cloudstack/.git/rebase-apply/patch:485: trailing > >> whitespace. > >> * method declares if it handles requests and/or responses that > carry > >> /Users/daan/cloudstack/cloudstack/.git/rebase-apply/patch:486: trailing > >> whitespace. > >> * sensitive data such as passwords, secret keys. > >> /Users/daan/cloudstack/cloudstack/.git/rebase-apply/patch:488: trailing > >> whitespace. > >> * Method implementation should call cmdReqIsCritical and/or > >> /Users/daan/cloudstack/cloudstack/.git/rebase-apply/patch:490: trailing > >> whitespace. > >> * in its request and/or response. If command doesn't carry any > >> sensitive > >> error: patch failed: > >> api/src/org/apache/cloudstack/api/command/admin/storage/AddS3Cmd.java:83 > >> error: > >> api/src/org/apache/cloudstack/api/command/admin/storage/AddS3Cmd.java: > patch > >> does not apply > >> error: > >> > api/src/org/apache/cloudstack/api/command/admin/storage/PrepareSecondaryStorageForMigrationCmd.java: > >> does not exist in index > >> error: patch failed: > >> > api/src/org/apache/cloudstack/api/command/user/autoscale/ListAutoScaleVmProfilesCmd.java:71 > >> error: > >> > api/src/org/apache/cloudstack/api/command/user/autoscale/ListAutoScaleVmProfilesCmd.java: > >> patch does not apply > >> error: patch failed: > >> > plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPConfigCmd.java:170 > >> error: > >> > plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPConfigCmd.java: > >> patch does not apply > >> error: patch failed: server/src/com/cloud/api/ApiServer.java:376 > >> error: server/src/com/cloud/api/ApiServer.java: patch does not apply > >> Patch failed at 0001 Squashed commits for CloudStack-4406 > >> The copy of the p
Re: Review Request 16385: Fix for CloudStack JIRA 4406
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16385/ --- (Updated Feb. 18, 2014, 11:41 a.m.) Review request for cloudstack and daan Hoogland. Changes --- Daan, I have created a pilot patch based on our discussion. In this I have modified existing APICommand annotation to add extra flags. Let me know if this looks okay or something needs to change. If this is fine I can start making changes to the rest of the code. Thanks, Mandar Bugs: CLOUDSTACK-4406 https://issues.apache.org/jira/browse/CLOUDSTACK-4406 Repository: cloudstack-git Description --- JIRA 4406 expects removal of cleanString() call for performance improvements. This is called when building audit trail for command responses and used for removing sensitive data (passwords, secret keys) from the log buffer. All the API responses do not carry such sensitive information so pattern matching done by cleanString against all API response strings can be costly. I propose following for a solution: * Modify BaseCmd class to add flags that will store cmd/response sensitivity * At init these flags will be set to false indicating no cmd req/resp carries sensitive data * any child api cmd class that will carry sensitive data in the req/resp should set the respective flags * before calling any logging function the flag should be checked and cleanString should be called only for cmds with flags set Pro: This approach will scale well as new cmds get added and no additional changes should be required. Con: Big change upfront as it will touch all API cmd classes that carry sensitive information along with BaseCmd class. NOTE: changes should be simple and straightforward though spread across multiple classes. Diffs (updated) - api/src/org/apache/cloudstack/api/APICommand.java 5587a48 api/src/org/apache/cloudstack/api/command/admin/account/CreateAccountCmd.java c5a2d1a api/src/org/apache/cloudstack/api/command/admin/account/DeleteAccountCmd.java 7c1b206 api/src/org/apache/cloudstack/api/command/admin/account/DisableAccountCmd.java 6fdbefe api/src/org/apache/cloudstack/api/command/admin/account/EnableAccountCmd.java 59d6acd api/src/org/apache/cloudstack/api/command/user/account/ListAccountsCmd.java bc93d21 server/src/com/cloud/api/ApiServer.java d715db6 Diff: https://reviews.apache.org/r/16385/diff/ Testing --- Using CloudMonkey following commands have been tested to make sure secret key/password is stripped from the response list users list accounts list virtualmachines create user update user create sshkeypair Thanks, Mandar Barve
Re: Review Request 16385: Fix for CloudStack JIRA 4406
plugins/network-elements/f5/src/com/cloud/api/commands/ListF5LoadBalancerNetworksCmd.java d671a6b plugins/network-elements/f5/src/com/cloud/api/commands/ListF5LoadBalancersCmd.java ce8eb20 plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/api/command/CreateServiceInstanceCmd.java 50457d8 plugins/network-elements/juniper-srx/src/com/cloud/api/commands/AddExternalFirewallCmd.java 96e9bc0 plugins/network-elements/juniper-srx/src/com/cloud/api/commands/AddSrxFirewallCmd.java 53e7bfc plugins/network-elements/juniper-srx/src/com/cloud/api/commands/ConfigureSrxFirewallCmd.java 303e987 plugins/network-elements/juniper-srx/src/com/cloud/api/commands/DeleteExternalFirewallCmd.java b24aa05 plugins/network-elements/juniper-srx/src/com/cloud/api/commands/DeleteSrxFirewallCmd.java 75aadc2 plugins/network-elements/juniper-srx/src/com/cloud/api/commands/ListExternalFirewallsCmd.java f87574e plugins/network-elements/juniper-srx/src/com/cloud/api/commands/ListSrxFirewallNetworksCmd.java a2d99a5 plugins/network-elements/juniper-srx/src/com/cloud/api/commands/ListSrxFirewallsCmd.java e8cdcd5 plugins/network-elements/netscaler/src/com/cloud/api/commands/AddNetscalerLoadBalancerCmd.java b744fff plugins/network-elements/netscaler/src/com/cloud/api/commands/ConfigureNetscalerLoadBalancerCmd.java f8bfd74 plugins/network-elements/netscaler/src/com/cloud/api/commands/DeleteNetscalerLoadBalancerCmd.java f0f6747 plugins/network-elements/netscaler/src/com/cloud/api/commands/ListNetscalerLoadBalancerNetworksCmd.java 25d1487 plugins/network-elements/netscaler/src/com/cloud/api/commands/ListNetscalerLoadBalancersCmd.java 75d03d7 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/AddNiciraNvpDeviceCmd.java 37491fd plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/DeleteNiciraNvpDeviceCmd.java 685debb plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/ListNiciraNvpDeviceNetworksCmd.java 3fb711d plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/ListNiciraNvpDevicesCmd.java 260d60f plugins/network-elements/opendaylight/src/main/java/org/apache/cloudstack/network/opendaylight/api/commands/AddOpenDaylightControllerCmd.java 80130c0 plugins/network-elements/opendaylight/src/main/java/org/apache/cloudstack/network/opendaylight/api/commands/DeleteOpenDaylightControllerCmd.java b556504 plugins/network-elements/opendaylight/src/main/java/org/apache/cloudstack/network/opendaylight/api/commands/ListOpenDaylightControllersCmd.java f3e2e39 plugins/network-elements/palo-alto/src/com/cloud/api/commands/AddPaloAltoFirewallCmd.java 7aba9c2 plugins/network-elements/palo-alto/src/com/cloud/api/commands/ConfigurePaloAltoFirewallCmd.java 80f02ad plugins/network-elements/palo-alto/src/com/cloud/api/commands/DeletePaloAltoFirewallCmd.java 4f147eb plugins/network-elements/palo-alto/src/com/cloud/api/commands/ListPaloAltoFirewallNetworksCmd.java d1b7425 plugins/network-elements/palo-alto/src/com/cloud/api/commands/ListPaloAltoFirewallsCmd.java ad4be72 plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/api/commands/AddSspCmd.java 085f873 plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/api/commands/DeleteSspCmd.java e23f642 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPConfigCmd.java 5e424de plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPRemoveCmd.java 535a545 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapAddConfigurationCmd.java 5686374 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java 100ffe6 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapDeleteConfigurationCmd.java b45bce5 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java 89cec65 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListConfigurationCmd.java b50970f plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListUsersCmd.java 5c65ac4 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapUserSearchCmd.java e2b050d server/src/com/cloud/api/ApiServer.java d715db6 Diff: https://reviews.apache.org/r/16385/diff/ Testing --- Using CloudMonkey following commands have been tested to make sure secret key/password is stripped from the response list users list accounts list virtualmachines create user update user create sshkeypair Thanks, Mandar Barve
Re: Review Request 16385: Fix for CloudStack JIRA 4406
-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/api/command/CreateServiceInstanceCmd.java 50457d8 plugins/network-elements/juniper-srx/src/com/cloud/api/commands/AddExternalFirewallCmd.java 96e9bc0 plugins/network-elements/juniper-srx/src/com/cloud/api/commands/AddSrxFirewallCmd.java 53e7bfc plugins/network-elements/juniper-srx/src/com/cloud/api/commands/ConfigureSrxFirewallCmd.java 303e987 plugins/network-elements/juniper-srx/src/com/cloud/api/commands/DeleteExternalFirewallCmd.java b24aa05 plugins/network-elements/juniper-srx/src/com/cloud/api/commands/DeleteSrxFirewallCmd.java 75aadc2 plugins/network-elements/juniper-srx/src/com/cloud/api/commands/ListExternalFirewallsCmd.java f87574e plugins/network-elements/juniper-srx/src/com/cloud/api/commands/ListSrxFirewallNetworksCmd.java a2d99a5 plugins/network-elements/juniper-srx/src/com/cloud/api/commands/ListSrxFirewallsCmd.java e8cdcd5 plugins/network-elements/netscaler/src/com/cloud/api/commands/AddNetscalerLoadBalancerCmd.java b744fff plugins/network-elements/netscaler/src/com/cloud/api/commands/ConfigureNetscalerLoadBalancerCmd.java f8bfd74 plugins/network-elements/netscaler/src/com/cloud/api/commands/DeleteNetscalerLoadBalancerCmd.java f0f6747 plugins/network-elements/netscaler/src/com/cloud/api/commands/ListNetscalerLoadBalancerNetworksCmd.java 25d1487 plugins/network-elements/netscaler/src/com/cloud/api/commands/ListNetscalerLoadBalancersCmd.java 75d03d7 plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/AddNiciraNvpDeviceCmd.java 37491fd plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/DeleteNiciraNvpDeviceCmd.java 685debb plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/ListNiciraNvpDeviceNetworksCmd.java 3fb711d plugins/network-elements/nicira-nvp/src/com/cloud/api/commands/ListNiciraNvpDevicesCmd.java 260d60f plugins/network-elements/opendaylight/src/main/java/org/apache/cloudstack/network/opendaylight/api/commands/AddOpenDaylightControllerCmd.java 80130c0 plugins/network-elements/opendaylight/src/main/java/org/apache/cloudstack/network/opendaylight/api/commands/DeleteOpenDaylightControllerCmd.java b556504 plugins/network-elements/opendaylight/src/main/java/org/apache/cloudstack/network/opendaylight/api/commands/ListOpenDaylightControllersCmd.java f3e2e39 plugins/network-elements/palo-alto/src/com/cloud/api/commands/AddPaloAltoFirewallCmd.java 7aba9c2 plugins/network-elements/palo-alto/src/com/cloud/api/commands/ConfigurePaloAltoFirewallCmd.java 80f02ad plugins/network-elements/palo-alto/src/com/cloud/api/commands/DeletePaloAltoFirewallCmd.java 4f147eb plugins/network-elements/palo-alto/src/com/cloud/api/commands/ListPaloAltoFirewallNetworksCmd.java d1b7425 plugins/network-elements/palo-alto/src/com/cloud/api/commands/ListPaloAltoFirewallsCmd.java ad4be72 plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/api/commands/AddSspCmd.java 085f873 plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/api/commands/DeleteSspCmd.java e23f642 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPConfigCmd.java 5e424de plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LDAPRemoveCmd.java 535a545 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapAddConfigurationCmd.java 5686374 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java 100ffe6 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapDeleteConfigurationCmd.java b45bce5 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java 89cec65 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListConfigurationCmd.java b50970f plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListUsersCmd.java 5c65ac4 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapUserSearchCmd.java e2b050d server/src/com/cloud/api/ApiServer.java d715db6 Diff: https://reviews.apache.org/r/16385/diff/ Testing --- Using CloudMonkey following commands have been tested to make sure secret key/password is stripped from the response list users list accounts list virtualmachines create user update user create sshkeypair Thanks, Mandar Barve
