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 t

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 b

Re: Review Request 16385: Fix for CloudStack JIRA 4406

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:

Re: Review Request 16385: Fix for CloudStack JIRA 4406

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 Prok

Re: Review Request 16385: Fix for CloudStack JIRA 4406

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 pa

Re: Review Request 16385: Fix for CloudStack JIRA 4406

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 e

Re: Review Request 16385: Fix for CloudStack JIRA 4406

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

Re: Review Request 16385: Fix for CloudStack JIRA 4406

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 pa

Re: Review Request 16385: Fix for CloudStack JIRA 4406

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 perform

Re: Review Request 16385: Fix for CloudStack JIRA 4406

Mandar, I¹ve ran into this checkin submitted by you: b0c6d4734724358df97b6fa4d8c5beb0f447745e - Updated APICommand annotation to add new flags that indicate if API request or response carry sensitive info And have a couple of comments on that. 1) I don¹t see the parameter being checked anywhere

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/#review35477 --- Ship it! b0c6d4734724358df97b6fa4d8c5beb0f447745e - daan Hoogland

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/#review35475 --- Ship it! b0c6d4734724358df97b6fa4d8c5beb0f447745e - daan Hoogland

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/#review35276 --- I'll download and money test. please adjust the description to indic

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. 24, 2014, 2:38 p.m.) Review request for cloudstack and daan Hoogl

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. 24, 2014, 2:37 p.m.) Review request for cloudstack and daan Hoogl

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/#review34691 --- I like. the good default is used (unlike what the description of the

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 Hoog

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 an

Re: Review Request 16385: Fix for CloudStack JIRA 4406

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 m

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,

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/#review34038 --- H Mandar, some little issues applying, but mostly: the methods for s

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/#review34037 --- Mandar, this does not apply to master. It seems note serious but the

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/#review34036 --- Mandar, this does not apply to master. It seems note serious but the

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. 7, 2014, 10:30 a.m.) Review request for cloudstack and daan Hoogl

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. 7, 2014, 10:30 a.m.) Review request for cloudstack and daan Hoogl

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

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/#review33306 --- Mandar, still feel like picking this up. As Nithin didn't reply anym

Re: Review Request 16385: Fix for CloudStack JIRA 4406

I mean to have the setting of the vars and the vars them selves be static , not the retrieving method. On Thu, Jan 9, 2014 at 1:52 PM, Mandar Barve wrote: > Daan, > I don't get the idea behind making the methods static. Making > getter/setters static will lead to instance vars losing their m

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?

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 > > > > > > You shouldn't have to override for every cmd. By default its fal

Re: Review Request 16385: Fix for CloudStack JIRA 4406

Nithin, I think your last points are valid but should not stop Mandar's change. Except for making the booleans static I think further improvements are for the next version and we should apply Mandar's version. On Thu, Jan 2, 2014 at 12:39 PM, Mandar Barve wrote: >This is an automatically gen

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 > > > > > > You shouldn't have to override for every cmd. By default its fal

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 > > > > > > Can you please create names which are more intuitive such as > > cmdRequestContainsSensitiveIn

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 > > > > > > Can you please create names which are more intuitive such as > > cmdRequestContainsSensitiveIn

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 Dec. 23, 2013, 6:13 p.m.) Review request for cloudstack and daan Hoogl

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 Dec. 23, 2013, 6:11 p.m.) Review request for cloudstack and daan Hoogl

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/#review30828 --- api/src/org/apache/cloudstack/api/BaseCmd.java

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

Re: Review Request 16385: Fix for CloudStack JIRA 4406

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 cmdResp

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 > > > > > > please make sure a clear and extensive javadoc is present on why and > > how this abstract m

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/#review30699 --- api/src/org/apache/cloudstack/api/BaseCmd.java

Review Request 16385: Fix for CloudStack JIRA 4406

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16385/ --- Review request for cloudstack and daan Hoogland. Bugs: CLOUDSTACK-4406 http