You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by kansal <gi...@git.apache.org> on 2015/12/02 13:43:00 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-9099: SecretKey is returned fr...
GitHub user kansal opened a pull request:
https://github.com/apache/cloudstack/pull/1152
CLOUDSTACK-9099: SecretKey is returned from the APIs - Fixed
The current implementation of User and account management API (in general) return the secret key as a user or account response.
Fix: Added a new API to explicitly return the secretKey and removed it from the user and account response.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kansal/cloudstack CLOUDSTACK-9099
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1152.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 #1152
----
commit 410045a97a75fd1e43972c66bc4882a30a5098bf
Author: Kshitij Kansal <ka...@gmail.com>
Date: 2015-12-02T10:43:45Z
CLOUDSTACK-9099: SecretKey is returned from the APIs - Fixed
----
---
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-9099: SecretKey is returned fr...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1152#discussion_r62336714
--- Diff: api/src/org/apache/cloudstack/api/command/admin/user/GetUserKeysCmd.java ---
@@ -0,0 +1,74 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.cloudstack.api.command.admin.user;
+
+
+import com.cloud.user.Account;
+import com.cloud.user.User;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.response.RegisterResponse;
+import org.apache.cloudstack.api.response.UserResponse;
+
+import java.util.List;
+import java.util.logging.Logger;
+
+@APICommand(name = "getUserKeys",
+ description = "This command allows the user to query the seceret and API keys for the account",
+ responseObject = RegisterResponse.class,
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = true)
--- End diff --
Please add the version annotation to indicate that this command was added for 4.9.
---
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-9099: SecretKey is returned fr...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1152#discussion_r62337130
--- Diff: server/test/com/cloud/user/MockAccountManagerImpl.java ---
@@ -401,5 +403,24 @@ public Long finalyzeAccountId(String accountName, Long domainId, Long projectId,
return null;
}
+ @Override
+ public List<String> getKeys(GetUserKeysCmd cmd) {
+ return null;
+ }
+
+ @Override
+ public void checkAccess(User user, ControlledEntity entity)
+ throws PermissionDeniedException {
+
+ }
+ @Override
+ public String getConfigComponentName() {
+ return null;
--- End diff --
Please return a blank string to avoid NPEs.
---
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-9099: SecretKey is returned fr...
Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-216217062
@DaanHoogland sure. Will rebase and keep the default value to false.
---
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-9099: SecretKey is returned fr...
Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-161944643
@jburwell Sure!! Will look into these. Adding the test cases and some UI changes for this to work. Will update the PR.
---
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-9099: SecretKey is returned fr...
Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-168295517
@DaanHoogland Agreed with that point. But its not only about the testing. I'm sure many people will be using it in their own integration. I think we should not change the response immediately like this without informing or making a noise about it. And it is because of this concern only, I added a flag for enabling/disabling the secret key in response.
---
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-9099: SecretKey is returned fr...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1152#discussion_r46649429
--- Diff: api/src/org/apache/cloudstack/api/command/admin/user/ListKeysCmd.java ---
@@ -0,0 +1,72 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.cloudstack.api.command.admin.user;
+
+
+import com.cloud.user.Account;
+import com.cloud.user.User;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.response.RegisterResponse;
+import org.apache.cloudstack.api.response.UserResponse;
+
+import java.util.logging.Logger;
+
+@APICommand(name = "listUserKeys",
+ description = "This command allows the user to query the seceret and API keys for the account",
+ responseObject = RegisterResponse.class,
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = true)
+
+public class ListKeysCmd extends BaseCmd{
+
+ @Parameter(name= ApiConstants.ID, type = CommandType.UUID, entityType = UserResponse.class, required = true, description = "ID of the user whose keys are required")
+ private Long id;
+
+ public static final Logger s_logger = Logger.getLogger(RegisterCmd.class.getName());
+ public static final String s_name = "listuserkeysresponse";
+
+ public Long getID(){
+ return id;
+ }
+
+ public String getCommandName(){
+ return s_name;
+ }
+
+ public long getEntityOwnerId(){
+ User user = _entityMgr.findById(User.class, getID());
+ if(user != null){
+ return user.getAccountId();
+ }
+ else return Account.ACCOUNT_ID_SYSTEM;
+ }
+ public void execute(){
+ String[] keys = _accountService.getKeys(this);
+ RegisterResponse response = new RegisterResponse();
+ if(keys != null){
--- End diff --
Add an check before setting the keys to check that ``keys`` has a length = 2 to avoid an ``ArrayIndexOutOfBoundsException``. If the length is not equal to 2, throw an ``IllegalStateException``.
---
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-9099: SecretKey is returned fr...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1152#discussion_r62336941
--- Diff: server/src/com/cloud/user/AccountManager.java ---
@@ -198,4 +200,11 @@ void buildACLViewSearchCriteria(SearchCriteria<? extends ControlledViewEntity> s
public static final String MESSAGE_ADD_ACCOUNT_EVENT = "Message.AddAccount.Event";
public static final String MESSAGE_REMOVE_ACCOUNT_EVENT = "Message.RemoveAccount.Event";
+ public static final ConfigKey<Boolean> UseSecretKeyInResponse = new ConfigKey<Boolean>(
+ "Advanced",
+ Boolean.class,
+ "use.secret.key.in.response",
+ "true",
--- End diff --
@kansal I agree with @DaanHoogland and @remibergsma -- it's about reasonable and secure defaults. We should not configure a management server insecurely by default.
---
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-9099: SecretKey is returned fr...
Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-163539799
@DaanHoogland Sure will try. Will take some time as I have to go through the documentation first.
---
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-9099: SecretKey is returned fr...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-162869020
@kansal looks good, but for a change like this I would like a marvin test to prove it and guarantee it's continued functioning/functionality Do you see chance to add that?
---
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-9099: SecretKey is returned fr...
Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-202844861
@DaanHoogland I am not sure of the amount of work that needs to be done for fixing all the existing test cases. Will revisit this and update. I still personally think that going with the optional parameter presently and assuming( and making sure) that the new test cases are written in compliance with this API will be a good way to go forward. Your views?
PS: I am still not very versed with the test case suits. Will check and revisit this.
---
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-9099: SecretKey is returned fr...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1152#discussion_r46947731
--- Diff: api/src/org/apache/cloudstack/api/command/admin/user/ListKeysCmd.java ---
@@ -0,0 +1,74 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.cloudstack.api.command.admin.user;
+
+
+import com.cloud.user.Account;
+import com.cloud.user.User;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.response.RegisterResponse;
+import org.apache.cloudstack.api.response.UserResponse;
+
+import java.util.List;
+import java.util.logging.Logger;
+
+@APICommand(name = "listUserKeys",
+ description = "This command allows the user to query the seceret and API keys for the account",
+ responseObject = RegisterResponse.class,
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = true)
+
+public class ListKeysCmd extends BaseCmd{
+
+ @Parameter(name= ApiConstants.ID, type = CommandType.UUID, entityType = UserResponse.class, required = true, description = "ID of the user whose keys are required")
+ private Long id;
+
+ public static final Logger s_logger = Logger.getLogger(RegisterCmd.class.getName());
+ public static final String s_name = "listuserkeysresponse";
--- End diff --
same here
---
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-9099: SecretKey is returned fr...
Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-165426036
Have updated this PR. Instead of directly removing the secret key from response, I have deprecated that as many regressions were using the secret key from those APIs for authentication. Maybe from next major release we can remove that.
@DaanHoogland marvin test cases on the way!!!
---
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-9099: SecretKey is returned fr...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-168527559
@kansal I don't agree that making noise first is the way to go. We should disable the return of the key first and document it. Security demands that we play it that way. We can allow users to enable this insecure bahaviour by setting a flag somewhere but it should not be default and catch the unaware users of guard. It will be work in the integration tests but that just will have to happen.
---
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-9099: SecretKey is returned fr...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1152#discussion_r62337172
--- Diff: server/test/com/cloud/user/MockAccountManagerImpl.java ---
@@ -401,5 +403,24 @@ public Long finalyzeAccountId(String accountName, Long domainId, Long projectId,
return null;
}
+ @Override
+ public List<String> getKeys(GetUserKeysCmd cmd) {
+ return null;
+ }
+
+ @Override
+ public void checkAccess(User user, ControlledEntity entity)
+ throws PermissionDeniedException {
+
+ }
+ @Override
+ public String getConfigComponentName() {
+ return null;
+ }
+
+ @Override
+ public ConfigKey<?>[] getConfigKeys() {
+ return null;
--- End diff --
Please return an empty array to avoid NPEs.
---
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-9099: SecretKey is returned fr...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-216215437
@kansal please go ahead and remove the key from the response. We'll test run it and add fixes to tests if needed. (cc @rhtyd my last comment is still valid)
---
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-9099: SecretKey is returned fr...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1152#discussion_r48695937
--- Diff: server/src/com/cloud/user/AccountManager.java ---
@@ -198,4 +200,11 @@ void buildACLViewSearchCriteria(SearchCriteria<? extends ControlledViewEntity> s
public static final String MESSAGE_ADD_ACCOUNT_EVENT = "Message.AddAccount.Event";
public static final String MESSAGE_REMOVE_ACCOUNT_EVENT = "Message.RemoveAccount.Event";
+ public static final ConfigKey<Boolean> UseSecretKeyInResponse = new ConfigKey<Boolean>(
+ "Advanced",
+ Boolean.class,
+ "use.secret.key.in.response",
+ "true",
--- End diff --
Agree with @DaanHoogland, it is easy enough to enable again should people need it. Is this setting available to the user or does it need to be added to the database as well?
---
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-9099: SecretKey is returned fr...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1152#discussion_r48657695
--- Diff: server/src/com/cloud/user/AccountManager.java ---
@@ -198,4 +200,11 @@ void buildACLViewSearchCriteria(SearchCriteria<? extends ControlledViewEntity> s
public static final String MESSAGE_ADD_ACCOUNT_EVENT = "Message.AddAccount.Event";
public static final String MESSAGE_REMOVE_ACCOUNT_EVENT = "Message.RemoveAccount.Event";
+ public static final ConfigKey<Boolean> UseSecretKeyInResponse = new ConfigKey<Boolean>(
+ "Advanced",
+ Boolean.class,
+ "use.secret.key.in.response",
+ "true",
--- End diff --
default should be false! this is a security issue.
---
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-9099: SecretKey is returned fr...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1152#discussion_r62337077
--- Diff: server/test/com/cloud/user/MockAccountManagerImpl.java ---
@@ -401,5 +403,24 @@ public Long finalyzeAccountId(String accountName, Long domainId, Long projectId,
return null;
}
+ @Override
+ public List<String> getKeys(GetUserKeysCmd cmd) {
+ return null;
--- End diff --
Please return a ``Collections.emptyList()`` to avoid NPEs.
---
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-9099: SecretKey is returned fr...
Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-162867278
@jburwell @DaanHoogland @kishankavala Have included some changes related to the UI. Now after generating the keys from UI, after ListUserCmd() api, listKeysCmd() will be called to fill the secret key as I have removed it from the response value of other API's.
Also added a test in which a normal user tries to call the listKeysCmd() for the admin account and hence giving a permission denied exception.
---
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-9099: SecretKey is returned fr...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1152#discussion_r46775072
--- Diff: api/src/com/cloud/user/AccountService.java ---
@@ -136,4 +140,6 @@ void checkAccess(Account account, AccessType accessType, boolean sameOwner, Stri
*/
UserAccount getUserAccountById(Long userId);
+ public String[] getKeys(ListKeysCmd cmd);
--- End diff --
I agree with @jburwell
---
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-9099: SecretKey is returned fr...
Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1152#discussion_r48704472
--- Diff: server/src/com/cloud/user/AccountManager.java ---
@@ -198,4 +200,11 @@ void buildACLViewSearchCriteria(SearchCriteria<? extends ControlledViewEntity> s
public static final String MESSAGE_ADD_ACCOUNT_EVENT = "Message.AddAccount.Event";
public static final String MESSAGE_REMOVE_ACCOUNT_EVENT = "Message.RemoveAccount.Event";
+ public static final ConfigKey<Boolean> UseSecretKeyInResponse = new ConfigKey<Boolean>(
+ "Advanced",
+ Boolean.class,
+ "use.secret.key.in.response",
+ "true",
--- End diff --
@remibergsma It is a part of global config and the admin can change 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-9099: SecretKey is returned fr...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-202822605
@kansal did you get to this yet?
---
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-9099: SecretKey is returned fr...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1152#discussion_r46947711
--- Diff: api/src/org/apache/cloudstack/api/command/admin/user/ListKeysCmd.java ---
@@ -0,0 +1,74 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.cloudstack.api.command.admin.user;
+
+
+import com.cloud.user.Account;
+import com.cloud.user.User;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.response.RegisterResponse;
+import org.apache.cloudstack.api.response.UserResponse;
+
+import java.util.List;
+import java.util.logging.Logger;
+
+@APICommand(name = "listUserKeys",
+ description = "This command allows the user to query the seceret and API keys for the account",
+ responseObject = RegisterResponse.class,
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = true)
+
+public class ListKeysCmd extends BaseCmd{
+
+ @Parameter(name= ApiConstants.ID, type = CommandType.UUID, entityType = UserResponse.class, required = true, description = "ID of the user whose keys are required")
+ private Long id;
+
+ public static final Logger s_logger = Logger.getLogger(RegisterCmd.class.getName());
--- End diff --
how about using LOGGER as a name for this static final? would be more in line with standards
---
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-9099: SecretKey is returned fr...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-168570237
@DaanHoogland I complete agree with you regarding exposing credential information. The best practice when credentials are lost is to require that they be changed. This approach makes the access to the sensitive information obvious to all users -- making it impossible for an attacker to hide such a breach.
In the past, we have removed sensitive data from existing API responses. For example, for CVE-2015-3251, we removed exposure of KVM credentials from the [listHosts call](https://github.com/apache/cloudstack/pull/682). Therefore, as a project, we have previously determined that security should trump API backwards compatibility. It should most certainly be prioritized over making the task of integration testing easier.
---
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-9099: SecretKey is returned fr...
Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-168581991
cc @DaanHoogland @jburwell Okay. Agreed with that. So I am setting the default value to false but for running tests and maybe some other existing integration we will have to make that value to true. Is that fine? Of course we need to fix the existing test cases so that maybe from the next release we can get away with this thing completely?
---
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-9099: SecretKey is returned fr...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1152#discussion_r46649508
--- Diff: api/src/com/cloud/user/AccountService.java ---
@@ -136,4 +140,6 @@ void checkAccess(Account account, AccessType accessType, boolean sameOwner, Stri
*/
UserAccount getUserAccountById(Long userId);
+ public String[] getKeys(ListKeysCmd cmd);
--- End diff --
Why is the return type defined as an array and not a ``List<String>``?
---
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-9099: SecretKey is returned fr...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-162342597
@kansal looking forward to your update. your intended change makes sense
---
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-9099: SecretKey is returned fr...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-168200764
@kansal when you say 'I have deprecated that as many regressions were using the secret key from those APIs for authentication', I think we should adjust those regression test to set the setting to true. Let's not do consessions to security for the sake of testing.
---
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-9099: SecretKey is returned fr...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-202852425
@kansal as the complexity is unknown I would just go ahead and update the pr. We'll see the damage and think of fixes as we go. As for setting the value to true for existing tests, fine. as long as the default is false. the fix (setting it to true for some test cases) is trivial and will probably not be needed very often. Who is going to check a response for a private key unless they really need 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-9099: SecretKey is returned fr...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1152#issuecomment-216206238
@kansal can you rebase against latest master and share state of your PR, thanks
@DaanHoogland @jburwell do we still have outstanding issues on PR; do we want this or not? thanks
---
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.
---