Persist IP addresses related to VM access via CPVM

[bernardodemarco]

Description

This PR proposes to persist IP addresses related to VM access via CPVM. Specifically, it stores the IP address of the client accessing a VM and the IP address of the console endpoint creator. To achieve this, the cloud.console_session table was extended with two new columns: client_address, which stores the client's IP address, and console_endpoint_creator_address, which captures the IP address of the console endpoint creator.

These IP addresses were already being captured for logging and validation purposes. The console endpoint creator's IP is captured here, at the start of the CreateConsoleEndpointCmd execution:

cloudstack/api/src/main/java/org/apache/cloudstack/api/command/user/consoleproxy/CreateConsoleEndpointCmd.java

Lines 65 to 68 in 47a6b70

	@Override
	public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException, ResourceAllocationException, NetworkRuleConflictException {
	String clientAddress = getClientAddress();
	ConsoleEndpoint endpoint = consoleManager.generateConsoleEndpoint(vmId, extraSecurityToken, clientAddress);

The client address is captured by calling session.getRemoteAddress().getAddress().getHostAddress() in this method, that gets executed when the client connects to the console:

cloudstack/services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java

Lines 157 to 160 in 47a6b70

	private boolean checkSessionSourceIp(final Session session, final String sourceIP) throws IOException {
	// Verify source IP
	String sessionSourceIP = session.getRemoteAddress().getAddress().getHostAddress();
	logger.info("Get websocket connection request from remote IP : " + sessionSourceIP);

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
• test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Screenshots (if appropriate):

How Has This Been Tested?

1. I manually added the client_address and console_endpoint_creator_address columns to the cloud.console_session table.
2. I generated a VM console endpoint.
3. I executed the following query in order to validate whether the stored IP was correct.

SELECT * FROM `cloud`.`console_session` ORDER BY created DESC LIMIT 1;

id	uuid	created	account_id	user_id	instance_id	host_id	acquired	removed	client_address	console_endpoint_creator_address
2	77f08cea-8068-471f-9464-44810e1ef545	2024-08-06 17:57:05	2	2	14	1	NULL	NULL	NULL	192.168.122.1

As can be noticed, the console_endpoint_creator_address column was populated accordingly, whereas the client_adress was still empty since the VM had not been accessed yet.

4. I accessed the VM via CPVM.
5. I executed the following query in order to validate whether the client stored IP was correct.

SELECT * FROM `cloud`.`console_session` ORDER BY created DESC LIMIT 1;

id	uuid	created	account_id	user_id	instance_id	host_id	acquired	removed	client_address	console_endpoint_creator_address
2	77f08cea-8068-471f-9464-44810e1ef545	2024-08-06 17:57:05	2	2	14	1	NULL	NULL	192.168.122.1	192.168.122.1

As can be noticed, the IP address of the client that accessed the VM through CPVM was persisted correctly.

[bernardodemarco]

@blueorangutan package

[blueorangutan]

@bernardodemarco 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]

Codecov Report

Attention: Patch coverage is 7.04225% with 66 lines in your changes missing coverage. Please review.

Project coverage is 16.06%. Comparing base (27d2de1) to head (77530fc).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...ava/com/cloud/upgrade/dao/Upgrade42010to42100.java	10.52%	34 Missing ⚠️
...a/src/main/java/com/cloud/vm/ConsoleSessionVO.java	0.00%	12 Missing ⚠️
.../agent/api/ConsoleAccessAuthenticationCommand.java	0.00%	8 Missing ⚠️
...udstack/consoleproxy/ConsoleAccessManagerImpl.java	0.00%	5 Missing ⚠️
...nt/resource/consoleproxy/ConsoleProxyResource.java	0.00%	2 Missing ⚠️
...n/java/com/cloud/vm/dao/ConsoleSessionDaoImpl.java	0.00%	2 Missing ⚠️
...ain/java/com/cloud/consoleproxy/AgentHookBase.java	0.00%	2 Missing ⚠️
...main/java/com/cloud/consoleproxy/ConsoleProxy.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9534      +/-   ##
============================================
- Coverage     16.06%   16.06%   -0.01%     
- Complexity    12863    12865       +2     
============================================
  Files          5641     5642       +1     
  Lines        493791   493852      +61     
  Branches      59858    59860       +2     
============================================
+ Hits          79327    79331       +4     
- Misses       405683   405739      +56     
- Partials       8781     8782       +1     

Flag	Coverage Δ
uitests	4.02% <ø> (ø)
unittests	16.90% <7.04%> (-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]

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

[github-actions]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[DaanHoogland]

@bernardodemarco , I understand what and how, but have a hard time to get the why of this PR. Can you expand on that please?

[bernardodemarco]

@bernardodemarco , I understand what and how, but have a hard time to get the why of this PR. Can you expand on that please?

The main idea behind the proposed enhancement is to help operators better identify who accessed a virtual machine console.

The cloud.console_session table already stores data relating to who (account_id and user_id) accessed a given virtual machine (instance_id), as well as when that happened (created and removed). By adding the client_address and console_endpoint_creator_address fields, it'll be easier to identify who accessed the console, which can be extremely helpful for security and auditing purposes.

[DaanHoogland]

@bernardodemarco , I understand what and how, but have a hard time to get the why of this PR. Can you expand on that please?

The main idea behind the proposed enhancement is to help operators better identify who accessed a virtual machine console.

The cloud.console_session table already stores data relating to who (account_id and user_id) accessed a given virtual machine (instance_id), as well as when that happened (created and removed). By adding the client_address and console_endpoint_creator_address fields, it'll be easier to identify who accessed the console, which can be extremely helpful for security and auditing purposes.

thanks, makes sense.

[DaanHoogland]

@nvazquez can you have a look at this one?

[github-actions]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[nvazquez]

Code LGTM

[sureshanaparti]

clgtm

[DaanHoogland]

@blueorangutan package

[blueorangutan]

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

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

[github-actions]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[weizhouapache]

client_address is always NULL in my testing.

I tested the PR again, following the steps provided on the description, and the IP addresses were stored correctly.

It's important to remember that if there were any running CPVMs prior to the environment update, then they would need to be recreated in order to test if the client_address column is being populated correctly.

Furthermore, it's interesting taking a look at the CPVM logs. The following logs were generated during my tests, which indicates that the client IP 192.168.122.1 was captured correctly.

2024-09-10T17:19:38,330 INFO  [cloud.consoleproxy.ConsoleProxyNoVNCHandler] (qtp2090537387-40:[]) Verifying session
source IP 192.168.122.1 from WebSocket connection request.

2024-09-10T17:19:38,331 DEBUG [cloud.consoleproxy.ConsoleProxyNoVNCHandler] (qtp2090537387-40:[]) Session source IP
192.168.122.1 has been verified successfully.

@weizhouapache, if the problem continues happening, could you please describe your tests steps and share the CPVM logs?

Thanks @bernardodemarco
It works after patching the cpvm

[blueorangutan]

[SF] Trillian test result (tid-11441)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 50973 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9534-t11441-kvm-ol8.zip
Smoke tests completed. 139 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_migrate_VM_and_root_volume	Error	97.32	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	Error	50.60	test_vm_life_cycle.py
test_08_migrate_vm	Error	0.08	test_vm_life_cycle.py
test_01_migrate_vm_strict_tags_success	Error	0.23	test_vm_strict_host_tags.py
test_02_migrate_vm_strict_tags_failure	Error	0.23	test_vm_strict_host_tags.py

[DaanHoogland]

@JoaoJandre , it looks like this is ready. should we merge it before 4.20?

[JoaoJandre]

@JoaoJandre , it looks like this is ready. should we merge it before 4.20?

I think we should leave this for 4.21. We're only accepting bugfixes currently.

[BryanMLima]

CLGTM, I did not manually test it.

[BryanMLima]

@blueorangutan package

[blueorangutan]

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

[blueorangutan]

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

[github-actions]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[weizhouapache]

@blueorangutan package

[blueorangutan]

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

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

[bernardodemarco]

@weizhouapache should we run the CI once again for this one? The tests failures here #9534 (comment) don't seem to be related

[weizhouapache]

@bernardodemarco sure

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

[SF] Trillian test result (tid-11875)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 51503 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9534-t11875-kvm-ol8.zip
Smoke tests completed. 140 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_secure_vm_migration	Error	403.97	test_vm_life_cycle.py