You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by bhaisaab <gi...@git.apache.org> on 2015/06/26 18:43:49 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8566: Filter sensitive details...
GitHub user bhaisaab opened a pull request:
https://github.com/apache/cloudstack/pull/536
CLOUDSTACK-8566: Filter sensitive details from listHosts API
Removes any username and password host details from the listHosts API response
before it's wired back to a client.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/cloudstack CLOUDSTACK-8566
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/536.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #536
----
commit 49d1f54f98ecf995beae928284f67df56a30f176
Author: Rohit Yadav <ro...@shapeblue.com>
Date: 2015-06-26T16:42:00Z
CLOUDSTACK-8566: Filter sensitive details from listHosts API
Removes any username and password host details from the listHosts API response
before it's wired back to a client.
Signed-off-by: Rohit Yadav <ro...@shapeblue.com>
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8566: Filter sensitive details...
Posted by serverchief <gi...@git.apache.org>.
Github user serverchief commented on the pull request:
https://github.com/apache/cloudstack/pull/536#issuecomment-115915514
Hi Rohit,
Both username and passwords are needed, and we don't want to remove them and we would like to know what they are, however, it would be great if we can obfuscate the password leaving only first 2 characters visible and making it X chars long.
For example, if password is "MySuperPass", perhaps the output would be "My******************************"
We don't want to expose the actual passwords length, so if we can fill it with extra "*" that would help. I'm not certain what restrictions are there, perhaps 32 chars maybe enough.
Thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8566: Filter sensitive details...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/536#discussion_r33399534
--- Diff: api/src/org/apache/cloudstack/api/command/admin/host/ListHostsCmd.java ---
@@ -203,9 +201,22 @@ public void execute() {
hostResponse.setObjectName("host");
hostResponses.add(hostResponse);
}
-
response.setResponses(hostResponses, result.second());
}
+ // Remove sensitive details from host response
+ List<HostResponse> hostResponsesList = response.getResponses();
+ for (HostResponse hostResponse: hostResponsesList) {
+ Map<String, String> hostDetails = hostResponse.getDetails();
+ if (hostDetails == null) continue;
+ if (hostDetails.containsKey("username")) {
+ hostDetails.remove("username");
+ }
+ if (hostDetails.containsKey("password")) {
+ hostDetails.remove("password");
+ }
+ hostResponse.setDetails(hostDetails);
--- End diff --
Why not perform this filtering on a defensive copy of the map in HostResponse.setDetails()? I am also concerned about directly manipulating the details map in this method as it changes the view for any other objects holding references to it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8566: Filter sensitive details...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab closed the pull request at:
https://github.com/apache/cloudstack/pull/536
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8566: Filter sensitive details...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/536#discussion_r33625103
--- Diff: api/src/org/apache/cloudstack/api/command/admin/host/ListHostsCmd.java ---
@@ -203,9 +201,22 @@ public void execute() {
hostResponse.setObjectName("host");
hostResponses.add(hostResponse);
}
-
response.setResponses(hostResponses, result.second());
}
+ // Remove sensitive details from host response
+ List<HostResponse> hostResponsesList = response.getResponses();
+ for (HostResponse hostResponse: hostResponsesList) {
+ Map<String, String> hostDetails = hostResponse.getDetails();
+ if (hostDetails == null) continue;
+ if (hostDetails.containsKey("username")) {
+ hostDetails.remove("username");
+ }
+ if (hostDetails.containsKey("password")) {
+ hostDetails.remove("password");
+ }
+ hostResponse.setDetails(hostDetails);
--- End diff --
@bhaisaab I incorrectly assumed the HostResponse was only be used to create the ListHostCmd result to the client. Based on the scope described, the location of this filtering is the narrowest scope possible. We should avoid assumptions about how the ``HostDetails`` object is being consumed across threads, and make a defensive copy of it before modifying it. Also, the code can be simplified by to remove the ``containsKey`` if blocks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8566: Filter sensitive details...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/536#issuecomment-115751581
I'm not sure where the host details are getting consumed as there are many APIs using the HostResponse class, so the safest fix was to filter out sensitive details from the host response in list hosts API. cc @jburwell @wilderrodrigues and others for review
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8566: Filter sensitive details...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/536#discussion_r33651106
--- Diff: api/src/org/apache/cloudstack/api/command/admin/host/ListHostsCmd.java ---
@@ -203,9 +201,22 @@ public void execute() {
hostResponse.setObjectName("host");
hostResponses.add(hostResponse);
}
-
response.setResponses(hostResponses, result.second());
}
+ // Remove sensitive details from host response
+ List<HostResponse> hostResponsesList = response.getResponses();
+ for (HostResponse hostResponse: hostResponsesList) {
+ Map<String, String> hostDetails = hostResponse.getDetails();
+ if (hostDetails == null) continue;
+ if (hostDetails.containsKey("username")) {
+ hostDetails.remove("username");
+ }
+ if (hostDetails.containsKey("password")) {
+ hostDetails.remove("password");
+ }
+ hostResponse.setDetails(hostDetails);
--- End diff --
@bhaisaab, @jburwell, returning password from other apis like reconnectHost etc. isnt required either (api doc response is no different)
I think it should be removed from the lower layer in HostJoinDaoImpl so that no api returns them
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8566: Filter sensitive details...
Posted by serverchief <gi...@git.apache.org>.
Github user serverchief commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/536#discussion_r33648388
--- Diff: api/src/org/apache/cloudstack/api/command/admin/host/ListHostsCmd.java ---
@@ -203,9 +201,22 @@ public void execute() {
hostResponse.setObjectName("host");
hostResponses.add(hostResponse);
}
-
response.setResponses(hostResponses, result.second());
}
+ // Remove sensitive details from host response
+ List<HostResponse> hostResponsesList = response.getResponses();
+ for (HostResponse hostResponse: hostResponsesList) {
+ Map<String, String> hostDetails = hostResponse.getDetails();
+ if (hostDetails == null) continue;
+ if (hostDetails.containsKey("username")) {
+ hostDetails.remove("username");
+ }
+ if (hostDetails.containsKey("password")) {
+ hostDetails.remove("password");
+ }
+ hostResponse.setDetails(hostDetails);
--- End diff --
hey guys, its upto you how you want to handle it, our end goal is not to expose entire access details when listHosts api call is made. As mentioned earlier, it would be ideal to leave few characters for username and password VS doing hostDetails.remove.
Thanks
-ilya
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8566: Filter sensitive details...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/536#issuecomment-116744035
@serverchief IMO ideally we should not be returning username/passwords. One can use config management systems to update their host passwords and use updateHost API to update the host's new password in CloudStack. In practice, if there is a need to still have the password information returned by the API we can obfuscate it before returning it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8566: Filter sensitive details...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/536#discussion_r33479990
--- Diff: api/src/org/apache/cloudstack/api/command/admin/host/ListHostsCmd.java ---
@@ -203,9 +201,22 @@ public void execute() {
hostResponse.setObjectName("host");
hostResponses.add(hostResponse);
}
-
response.setResponses(hostResponses, result.second());
}
+ // Remove sensitive details from host response
+ List<HostResponse> hostResponsesList = response.getResponses();
+ for (HostResponse hostResponse: hostResponsesList) {
+ Map<String, String> hostDetails = hostResponse.getDetails();
+ if (hostDetails == null) continue;
+ if (hostDetails.containsKey("username")) {
+ hostDetails.remove("username");
+ }
+ if (hostDetails.containsKey("password")) {
+ hostDetails.remove("password");
+ }
+ hostResponse.setDetails(hostDetails);
--- End diff --
@jburwell The HostResponse created by service layer is being consumed at several places such as AddHostCmd, UpdateHostCmd, ReconnectHostCmd etc. while the details are set in HostJoinDaoImpl (much lower layer than the service layer), this is why I thought the best fix would be to fix the response in ListHostsCmd before it is wired back to the client.
The response class gets instantiated in the current request thread and is not referenced by any other threads or components, that's why it's safe to simply manipulate the map in the response class without adding the overhead of copying it. It's safe in this context, though I agree with the immutable pattern you're proposing here - I can add code to copy the details map instead of manipulating it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8566: Filter sensitive details...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/536#issuecomment-117600568
I guess a better fix needs to be worked on, closing now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---