Skip to content

Comments

Decrypt account and domain configurations when needed#9088

Merged
DaanHoogland merged 1 commit intoapache:4.19from
scclouds:fix-account-and-domain-encrypted-config-values
Jun 18, 2024
Merged

Decrypt account and domain configurations when needed#9088
DaanHoogland merged 1 commit intoapache:4.19from
scclouds:fix-account-and-domain-encrypted-config-values

Conversation

@BryanMLima
Copy link
Contributor

@BryanMLima BryanMLima commented May 16, 2024

Description

This PR fixes an issue when using the LDAP integration with ACS. PR #6812 normalized the account and domain configurations to only encrypt the values with configurations in the Hidden and Secure categories. However, that PR failed to address the use of these configurations. This resulted in using the encrypted value of the configuration, when it should decrypt it first, as observed in issue #8637.

This problem was fixed by adding the method getActualValue() that checks if the configurations in the Account and Domain are in the Hidden and Secure categories, and decrypting it, if needed.

NOTE: For users that manually set the configurations ldap.bind.password and ldap.truststore.password to a plain value in order to fix the faulty behaviour, it is required to store them encrypted. It will not be possible to update the configuration via UI, as an exception will be thrown when ACS tries to decrypt the plain value. To fix this, it is required to set the password again for ACS to encrypt it. There are two options:

  • Manually set the configuration via CloudMonkey, for example update configuration domainid=<domain-uuid> name="ldap.bind.password" value="password";
  • Or, removing the defined configuration through the database via the query DELETE from cloud.domain_details WHERE name like "%ldap%password%", and setting the configuration via UI for the affected domains.

Fixes #8637

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)
  • build/CI

Feature/Enhancement Scale or Bug Severity

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

How Has This Been Tested?

In a lab with version 4.19.0 installed, I configured an LDAP server and tried to create and LDAP account, which resulted in the following error LDAP: error code 49 - Invalid Credentials. After applying the changes in this patch, no exception was thrown, and the accounts in the LDAP server were correctly listed.

@BryanMLima BryanMLima added this to the 4.19.1.0 milestone May 16, 2024
@BryanMLima BryanMLima self-assigned this May 16, 2024
@BryanMLima
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@BryanMLima a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@codecov
Copy link

codecov bot commented May 16, 2024

Codecov Report

Attention: Patch coverage is 0% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 14.96%. Comparing base (ea9a0f4) to head (9f4d800).
Report is 15 commits behind head on 4.19.

Files Patch % Lines
...ava/com/cloud/domain/dao/DomainDetailsDaoImpl.java 0.00% 8 Missing ⚠️
...ain/java/com/cloud/user/AccountDetailsDaoImpl.java 0.00% 8 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               4.19    #9088      +/-   ##
============================================
- Coverage     14.96%   14.96%   -0.01%     
+ Complexity    10995    10985      -10     
============================================
  Files          5373     5373              
  Lines        469024   469036      +12     
  Branches      58818    60591    +1773     
============================================
- Hits          70197    70172      -25     
- Misses       391056   391096      +40     
+ Partials       7771     7768       -3     
Flag Coverage Δ
uitests 4.31% <ø> (ø)
unittests 15.66% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9624

@weizhouapache
Copy link
Member

thanks @BryanMLima
For users who have fixed the issue by using plain password in the database, do they need to roll back to the encrypted value?

@BryanMLima
Copy link
Contributor Author

thanks @BryanMLima For users who have fixed the issue by using plain password in the database, do they need to roll back to the encrypted value?

@weizhouapache, yes, an exception will be thrown as ACS will try to decrypt a plain value. Therefore, users that changed to a plain value will need to manually update the password, via CloudMonkey, for example.

@weizhouapache
Copy link
Member

thanks @BryanMLima For users who have fixed the issue by using plain password in the database, do they need to roll back to the encrypted value?

@weizhouapache, yes, an exception will be thrown as ACS will try to decrypt a plain value. Therefore, users that changed to a plain value will need to manually update the password, via CloudMonkey, for example.

thanks for the clarification @BryanMLima
if possible, can you add the steps how to update the value via cloudmonkey or mysql in the description, in case the users have already changed it to a plain value ?

@BryanMLima
Copy link
Contributor Author

thanks @BryanMLima For users who have fixed the issue by using plain password in the database, do they need to roll back to the encrypted value?

@weizhouapache, yes, an exception will be thrown as ACS will try to decrypt a plain value. Therefore, users that changed to a plain value will need to manually update the password, via CloudMonkey, for example.

thanks for the clarification @BryanMLima if possible, can you add the steps how to update the value via cloudmonkey or mysql in the description, in case the users have already changed it to a plain value ?

Sure, no problem, I added in the description of the PR. The same instructions could be present in the release notes when upgrading to version 4.19.1.0.

@weizhouapache
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9648

@weizhouapache
Copy link
Member

@blueorangutan test

@blueorangutan
Copy link

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

@DaanHoogland
Copy link
Contributor

NOTE: For users that manually set the configurations ldap.bind.password and ldap.truststore.password to a plain value in order to fix the faulty behaviour, it is required to store them encrypted. It will not be possible to update the configuration via UI, as an exception will be thrown when ACS tries to decrypt the plain value. To fix this, it is required to set the password again for ACS to encrypt it. There are two options:

* Manually set the configuration via CloudMonkey, for example `update configuration domainid=<domain-uuid> name="ldap.bind.password" value="password"`;

* Or, removing the defined configuration through the database via the query `DELETE from cloud.domain_details WHERE name like "%ldap%password%"`, and setting the configuration via UI for the affected domains.

I think we need to add this to the release notes @BryanMLima

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clgtm, would these constructs have to be added to other scopes as well? i.e. move the code to the GenercDaoBase !?

@blueorangutan
Copy link

[SF] Trillian test result (tid-10233)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 50587 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9088-t10233-kvm-centos7.zip
Smoke tests completed. 127 look OK, 4 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_01_events_resource Error 417.97 test_events_resource.py
test_02_trigger_shutdown Failure 341.70 test_safe_shutdown.py
test_01_secure_vm_migration Error 135.30 test_vm_life_cycle.py
test_01_secure_vm_migration Error 135.30 test_vm_life_cycle.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL Failure 612.40 test_vpc_redundant.py
test_02_redundant_VPC_default_routes Failure 982.59 test_vpc_redundant.py
test_05_rvpc_multi_tiers Failure 636.10 test_vpc_redundant.py
test_05_rvpc_multi_tiers Error 636.12 test_vpc_redundant.py

@BryanMLima
Copy link
Contributor Author

clgtm, would these constructs have to be added to other scopes as well? i.e. move the code to the GenercDaoBase !?

Currently, there is no need, as the changes are only in respect to the removal of the @Encrypt annotation from Account and Domain details tables in PR #6812.

@weizhouapache
Copy link
Member

@blueorangutan test rocky8 kvm-rocky8

@blueorangutan
Copy link

@weizhouapache a [SL] Trillian-Jenkins test job (rocky8 mgmt + kvm-rocky8) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-10256)
Environment: kvm-rocky8 (x2), Advanced Networking with Mgmt server r8
Total time taken: 48610 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9088-t10256-kvm-rocky8.zip
Smoke tests completed. 130 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_01_events_resource Error 414.81 test_events_resource.py

Copy link
Member

@weizhouapache weizhouapache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code lgtm
Not tested

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9763

@DaanHoogland
Copy link
Contributor

@blueorangutan test alma9 kvm-alma9

@blueorangutan
Copy link

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

@DaanHoogland DaanHoogland linked an issue Jun 3, 2024 that may be closed by this pull request
@blueorangutan
Copy link

[SF] Trillian test result (tid-10333)
Environment: kvm-alma9 (x2), Advanced Networking with Mgmt server a9
Total time taken: 54935 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9088-t10333-kvm-alma9.zip
Smoke tests completed. 130 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_list_system_vms_metrics_history Failure 0.23 test_metrics_api.py

@borisstoyanov borisstoyanov self-assigned this Jun 12, 2024
Copy link
Contributor

@borisstoyanov borisstoyanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, couldn't test it locally but I'm happy with your testing @BryanMLima, would it be possible to document the changes in a doc-pr?
https://github.com/apache/cloudstack-documentation

@BryanMLima
Copy link
Contributor Author

LGTM, couldn't test it locally but I'm happy with your testing @BryanMLima, would it be possible to document the changes in a doc-pr? https://github.com/apache/cloudstack-documentation

@borisstoyanov, thanks for the review. Regarding the doc-pr, you mean creating instructions for the release notes, right?

@DaanHoogland
Copy link
Contributor

LGTM, couldn't test it locally but I'm happy with your testing @BryanMLima, would it be possible to document the changes in a doc-pr? https://github.com/apache/cloudstack-documentation

@borisstoyanov, thanks for the review. Regarding the doc-pr, you mean creating instructions for the release notes, right?

Don't want to talk before my turn @BryanMLima , but yes, I am pretty sure that is what he means.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

Wrong ldap.bind.password after 4.19 upgrade

5 participants