Skip to content

Secure KVM VNC Console Access Using the CA Framework #7015

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Jan 27, 2023

Conversation

nvazquez
Copy link
Contributor

Description

This PR allows securing the console access through CloudStack to the virtual machines running on KVM. The secure access is achieved through the generated certificates for the CA Framework in CloudStack, that provides mutual TLS connections between agents. These certificates are used to also secure the connection between the console proxies and the VNC ports for VM console access.

This feature is only supported on the KVM hypervisor

Design Document: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Secure+KVM+VNC+connection+using+the+CA+framework

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

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

image

How Has This Been Tested?

Tested on KVM environment, enabling TLS on VNC

@nvazquez
Copy link
Contributor Author

@blueorangutan package

@apache apache deleted a comment from blueorangutan Dec 22, 2022
@apache apache deleted a comment from blueorangutan Dec 22, 2022
@blueorangutan
Copy link

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

@codecov
Copy link

codecov bot commented Dec 22, 2022

Codecov Report

Merging #7015 (e75a69a) into main (c0a32d0) will increase coverage by 0.01%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main    #7015      +/-   ##
============================================
+ Coverage     11.77%   11.78%   +0.01%     
- Complexity     7662     7665       +3     
============================================
  Files          2503     2505       +2     
  Lines        245958   246029      +71     
  Branches      38374    38382       +8     
============================================
+ Hits          28953    28986      +33     
- Misses       213240   213272      +32     
- Partials       3765     3771       +6     
Impacted Files Coverage Δ
...ava/com/cloud/servlet/ConsoleProxyClientParam.java 0.00% <0.00%> (ø)
...udstack/consoleproxy/ConsoleAccessManagerImpl.java 5.21% <0.00%> (-0.34%) ⬇️
...com/cloud/agent/manager/ConnectedAgentAttache.java 25.00% <0.00%> (-12.50%) ⬇️
...m/resource/wrapper/LibvirtReadyCommandWrapper.java 61.53% <0.00%> (-11.19%) ⬇️
...dstack/network/contrail/model/ModelObjectBase.java 21.15% <0.00%> (-7.70%) ⬇️
...n/java/com/cloud/storage/VolumeApiServiceImpl.java 12.96% <0.00%> (-0.03%) ⬇️
...oud/hypervisor/vmware/mo/HypervisorHostHelper.java 15.54% <0.00%> (-0.02%) ⬇️
...oud/hypervisor/vmware/resource/VmwareResource.java 4.50% <0.00%> (-0.01%) ⬇️
...m/cloud/hypervisor/vmware/mo/VirtualMachineMO.java 0.93% <0.00%> (-0.01%) ⬇️
... and 12 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

clggtm, just some remarks on structure and logging.
needs extensive testing though, might be good to have in 4.18

Comment on lines +91 to +103
if (manager == null) {
if (socketHandler.readUnsignedInteger(8) == 0) {
int result = socketHandler.readUnsignedInteger(32);
String reason;
if (result == RfbConstants.VNC_AUTH_FAILED || result == RfbConstants.VNC_AUTH_TOO_MANY) {
reason = socketHandler.readString();
} else {
reason = "Authentication failure (protocol error)";
}
throw new CloudRuntimeException(reason);
}
setParam();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

move to a handleErrorState() type of method?

@DaanHoogland DaanHoogland added this to the 4.18.0.0 milestone Dec 22, 2022
@nvazquez
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@nvazquez a 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: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 5057

@DaanHoogland
Copy link
Contributor

SonarCloud Quality Gate failed. Quality Gate failed

Bug A 0 Bugs Vulnerability A 0 Vulnerabilities Security Hotspot E 2 Security Hotspots Code Smell A 38 Code Smells

0.3% 0.3% Coverage 0.4% 0.4% Duplication

@nvazquez please go through these.

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.

overall code LGTM
left few minor comments

great job @nvazquez !
I'd learn these codes when I have time :-D

@@ -471,22 +474,31 @@ const UI = {
clearTimeout(UI.statusTimeout);

switch (statusType) {
case 'encrypted':
Copy link
Member

Choose a reason for hiding this comment

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

we need to re-apply these changes when upgrade novnc.
not a big issue, just need to pay a bit more attention

@nvazquez
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@nvazquez a 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.

@nvazquez nvazquez marked this pull request as ready for review January 19, 2023 13:04
@nvazquez
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@nvazquez a 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.

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.4% 0.4% Coverage
0.5% 0.5% Duplication

@blueorangutan
Copy link

Packaging result: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 5340

@nvazquez
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@nvazquez a 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: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 5347

@DaanHoogland
Copy link
Contributor

@borisstoyanov , you approved based on manual testing, am i right?

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@nvazquez nvazquez marked this pull request as draft January 20, 2023 14:29
@blueorangutan
Copy link

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

@blueorangutan
Copy link

Trillian test result (tid-5929)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 43845 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7015-t5929-kvm-centos7.zip
Smoke tests completed. 106 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_08_upgrade_kubernetes_ha_cluster Failure 567.45 test_kubernetes_clusters.py

@rohityadavcloud rohityadavcloud marked this pull request as ready for review January 27, 2023 10:19
@rohityadavcloud
Copy link
Member

LGTM, tested this in an old env which is upgraded to this feature. The old VM (ssvm, cpvm, VR etc) had unencrypted vnc console; on stop/start they were shown as encrypted. I also tested the provisionCertificate API and read the documentation notes and cross-checked all requirements.
Screenshot 2023-01-27 at 5 21 14 PM

@rohityadavcloud
Copy link
Member

Merging this based on review, testing and smoketests. cc @DaanHoogland

@rohityadavcloud rohityadavcloud merged commit eac357c into apache:main Jan 27, 2023
@rohityadavcloud rohityadavcloud deleted the kvm-secure-console-access branch January 27, 2023 11:52
@GutoVeronezi
Copy link
Contributor

Just for the record, I have tested both access (encrypted and unencrypted) and it is working fine:

  • with:
    image

  • without:
    image

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

Successfully merging this pull request may close these issues.

9 participants