Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Externalize tls version and security protocols configuration on mail sending#5119

Merged
nvazquez merged 2 commits intoapache:mainapache/cloudstack:mainfrom
GutoVeronezi:externalize-tls-version-and-security-protocols-configuration-on-mail-sendingGutoVeronezi/cloudstack:externalize-tls-version-and-security-protocols-configuration-on-mail-sendingCopy head branch name to clipboard
Jul 21, 2021
Merged

Externalize tls version and security protocols configuration on mail sending#5119
nvazquez merged 2 commits intoapache:mainapache/cloudstack:mainfrom
GutoVeronezi:externalize-tls-version-and-security-protocols-configuration-on-mail-sendingGutoVeronezi/cloudstack:externalize-tls-version-and-security-protocols-configuration-on-mail-sendingCopy head branch name to clipboard

Conversation

@GutoVeronezi
Copy link
Contributor

@GutoVeronezi GutoVeronezi commented Jun 16, 2021

Description

  • Some servers (such as Microsoft exchange) are deprecating TLSv1 and TLSv1.1. On alert's and project's mailing settings, ACS uses the protocol TLSv1; therefore, operators do not have options to choose which version they want to use.

  • ACS has configurations (\*.smtp.useAuth) to inform if it will use secure SMTP authentication when sending emails; However, this configuration only refers to the use of SSL. Operators have no means to choose if they want to use StartTLS option.

This PR intends to externalize the protocol's and StartTLS settings on alert's and project's mail sending. Alert and project (also quota) send email through the class SMTPMailSender, inserted in PR 4954, which will handle these configurations.

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

How Has This Been Tested?

It was tested in a test lab.

I set SMTP global settings to project and alert and enabled StartTLS (global setting); Then I allowed Less secure app access on my Google account and set the following settings:

server address: smtp.gmail.com
username: <gmail address>
password: <gmail password>
port (TLS): 587
  • To alert, I started mgmt;
  • To project, I enabled global setting project.invite.required and added a user via email;

In Gmail, we have an option to show original message, which provides the TLS version and the cipher. So I changed the configuration a few times and sent some alerts and invitations to verify if both versions, in configuration and Gmail, were the same.

@yadvr
Copy link
Member

yadvr commented Jun 18, 2021

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Jun 18, 2021

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

@blueorangutan
Copy link

blueorangutan commented Jun 18, 2021

Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 275

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Jun 18, 2021

@GutoVeronezi It makes sense to make this configuratble, but should we allow it to be different for alert and project? seems like it is a true global setting.

@blueorangutan
Copy link

blueorangutan commented Jun 18, 2021

Trillian Build Failed (tid-1005)

@GutoVeronezi
Copy link
Contributor Author

GutoVeronezi commented Jun 18, 2021

@GutoVeronezi It makes sense to make this configuratble, but should we allow it to be different for alert and project? seems like it is a true global setting.

@DaanHoogland I am following the way mail settings are implemented today; each scope (quota, alert and project) has its own settings. Although, I don't see any plausible reason to keep settings to each scope (it seems that an operator could use different providers to send mail is a remote probability).

I think that if we turn these global, we should turn those too, to maintain the same pattern.

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Jun 18, 2021

I think that if we turn these global, we should turn those too, to maintain the same pattern.

makes sense.

@GutoVeronezi
Copy link
Contributor Author

GutoVeronezi commented Jun 28, 2021

Can anyone review this?

RodrigoDLopez
RodrigoDLopez approved these changes Jun 28, 2021
View reviewed changes
Copy link
Contributor

@RodrigoDLopez RodrigoDLopez left a comment

Choose a reason for hiding this comment

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

Nice work @GutoVeronezi
The Code LGTM, i will try run some manual tests. But i'm not sure if i can do this in my env.

@yadvr
Copy link
Member

yadvr commented Jul 15, 2021

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Jul 15, 2021

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

@blueorangutan
Copy link

blueorangutan commented Jul 15, 2021

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 550

DaanHoogland
DaanHoogland approved these changes Jul 16, 2021
View reviewed changes
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

@sureshanaparti sureshanaparti added this to the 4.16.0.0 milestone Jul 16, 2021
@GutoVeronezi
Copy link
Contributor Author

GutoVeronezi commented Jul 20, 2021

@DaanHoogland @RodrigoDLopez is there anything else to do?

@blueorangutan
Copy link

blueorangutan commented Jul 20, 2021

Trillian Build Failed (tid-1324)

@blueorangutan
Copy link

blueorangutan commented Jul 20, 2021

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 599

@nvazquez
Copy link
Contributor

nvazquez commented Jul 21, 2021

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Jul 21, 2021

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

nvazquez
nvazquez approved these changes Jul 21, 2021
View reviewed changes
Copy link
Contributor

@nvazquez nvazquez left a comment

Choose a reason for hiding this comment

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

LGTM

@nvazquez
Copy link
Contributor

nvazquez commented Jul 21, 2021

@GutoVeronezi as the PR is only exposing config keys for existing properties I think a round of tests will be enough as there are no changes in the functionality

@blueorangutan
Copy link

blueorangutan commented Jul 21, 2021

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 608

@nvazquez
Copy link
Contributor

nvazquez commented Jul 21, 2021

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Jul 21, 2021

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

GabrielBrascher
GabrielBrascher approved these changes Jul 21, 2021
View reviewed changes
Copy link
Member

@GabrielBrascher GabrielBrascher 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

@blueorangutan
Copy link

blueorangutan commented Jul 21, 2021

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

Test Result Time (s) Test File
test_07_deploy_kubernetes_ha_cluster Failure 3614.11 test_kubernetes_clusters.py
test_08_deploy_and_upgrade_kubernetes_ha_cluster Failure 0.06 test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster Failure 0.04 test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown Error 83.57 test_kubernetes_clusters.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL Failure 545.96 test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers Failure 421.73 test_vpc_redundant.py

@nvazquez
Copy link
Contributor

nvazquez commented Jul 21, 2021

Merging based on approvals and test results as failures are intermitent issues

@nvazquez nvazquez merged commit eb3acc3 into apache:main Jul 21, 2021
@blueorangutan
Copy link

blueorangutan commented Jul 22, 2021

Trillian test result (tid-1331)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 83146 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5119-t1331-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_network.py
Intermittent failure detected: /marvin/tests/smoke/test_reset_vm_on_reboot.py
Intermittent failure detected: /marvin/tests/smoke/test_resource_accounting.py
Intermittent failure detected: /marvin/tests/smoke/test_router_dhcphosts.py
Intermittent failure detected: /marvin/tests/smoke/test_router_dns.py
Intermittent failure detected: /marvin/tests/smoke/test_router_dnsservice.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_iptables_default_policy.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_routers.py
Intermittent failure detected: /marvin/tests/smoke/test_service_offerings.py
Intermittent failure detected: /marvin/tests/smoke/test_snapshots.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_volumes.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 79 look OK, 10 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
ContextSuite context=TestResetVmOnReboot>:setup Error 0.00 test_reset_vm_on_reboot.py
ContextSuite context=TestRAMCPUResourceAccounting>:setup Error 0.00 test_resource_accounting.py
ContextSuite context=TestRouterDns>:setup Error 0.00 test_router_dns.py
ContextSuite context=TestRouterDnsService>:setup Error 0.00 test_router_dnsservice.py
ContextSuite context=TestRouterIpTablesPolicies>:setup Error 0.00 test_routers_iptables_default_policy.py
ContextSuite context=TestVPCIpTablesPolicies>:setup Error 0.00 test_routers_iptables_default_policy.py
ContextSuite context=TestIsolatedNetworks>:setup Error 0.00 test_routers_network_ops.py
ContextSuite context=TestRedundantIsolateNetworks>:setup Error 0.00 test_routers_network_ops.py
ContextSuite context=TestRouterServices>:setup Error 0.00 test_routers.py
ContextSuite context=TestCpuCapServiceOfferings>:setup Error 0.00 test_service_offerings.py
ContextSuite context=TestServiceOfferings>:setup Error 0.12 test_service_offerings.py
ContextSuite context=TestSnapshotRootDisk>:setup Error 0.00 test_snapshots.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers Failure 532.16 test_vpc_redundant.py

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

Reviewers

@DaanHoogland DaanHoogland DaanHoogland approved these changes

@GabrielBrascher GabrielBrascher GabrielBrascher approved these changes

@nvazquez nvazquez nvazquez approved these changes

+1 more reviewer

@RodrigoDLopez RodrigoDLopez RodrigoDLopez approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Severity:Minor type:enhancement

Projects

None yet

Milestone

4.16.0.0

Development

Successfully merging this pull request may close these issues.

8 participants

@GutoVeronezi @yadvr @blueorangutan @DaanHoogland @nvazquez @GabrielBrascher @RodrigoDLopez @sureshanaparti
Morty Proxy This is a proxified and sanitized view of the page, visit original site.