Skip to content

novnc: Add source IP check#4736

Merged
yadvr merged 3 commits into
apache:4.15from
ustcweizhou:4.16-novnc-sourceip-check
Mar 6, 2021
Merged

novnc: Add source IP check#4736
yadvr merged 3 commits into
apache:4.15from
ustcweizhou:4.16-novnc-sourceip-check

Conversation

@ustcweizhou

Copy link
Copy Markdown
Contributor

Description

This PR adds a global setting "novnc.console.sourceip.check.enabled", it is false by default. If it is true, The source IP to access novnc console must be same as the IP in request to management server for console URL.

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

Screenshots (if appropriate):

How Has This Been Tested?

Open a vm console,
view page source and get exact URL in ifram
access the URL from other server/ip

Current behavior:
succeeds.

New behavior
if novnc.console.sourceip.check.enabled is "false", succeeds.
if novnc.console.sourceip.check.enabled is "true", fails.

@yadvr

yadvr commented Mar 1, 2021

Copy link
Copy Markdown
Member

This looks like a good security fix, can we get this in 4.15 if applicable @weizhouapache ?

@weizhouapache

Copy link
Copy Markdown
Member

This looks like a good security fix, can we get this in 4.15 if applicable @weizhouapache ?

@rhtyd
yeah, if we agree this is a security fix , I will rebase this pr with 4.15.
@DaanHoogland @shwstppr @davidjumani your opinions ?

@yadvr yadvr added this to the 4.15.1.0 milestone Mar 1, 2021
@yadvr yadvr changed the base branch from master to 4.15 March 1, 2021 10:04
@yadvr yadvr changed the base branch from 4.15 to master March 1, 2021 10:04
@shwstppr

shwstppr commented Mar 1, 2021

Copy link
Copy Markdown
Contributor

+1 for having it in 4.15

@shwstppr shwstppr requested a review from davidjumani March 1, 2021 10:39

@DaanHoogland DaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

two bits i'd like to see in separate methods, but code looks good

@DaanHoogland

Copy link
Copy Markdown
Contributor

+1 for moving to 4.15

@yadvr

yadvr commented Mar 2, 2021

Copy link
Copy Markdown
Member

@weizhouapache can you change base branch of the PR and rebase to 4.15?

@ustcweizhou ustcweizhou force-pushed the 4.16-novnc-sourceip-check branch from ec3c8e5 to 892b898 Compare March 2, 2021 09:11
@weizhouapache

Copy link
Copy Markdown
Member

@weizhouapache can you change base branch of the PR and rebase to 4.15?

@rhtyd done.

@weizhouapache weizhouapache changed the base branch from master to 4.15 March 2, 2021 09:12
@yadvr

yadvr commented Mar 2, 2021

Copy link
Copy Markdown
Member

thnx @weizhouapache
@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

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

@yadvr

yadvr commented Mar 2, 2021

Copy link
Copy Markdown
Member

@blueorangutan test

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

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

Test Result Time (s) Test File
test_01_migrate_VM_and_root_volume Error 63.19 test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks Error 48.09 test_vm_life_cycle.py

@GabrielBrascher GabrielBrascher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code LGTM.

@yadvr

yadvr commented Mar 3, 2021

Copy link
Copy Markdown
Member

@shwstppr can you do manual novnc check with a built env to see if novnc work (basic checks wrt this PR).

@shwstppr

shwstppr commented Mar 5, 2021

Copy link
Copy Markdown
Contributor

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

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

Test Result Time (s) Test File
test_02_migrate_VM_with_two_data_disks Error 48.10 test_vm_life_cycle.py

@davidjumani davidjumani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Would it be a good idea to inform the user why the connection failed ?

@yadvr yadvr merged commit df4103f into apache:4.15 Mar 6, 2021
DaanHoogland pushed a commit to shapeblue/cloudstack that referenced this pull request May 20, 2022
* novnc: Add client IP check for novnc console in cloudstack 4.16

* novnc ip check : Fix restart CPVM or mgt server does not update novnc param

* novnc ip check: move to method
DaanHoogland pushed a commit to shapeblue/cloudstack that referenced this pull request May 20, 2022
* novnc: Add client IP check for novnc console in cloudstack 4.16

* novnc ip check : Fix restart CPVM or mgt server does not update novnc param

* novnc ip check: move to method
shwstppr pushed a commit to shapeblue/cloudstack that referenced this pull request Jan 17, 2023
* novnc: Add client IP check for novnc console in cloudstack 4.16

* novnc ip check : Fix restart CPVM or mgt server does not update novnc param

* novnc ip check: move to method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants