Handle console session in multiple management servers#7094
Conversation
GutoVeronezi
requested review from
DaanHoogland,
borisstoyanov,
nvazquez and
yadvr
engine/schema/src/main/java/com/cloud/vm/AllowedConsoleSessionVo.java
Outdated
Show resolved
Hide resolved
engine/schema/src/main/resources/META-INF/db/schema-41720to41800.sql
Outdated
Show resolved
Hide resolved
engine/schema/src/main/resources/META-INF/db/schema-41720to41800.sql
Outdated
Show resolved
Hide resolved
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
except for the name part 'allowed', which seems to be immplied in any case, looks good.
Also I am still not comfortable approving logs that are
- debug
- expanding parameters
- not having a guard with isDebugEnabled()
You have an PR in the making that will deal with that, can you share something about that?
engine/schema/src/main/java/com/cloud/vm/AllowedConsoleSessionVo.java
Outdated
Show resolved
Hide resolved
|
@blueorangutan package |
|
@DaanHoogland 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. |
|
Packaging result: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 5269 |
|
@blueorangutan test matrix |
|
@DaanHoogland a Trillian-Jenkins matrix job (centos7 mgmt + xenserver71, rocky8 mgmt + vmware67u3, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests |
@DaanHoogland, regarding the PR, we are working to open it today. As soon as it is opened, we ping you. Edit 1: it will take a bit more than expected (one day or two); |
|
@GutoVeronezi I've tested it on a single mgmt server and I'm not able to access any VM console, could you please verify? On each case I get this error from the CPVM logs: |
…cts according to the table name
|
Trillian test result (tid-5853)
|
|
Trillian test result (tid-5855)
|
|
Trillian test result (tid-5854)
|
| @@ -151,11 +156,16 @@ public boolean isSessionAllowed(String sessionUuid) { | |||
|
|
|||
There was a problem hiding this comment.
@GutoVeronezi the problem listed previously for one mgmt server comes from the method isSessionAllowed that does not query the database for the session UUID and always returns false, therefore the ConsoleAccessAuthenticationAnswer is false
Codecov Report
@@ Coverage Diff @@
## main #7094 +/- ##
============================================
+ Coverage 11.76% 11.78% +0.01%
- Complexity 7661 7668 +7
============================================
Files 2503 2505 +2
Lines 245958 246021 +63
Branches 38374 38381 +7
============================================
+ Hits 28946 28988 +42
- Misses 213248 213261 +13
- Partials 3764 3772 +8
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
SonarCloud Quality Gate failed. |
|
|
||
| protected void persistConsoleSession(String sessionUuid, long instanceId, long hostId) { | ||
| ConsoleSessionVO consoleSessionVo = new ConsoleSessionVO(); | ||
| consoleSessionVo.setUuid(sessionUuid); |
There was a problem hiding this comment.
these lines can be replaced by a constructor
There was a problem hiding this comment.
On principle, I would rather not create a constructor for informing the object properties, as the more properties it has, the longer the constructor would get, becoming harder to read/interpret the code. For Python, that supports keyword arguments, this approach works fine, though.
engine/schema/src/main/resources/META-INF/db/schema-41720to41800.sql
Outdated
Show resolved
Hide resolved
|
@GutoVeronezi thanks for the refactor, you are right I have now checked that the lastUsedTime on the ConsoleProxyLoadReportCommand keeps being updated even when the session is not used, that makes the session not to timeout |
yadvr
left a comment
yadvr
left a comment
There was a problem hiding this comment.
Lgtm, the license header has extra newlines compared to existing files. Didn’t test it.
|
@blueorangutan package |
|
@rohityadavcloud a Jenkins job has been kicked to build packages. It will be bundled with SystemVM template(s). I'll keep you posted as I make progress. |
|
Packaging result: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 5300 |
|
@blueorangutan test matrix |
|
@DaanHoogland a Trillian-Jenkins matrix job (centos7 mgmt + xenserver71, rocky8 mgmt + vmware67u3, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests |
|
Trillian test result (tid-5890)
|
|
Trillian test result (tid-5888)
|
|
Trillian test result (tid-5889)
|
|
@blueorangutan test rocky8 vmware-67u3 |
|
@DaanHoogland a Trillian-Jenkins test job (rocky8 mgmt + vmware-67u3) has been kicked to run smoke tests |
|
Trillian test result (tid-5906)
|
nvazquez
left a comment
nvazquez
left a comment
There was a problem hiding this comment.
LGTM manually tested on KVM Rocky 8 env with 2 management servers
|
Nice job @GutoVeronezi - I saw this issue as well earlier. Regarding the |
|
@mlsorensen, as this data can be used for auditing access to the consoles, I do not see a problem with keeping them; however, the data may become obsolete or operators may not want to store them. Your proposal for a cleanup task is a good idea. We can implement it with customized intervals and retention, just like other tasks ACS has. @DaanHoogland, do we still have time to implement this feature in 4.18 or it is already in freeze? |
|
@DaanHoogland @GutoVeronezi I've created the PR: #7132, can you please review? |
@GutoVeronezi , I am quite lenient with freeze. I wanted to create the first RC on monday but a couple of PR do not seem to make it. Most noticably tungsten, which I really want in, albeit as experimental feature. |
Yes, I agree it could be useful for audit. I am mostly thinking about size, a decent cloudstack install could have thousands of console requests a day. I think mysql could handle it, but it is like a log that never rolls. Probably not an immediate issue. |
7 participants
Description
PR #6577 introduced a new mechanism of validation of console sessions, which makes the access tokens one time usable in order to avoid reply attacks and prevent access to the console in case of data leak. When generating the console token, the management server (MS) that processed the request saves it in memory for further validation. When using the token, a request is sent to the CPVM that will validate it (the token) against the MS.
In an environment that has more than one MS, the CPVM will establish communication with the first available MS that it founds. In this case, if the CPVM communicates with
MS Aand the console token is generated byMS A, the token will work; however, if the CPVM communicates withMS Aand the console token is generated byMS B, the token will not work because the token is being validated againstMS Aand the information is in the memory ofMS B.This PR intends to handle this situation by temporally saving the console token in the database instead of saving it in the memory of the MS that generated it. This way, independently of which MS generated the token or which MS the CPVM communicates with, the token will be validated. After using the token, it is removed from the database and also from the CPVM.
Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
I created an environment with 2 MS (A and B) and started only A. I enabled the zone in order to create the CPVM. After that, guaranting that the CPVM was communicating with MS A, I started MS B.
Previous to the changes, I executed two test cases:
createConsoleEndpoint) through MS A and used it. As the CPVM was communicating with MS A, it worked.createConsoleEndpoint) through MS B and used it. As the CPVM was communicating with MS A, it did not worked, presenting the messageFailed to connect to server / access token has expiredWith the changes, I executed again the two test cases:
createConsoleEndpoint) through MS A and used it. As the CPVM was communicating with MS A, it worked.createConsoleEndpoint) through MS B and used it. As the CPVM was communicating with MS A, it worked too, because the session was being stored in the database.After using the URLs, I checked the database and verified that there were no entries in
cloud.allowed_console_session, as expected.