GUACAMOLE-2138: Add connection-timeout parameter#1117
GUACAMOLE-2138: Add connection-timeout parameter#1117tkuhlengel wants to merge 5 commits intoapache:mainfrom
Conversation
* Add defaults for parameter connection-timeout in guacamole.properties to disable feature. * Implement the connection timeout using a Map based on creation time * Integrate the connection timeout check into the existing idle timeout function.
There was a problem hiding this comment.
Pull Request Overview
This PR adds a connection timeout feature to automatically terminate connections after a specified duration, regardless of activity level. This addresses security and auditing requirements for ensuring idle users are disconnected within set time limits.
- Adds a new
connection-timeoutparameter inguacamole.properties(defaults to 0/disabled) - Implements connection timeout tracking using a Map to store tunnel creation times
- Integrates connection timeout checks into the existing session eviction mechanism
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| HashTokenSessionMap.java | Adds connection-timeout property configuration and integrates timeout checking into session eviction task |
| GuacamoleSession.java | Implements tunnel creation time tracking and expiration logic for individual connections |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| UserTunnel tunnel = tunnels.get(tunnelId); | ||
| if (tunnel != null) { | ||
| try { | ||
| tunnel.close(); | ||
| logger.debug("Closed tunnel \"{}\" due to connection timeout.", tunnelId); | ||
| } | ||
| catch (GuacamoleException e) { | ||
| logger.debug("Unable to close expired tunnel \"" + tunnelId + "\".", e); | ||
| } | ||
| // Remove from both maps regardless of whether close succeeded | ||
| tunnels.remove(tunnelId); | ||
| tunnelCreationTimes.remove(tunnelId); | ||
| closedCount++; | ||
| } | ||
| } |
There was a problem hiding this comment.
The tunnel removal logic is duplicated here and in the removeTunnel method. Consider extracting this into a private helper method to avoid code duplication.
| UserTunnel tunnel = tunnels.get(tunnelId); | |
| if (tunnel != null) { | |
| try { | |
| tunnel.close(); | |
| logger.debug("Closed tunnel \"{}\" due to connection timeout.", tunnelId); | |
| } | |
| catch (GuacamoleException e) { | |
| logger.debug("Unable to close expired tunnel \"" + tunnelId + "\".", e); | |
| } | |
| // Remove from both maps regardless of whether close succeeded | |
| tunnels.remove(tunnelId); | |
| tunnelCreationTimes.remove(tunnelId); | |
| closedCount++; | |
| } | |
| } | |
| if (removeTunnelById(tunnelId, "Closed tunnel \"{}\" due to connection timeout.", "Unable to close expired tunnel \"{}\".")) { | |
| closedCount++; | |
| } | |
| } | |
| /** | |
| * Removes the tunnel with the given ID from the session, closing it and removing | |
| * its creation time. Returns true if a tunnel was removed, false otherwise. | |
| * | |
| * @param tunnelId | |
| * The ID of the tunnel to remove. | |
| * @param successMsg | |
| * The message to log on successful close. | |
| * @param errorMsg | |
| * The message to log on error. | |
| * @return | |
| * true if a tunnel was removed, false otherwise. | |
| */ | |
| private boolean removeTunnelById(String tunnelId, String successMsg, String errorMsg) { | |
| UserTunnel tunnel = tunnels.get(tunnelId); | |
| if (tunnel != null) { | |
| try { | |
| tunnel.close(); | |
| logger.debug(successMsg, tunnelId); | |
| } | |
| catch (GuacamoleException e) { | |
| logger.debug(errorMsg, tunnelId, e); | |
| } | |
| tunnels.remove(tunnelId); | |
| tunnelCreationTimes.remove(tunnelId); | |
| return true; | |
| } | |
| return false; | |
| } |
There was a problem hiding this comment.
I'm not sure I agree with CoPilot, here; however, is there any reason not to use the removeTunnel() method?
There was a problem hiding this comment.
I'm not the expert in this particular case.
My understanding of removeTunnel is that it doesn't close an active tunnel, only removes it from the list of active tunnels. I think that's the equivalent of disassociating it from one user?
I could use it on line +375 in this chunk, but I'm not sure how much difference it will make to make it smaller or meaningfully better other than consistency. I still need to close the tunnel first.
- Introduce creationTime field in AbstractGuacamoleTunnel - Implement getCreationTime method in GuacamoleTunnel interface - Update DelegatingGuacamoleTunnel to return tunnel creation time - Remove tunnel creation time tracking map from GuacamoleSession - Adjust methods in GuacamoleSession to use GuacamoleTunnel.getCreationTime() and reuse the existing Map of active tunnels.
|
I think this is the full "whole-server" version I have for now, addressing the comments above. I saw your comments about adding feedback to guacd. Is that something I would need to implement in a guacd pull request for this to be approved? |
| UserTunnel tunnel = tunnels.get(tunnelId); | ||
| if (tunnel != null) { | ||
| try { | ||
| tunnel.close(); | ||
| logger.debug("Closed tunnel \"{}\" due to connection timeout.", tunnelId); | ||
| } | ||
| catch (GuacamoleException e) { | ||
| logger.debug("Unable to close expired tunnel \"" + tunnelId + "\".", e); | ||
| } | ||
| // Remove from both maps regardless of whether close succeeded | ||
| tunnels.remove(tunnelId); | ||
| tunnelCreationTimes.remove(tunnelId); | ||
| closedCount++; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure I agree with CoPilot, here; however, is there any reason not to use the removeTunnel() method?
Yes, any sort of feedback look for guacd would require some changes both here and in the guacd code. |
* Formatting and style cleanup.
|
In terms of communication with guacd:
|
I'm tempted to say make another one, because of things I've mentioned above - the implementation, here, isn't so much a "timeout" as it is a limit. To me, "timeout" carries a notion of waiting for something - user interaction, a network response, etc. - and not getting that. In this case, you're actually putting a cap on the session duration regardless of whether the user is active or not, the server is responsive or not, etc.
I would guess there just hasn't been a need for it in the past. It could probably be added - my only caution would be to make sure that adding it doesn't break compatibility with clients that don't send a message. |
- Add method to send error instruction for connection timeout - Close tunnels with guacd notification when exceeding max duration - Improve logging for tunnel closure and error handling
Summary
In my company's business use case for Guacamole, for security and auditing purposes, we need to be able to ensure that any idle user is disconnected and logged off within a set period of time of idleness.
In an ideal version of such a system, we would do the following.
This requires a lot of conditionals and would be more difficult to implement and maintain in an ongoing project like Guacamole.
A more practical, yet sufficient, version is:
This second option meets our business needs, and we would like to share it with others.
Features
connection-timeoutinguacamole.properties, disconnecting users afterconnection-timeoutminutes. Defaults to 0, disabling the feature.