You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Mandar Barve <ma...@sungard.com> on 2013/11/28 10:38:54 UTC
[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
Posted by Mandar Barve <ma...@sungard.com>.
Thanks Daan.
Thanks,
Mandar
On Fri, Nov 29, 2013 at 5:16 PM, Daan Hoogland <da...@gmail.com>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 <ma...@sungard.com>
> 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 <daan.hoogland@gmail.com
> >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 <mandar.barve@sungard.com
> >
> >> 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.hoogland@gmail.com
> >> >wrote:
> >> >
> >> >> On Thu, Nov 28, 2013 at 10:38 AM, Mandar Barve <
> >> mandar.barve@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
Posted by Daan Hoogland <da...@gmail.com>.
Mandar,
I agree, go ahead. Let me know if you need any help.
On Fri, Nov 29, 2013 at 12:40 PM, Mandar Barve <ma...@sungard.com> 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 <da...@gmail.com>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 <ma...@sungard.com>
>> 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.hoogland@gmail.com
>> >wrote:
>> >
>> >> On Thu, Nov 28, 2013 at 10:38 AM, Mandar Barve <
>> mandar.barve@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
Posted by Mandar Barve <ma...@sungard.com>.
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 <da...@gmail.com>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 <ma...@sungard.com>
> 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.hoogland@gmail.com
> >wrote:
> >
> >> On Thu, Nov 28, 2013 at 10:38 AM, Mandar Barve <
> mandar.barve@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
Posted by Daan Hoogland <da...@gmail.com>.
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 <ma...@sungard.com> 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 <da...@gmail.com>wrote:
>
>> On Thu, Nov 28, 2013 at 10:38 AM, Mandar Barve <ma...@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
Posted by Mandar Barve <ma...@sungard.com>.
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 <da...@gmail.com>wrote:
> On Thu, Nov 28, 2013 at 10:38 AM, Mandar Barve <ma...@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
Posted by Daan Hoogland <da...@gmail.com>.
On Thu, Nov 28, 2013 at 10:38 AM, Mandar Barve <ma...@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