Fix JsonSyntaxException when creating API command response #4355 by GabrielBrascher · Pull Request #4387 · apache/cloudstack

GabrielBrascher · 2020-10-06T21:02:58Z

Description

Added missing \" (from "*****" to "*****\"") at obfuscatePassword method from AsyncJobManagerImpl.java#L485

cloudstack/framework/jobs/src/main/java/org/apache/cloudstack/framework/jobs/impl/AsyncJobManagerImpl.java

Line 485 in 0f3f2a0

result = resp[0] + pattern + psswd.replace(psswd.substring(2, psswd.length() - 1), "*****") + "," + resp[1].split(",", 2)[1]; 

The lack of " caused a JsonSyntaxException exception when the host response had a password (e.g. if the host has out of band enabled e.g. IPMI or Redfish).

Fixes: #4355

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

How Has This Been Tested?

JUnit test
manual tests

GabrielBrascher · 2020-10-06T23:39:31Z

@blueorangutan package

blueorangutan · 2020-10-06T23:40:01Z

@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-10-07T00:18:45Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2140

DaanHoogland

lgtm

DaanHoogland · 2020-10-07T17:26:42Z

@blueorangutan test

blueorangutan · 2020-10-07T17:28:05Z

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2020-10-08T02:12:26Z

Trillian test result (tid-2910)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 29745 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4387-t2910-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermittent failure detected: /marvin/tests/smoke/test_outofbandmanagement_nestedplugin.py
Intermittent failure detected: /marvin/tests/smoke/test_outofbandmanagement.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 81 look OK, 4 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestDeployVirtioSCSIVM>:setup	`Error`	0.00	test_deploy_virtio_scsi_vm.py
test_oobm_issue_power_cycle	`Error`	2.17	test_outofbandmanagement_nestedplugin.py
test_oobm_issue_power_off	`Error`	1.16	test_outofbandmanagement_nestedplugin.py
test_oobm_issue_power_on	`Error`	1.16	test_outofbandmanagement_nestedplugin.py
test_oobm_issue_power_reset	`Error`	2.16	test_outofbandmanagement_nestedplugin.py
test_oobm_issue_power_soft	`Error`	2.18	test_outofbandmanagement_nestedplugin.py
test_oobm_issue_power_status	`Error`	2.17	test_outofbandmanagement_nestedplugin.py
test_oobm_enabledisable_across_clusterzones	`Error`	20.11	test_outofbandmanagement.py
test_oobm_issue_power_cycle	`Error`	4.29	test_outofbandmanagement.py
test_oobm_issue_power_off	`Error`	4.25	test_outofbandmanagement.py
test_oobm_issue_power_on	`Error`	4.24	test_outofbandmanagement.py
test_oobm_issue_power_reset	`Error`	3.25	test_outofbandmanagement.py
test_oobm_issue_power_soft	`Error`	3.26	test_outofbandmanagement.py
test_oobm_issue_power_status	`Error`	4.25	test_outofbandmanagement.py
test_oobm_zchange_password	`Error`	5.32	test_outofbandmanagement.py
test_hostha_kvm_host_degraded	`Error`	4.43	test_hostha_kvm.py
test_hostha_kvm_host_fencing	`Error`	4.45	test_hostha_kvm.py
test_hostha_kvm_host_recovering	`Error`	7.52	test_hostha_kvm.py

DaanHoogland · 2020-10-09T07:47:11Z

didn't work @GabrielBrascher

======================================================================
ERROR: Tests degraded HA state when agent is stopped/killed
----------------------------------------------------------------------
Traceback (most recent call last):
File "/marvin/tests/smoke/test_hostha_kvm.py", line 355, in test_hostha_kvm_host_degraded
self.assertIssueCommandState('ON', 'On')
File "/marvin/tests/smoke/test_hostha_kvm.py", line 493, in assertIssueCommandState
self.issuePowerActionCmd(command)
File "/marvin/tests/smoke/test_hostha_kvm.py", line 539, in issuePowerActionCmd
raise e
Exception: Job failed: {jobprocstatus : 0, created : u'2020-10-08T02:10:21+0000', completed : u'2020-10-08T02:10:21+0000', cmd : u'org.apache.cloudstack.api.command.admin.outofbandmanagement.IssueOutOfBandManagementPowerActionCmd', userid : u'a8138930-08c3-11eb-8574-1e00ca0138c9', jobstatus : 2, jobid : u'f8439839-f821-4bd6-b7a3-820345992038', jobresultcode : 530, jobresulttype : u'object', jobresult : {errorcode : 530, errortext : u'com.google.gson.stream.MalformedJsonException: Unterminated object near "password":"p*****"","action":"ON","desc'}, accountid : u'a812be97-08c3-11eb-8574-1e00ca0138c9'}

note that in this case the replacement yield a double double quote ("password":"p*****""), which is one in excess.

GabrielBrascher · 2020-10-09T16:00:16Z

@DaanHoogland what about removing the password from the JSON response?

DaanHoogland · 2020-10-14T07:57:27Z

@GabrielBrascher If sometimes an extra '"' is added and sometimes not, this is still going to be an issue if you try to remove it. I'd rather replace the exact "something" with "*" or "s". it seems that we are brushing a bug under the carpet otherwise.

yadvr · 2020-10-14T10:13:32Z

 String[] resp = result.split(pattern);
 String psswd = resp[1].toString().split(",")[0];
- result = resp[0] + pattern + psswd.replace(psswd.substring(2, psswd.length() - 1), "*****") + "," + resp[1].split(",", 2)[1];
+ result = resp[0] + pattern + psswd.replace(psswd.substring(2, psswd.length() - 1), "*****\"") + "," + resp[1].split(",", 2)[1];


Why not complete replace with the ****?

I am not sure if replacing the password with **** would then affect the intention of the password response.
I think that the idea was to offer admins a hint on the password.
But that is good to keep in mind, thanks for the heads up @rhtyd. What do you think @DaanHoogland?

GabrielBrascher · 2020-10-14T14:19:34Z

@DaanHoogland I proposed removing the password because I don't see much of use in having it on the response, it seems that does not aggregate relevant information.

DaanHoogland · 2020-10-14T14:27:35Z

It is adding value but only if having a password is optional, @GabrielBrascher. And i think hiding it should be optional as well as a private cloud operator might opt for having it visible. I am not going to argue that anymore, btw.

GabrielBrascher · 2020-10-14T15:04:14Z

@DaanHoogland fair enough, I am going to re-check these flows to propose a fix keeping the response as it is operating on a consistent password.

GabrielBrascher · 2020-10-14T21:04:05Z

@DaanHoogland I found out why some cases the regex failed and some not. Updated code to fix it (for good, I hope). Also added test case addressing both situations.

The regex uses comma to set the split separator: String psswd = resp[1].toString().split(",")[0];
This works on cases such as:

...,"password":"password","action":"OFF"...

However, the specific case that I got with OOBM it has the following JSON String:

"username":"root","password":"password"},"resourcestate":"PrepareForMaintenance","hahost":false

With that said, the code has been updated to deal with both cases: "password":"password"}, and "password":"password",.

GabrielBrascher · 2020-10-14T21:04:43Z

@blueorangutan package

blueorangutan · 2020-10-14T21:05:31Z

@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-10-14T21:58:35Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2180

DaanHoogland · 2020-10-15T12:02:36Z

@blueorangutan test

blueorangutan · 2020-10-15T12:03:35Z

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2020-10-15T23:04:40Z

Trillian test result (tid-2969)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37791 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4387-t2969-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 85 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

DaanHoogland · 2020-10-16T07:18:25Z

@rhtyd no further comments?

GabrielBrascher added 2 commits October 6, 2020 17:40

Add missing \" on obfuscatePassword
bd27580

Add tests for AsyncJobManagerImpl.obfuscatePassword
b04b08b

GabrielBrascher mentioned this pull request Oct 6, 2020
JsonSyntaxException when creating API command response #4355
Closed

GabrielBrascher requested a review from DaanHoogland October 7, 2020 10:46

DaanHoogland approved these changes Oct 7, 2020
View reviewed changes

GabrielBrascher added this to the 4.15.0.0 milestone Oct 7, 2020

GabrielBrascher self-assigned this Oct 7, 2020

GabrielBrascher added the type:bug label Oct 7, 2020

yadvr reviewed Oct 14, 2020
View reviewed changes

fix for "password},"
7ef2f27

DaanHoogland requested a review from yadvr October 20, 2020 09:54

yadvr approved these changes Oct 20, 2020
View reviewed changes

yadvr merged commit 1ba6a49 into apache:masterOct 20, 2020

davidjumani mentioned this pull request Oct 21, 2020
Fixing count for findHostsForMigration #4375
Merged
5 tasks

DaanHoogland added the Severity:Major label Nov 10, 2020

Uh oh!

Conversation

GabrielBrascher commented Oct 6, 2020

Description

Types of changes

How Has This Been Tested?

Uh oh!

GabrielBrascher commented Oct 6, 2020

Uh oh!

blueorangutan commented Oct 6, 2020

Uh oh!

blueorangutan commented Oct 7, 2020

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Oct 7, 2020

Uh oh!

blueorangutan commented Oct 7, 2020

Uh oh!

blueorangutan commented Oct 8, 2020

Uh oh!

DaanHoogland commented Oct 9, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GabrielBrascher commented Oct 9, 2020

Uh oh!

DaanHoogland commented Oct 14, 2020

Uh oh!

yadvrOct 14, 2020

Choose a reason for hiding this comment

Uh oh!

GabrielBrascherOct 14, 2020

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher commented Oct 14, 2020

Uh oh!

DaanHoogland commented Oct 14, 2020

Uh oh!

GabrielBrascher commented Oct 14, 2020

Uh oh!

GabrielBrascher commented Oct 14, 2020

Uh oh!

GabrielBrascher commented Oct 14, 2020

Uh oh!

blueorangutan commented Oct 14, 2020

Uh oh!

blueorangutan commented Oct 14, 2020

Uh oh!

DaanHoogland commented Oct 15, 2020

Uh oh!

blueorangutan commented Oct 15, 2020

Uh oh!

blueorangutan commented Oct 15, 2020

Uh oh!

DaanHoogland commented Oct 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DaanHoogland commented Oct 9, 2020•
edited
Loading