----------------------------------------------------------- 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 review request says. The syntax for the api developer is simple. in case of sloppy programming we loose performance because the default is true, but we don't security (testing needed) We could remove the default to make sure developers think but they might grow the habit of putting false, so I'm not sure on that. Good work Mandar - daan Hoogland On Feb. 18, 2014, 11:41 a.m., Mandar Barve wrote: > > ----------------------------------------------------------- > 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. > > > 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/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 > >
