Add Cumulocity remote access plugin #1694

jarhodes314 · 2023-01-25T15:12:00Z

Signed-off-by: James Rhodes james.rhodes@softwareag.com

Proposed changes

This adds support for the c8y_RemoteAccessConnect operation, which enables the remote access tab in the Cumulocity UI. It reads a 530 SmartREST message, based on which it sets up a proxy between a TCP connection to the specified host and port (e.g. 127.0.0.1:22, whatever is specified by the user in the Cumulocity UI), and the corresponding Websocket on cloud. It then copies all data between this Websocket and the TCP socket and vice versa, shutting down when one or other of the connections closes.

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
Documentation Update (if none of the other choices apply)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

I have read the CONTRIBUTING doc
I have signed the CLA (in all commits with git commit -s)
I ran cargo fmt as mentioned in CODING_GUIDELINES
I used cargo clippy as mentioned in CODING_GUIDELINES
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Further comments

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

didier-wenzek

Thank you. This looks good. And I have only an unrelated request regarding the Cargo.toml

plugins/c8y_remote_access_plugin/Cargo.toml

didier-wenzek · 2023-01-26T08:03:53Z

plugins/c8y_remote_access_plugin/Cargo.toml

+futures = "0.3.25"
+futures-util = "0.3.25"
+http = "0.2.8"
+miette = { version = "5.5.0", features = ["fancy"] }


First time I cross this crate, and I'm a bit curious as reporting errors to users is non-trivial.

What's your experience with it? Would you recommend to use it for tedge?

I prefer the output compared to anyhow, and it obviously has support for more complex things (although with a lot of focus on parse errors). I don't think it makes a huge difference, but it's an easy drop-in replacement for anyhow.

For instance, by slightly tweaking tedge_config to contain something like this

#[derive(miette::Diagnostic, thiserror::Error, Debug)] pub enum ConfigSettingError { #[error("A value for `{key}` is missing.")] #[diagnostic(help("A value can be set with `tedge config set {key} <value>`"))] ConfigNotSet { key: &'static str }, ... }

I could get this output (with colour when running in a terminal):

Error: × A value for `c8y.url` is missing. help: A value can be set with `tedge config set c8y.url <value>`

didier-wenzek · 2023-01-26T08:52:20Z

plugins/c8y_remote_access_plugin/src/config.rs

+}
+
+impl TedgeConfig {
+    pub async fn read_from_disk() -> miette::Result<Self> {


There is a crate tedge_config that we are supposed to use to read /etc/tedge/tedge.toml.

Reading these straightforward lines of code, one really wonders why tedge_config is so huge. To be fair with this crate, it has been written with strong validation constraints that have been removed meantime.

didier-wenzek · 2023-01-26T08:56:49Z

plugins/c8y_remote_access_plugin/src/input.rs

+        ensure!(
+            id == 530,
+            "SmartREST message is not a RemoteAccessConnect operation"
+        );


I really like this style to validate inputs.

One must consider this for thin-edge inputs.

plugins/c8y_remote_access_plugin/src/proxy.rs

Co-authored-by: Didier Wenzek <didier.wenzek@free.fr>

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

didier-wenzek

I will approve. However can you fix:

Cargo.lock
cargo +nightly fmt
cargo clippy -- -D warnings.

Thank you!

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

PradeepKiruvale · 2023-01-28T01:46:03Z

docs/src/howto-guides/027_remote_access_with_cumulocity.md

@@ -0,0 +1,42 @@
+# Cumulocity RemoteAcessConnect plugin
+
+To access a device remotely that runs thin-edge.io, a plugin of the operation plugin concept is used. The tedge_agent is checking for cloud remote access operation and is triggering the particular plugin. You can use the remote access tab in device management to access the device via SSH or VNC.


Suggested change

To access a device remotely that runs thin-edge.io, a plugin of the operation plugin concept is used. The tedge_agent is checking for cloud remote access operation and is triggering the particular plugin. You can use the remote access tab in device management to access the device via SSH or VNC.

To access a device remotely that runs thin-edge.io, a plugin of the operation plugin concept is used. The tedge-agent is checking for cloud remote access operation and is triggering the particular plugin. You can use the remote access tab in device management to access the device via SSH or VNC.

docs/src/howto-guides/027_remote_access_with_cumulocity.md

PradeepKiruvale · 2023-01-28T01:52:22Z

docs/src/howto-guides/027_remote_access_with_cumulocity.md

+        height: 500px;
+        object-position: left;
+    }
+</style>


I feel the doc must explain how to start the remote access plugin. Is this going to be a systemd service?

No this plugin is launched by the mapper on Smartrest 530.

If mapper launches this by mapper then it will be mandatory right? If someone does not need this feature then one can't disable it, right?

I'm not entirely sure what the install flow should be for this, but this isn't really true in any case, as the user would just need to delete /etc/tedge/operations/c8y/c8y_RemoteAccessConnect to disable this, and then they can delete /usr/bin/c8y_remote_access_plugin as it's no longer needed.

@reubenmiller can you please help us here, how you would like to package and install it?

The /etc/tedge/operations/c8y/c8y_RemoteAccessConnect file should only be installed when the c8y-remote-access-plugin.deb package is installed.

And if the c8y-remote-access-plugin package is removed, then it should also delete the /etc/tedge/operations/c8y/c8y_RemoteAccessConnect file.

PradeepKiruvale · 2023-01-28T02:09:25Z

plugins/c8y_remote_access_plugin/src/auth.rs

+        .await
+        .into_diagnostic()
+        .with_context(|| format!("Waiting {duration:?} for Cumulocity to respond with JWT"))
+}


The above code is for retrieving the JWT token right. There is already a library that exists, can reuse this code I feel.

Unfortunately there is no true API for that.

Why not just reuse C8YHttpProxy::get_jwt_token? Would add a dependency on c8y_api which I feel is OK.

The issue with C8yJwtTokenRetriever::get_jwt_token is that this comes with a lot of specific stuffs (whole TEdgeConfig, SMCumulocityMapperError, SmartRestJwtResponse ...).

So, yes, it would be good to share this code by behind a lighter API (MqttConfig, JwtToken, JwtError) .

Can we park the "lighter API" part for another ticket? And for now use the C8YHttpProxy::get_jwt_token as the c8y-remote-access-plugin is hosted in the thin-edge.io repository, and it will be eventually merged into the future c8y-devicemanagement-plugin once the refactoring topic is done.

plugins/c8y_remote_access_plugin/src/main.rs

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

didier-wenzek

Approved.

I will merge it despite the error on Cargo.lock. However, the check for unused deps is also failing now.

albinsuresh · 2023-01-30T14:45:16Z

crates/core/tedge_mapper/src/c8y/mapper.rs

@@ -186,6 +186,22 @@ fn create_directories(config_dir: &Path) -> Result<(), anyhow::Error> {
        0o644,
        None,
    )?;
+    create_file_with_user_group(


This code should be moved form this main mapper to the plugin itself so that this operation is enabled only when the plugin is installed. For reference check the c8y_config_plugin impl here:

thin-edge.io/plugins/c8y_configuration_plugin/src/main.rs

Line 168 in a72e543

create_file_with_user_group(

The convention that we use to do such initialisations during installation is by adding a --init option to the script, which is invoked during the installation the debian package via its postinst script.

Here is how it's done by the c8y-config-plugin:

thin-edge.io/plugins/c8y_configuration_plugin/src/main.rs

Line 131 in a72e543

fn init(cfg_dir: PathBuf) -> Result<(), anyhow::Error> {

And here is the postinst script:

thin-edge.io/configuration/debian/c8y-configuration-plugin/postinst

Line 6 in a72e543

c8y-configuration-plugin --init

albinsuresh · 2023-01-30T15:00:18Z

plugins/c8y_remote_access_plugin/src/config.rs

+use miette::IntoDiagnostic;
+use std::net::IpAddr;
+
+pub struct TedgeConfig {


I'd have named this the RemoteAccessPluginConfig to avoid any ambiguity with tedge_config::TEdgeConfig.

albinsuresh · 2023-01-30T15:02:00Z

plugins/c8y_remote_access_plugin/src/config.rs

+}
+
+impl TedgeConfig {
+    pub fn read_from_disk() -> miette::Result<Self> {


Optional: How about implementing a TryFrom<TEdgeConfig> for RemoteAccessPluginConfig instead of this function?

albinsuresh · 2023-01-30T15:03:11Z

plugins/c8y_remote_access_plugin/src/config.rs

+        use tedge_config::C8yUrlSetting;
+        use tedge_config::ConfigSettingAccessor;
+        use tedge_config::MqttBindAddressSetting;
+        use tedge_config::MqttPortSetting;


Quite an unusual style, at least for me. Are there any benefits in keeping these use statements at the method level instead of the module?

On the contrary, I like this style that avoids to pollute the module namespace.

albinsuresh · 2023-01-30T15:07:06Z

plugins/c8y_remote_access_plugin/src/auth.rs

+        .await
+        .into_diagnostic()
+        .with_context(|| format!("Waiting {duration:?} for Cumulocity to respond with JWT"))
+}


Why not just reuse C8YHttpProxy::get_jwt_token? Would add a dependency on c8y_api which I feel is OK.

docs/src/howto-guides/027_remote_access_with_cumulocity.md

albinsuresh · 2023-01-30T15:24:41Z

plugins/c8y_remote_access_plugin/src/proxy.rs

+
+            select(incoming, outgoing).await;
+        }
+        println!("STOPPING");


When does this plugin stop realistically? Can we "end" the session remotely from the cloud "Remote Access" UI?

Usually it would stop when someone closes the VNC/SSH window, which causes the Websocket to be closed.

albinsuresh · 2023-01-30T15:32:54Z

docs/src/howto-guides/howto-guides.md

@@ -26,3 +26,4 @@
 24. [How to manage configuration files with Cumulocity](./025_config_management_plugin.md)
 25. [How to install thin-edge manually with openrc](./026_how_to_install_thin_edge_manually.md)
 26. [How to enable configuration management on child devices](./child_device_config_management_agent.md)
+27. [How to connect to your thin-edge.io device with Cumulocity remote access](./027_remote_access_with_cumulocity.md)


Suggested change

27. [How to connect to your thin-edge.io device with Cumulocity remote access](./027_remote_access_with_cumulocity.md)

27. [How to remotely connect to your thin-edge.io device via SSH/VNC/Telnet with Cumulocity remote access](./027_remote_access_with_cumulocity.md)

docs/src/howto-guides/027_remote_access_with_cumulocity.md

albinsuresh · 2023-01-30T15:35:19Z

docs/src/howto-guides/027_remote_access_with_cumulocity.md

@@ -0,0 +1,41 @@
+# Cumulocity RemoteAcessConnect plugin
+


A link to the Cumulocity remote access feature somewhere in the doc would be nice.

Yes I would agree, a link to the Cumulocity IoT docs would be useful, as one of the default mistakes is to forget to add the Remote access admin permission to your user/role. Otherwise you don't see the Remote access tab in the Device Management application

plugins/c8y_remote_access_plugin/Cargo.toml

reubenmiller

The c8y-remote-access-plugin entry needs to be added to ci/package_list.sh which is used by variables scripts (most notable the main build script ci/build_scripts/build.sh).

For example just add the package to the RELEASE_PACKAGES entries:

RELEASE_PACKAGES=(
    tedge
    tedge-mapper
    tedge-agent
    tedge-watchdog
    tedge-apt-plugin
    c8y-log-plugin
    c8y-configuration-plugin
    c8y-remote-access-plugin
)

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

albinsuresh · 2023-02-01T10:17:22Z

plugins/c8y_remote_access_plugin/src/input.rs

+}
+
+#[derive(Debug, PartialEq, Eq)]
+pub enum Command {


It would have been nice if clap could parse the input arguments to this enum directly as that would eliminate the need for us to validate exclusive arg combinations.

Yeah, I don't think this is possible unfortunately. The #[clap(group(...))] does at least give good error messages when multiple arguments are provided.

plugins/c8y_remote_access_plugin/src/auth.rs

plugins/c8y_remote_access_plugin/src/config.rs

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

reubenmiller · 2023-02-01T13:53:29Z

I retested the changes and the install/uninstall now works perfectly (except for the fact that the mapper needs to be restarted sometimes, but that is not in the scope of this ticket).

However we do need to change how/when the local proxy is being spawned and when the main process should return to tedge (so that the operation status is transitioned in the correct order).

At the moment, the plugin only returns when the plugin process is finished. This means if there are two connections at the same time, the wrong operation will be transitioned in the cloud.

Example

The following process leads to the incorrect Cumulocity IoT Operation being updated to SUCCESSFUL. The actions are performed on the same device.

User A connects using webssh. Operation 1 is kept in EXECUTING status until the window is closed
User B connects using webssh. Operation 2 is kept in EXECUTING status until the window is closed
User B closes the window first. This results in Operation 1 being set to SUCCESSFUL instead of Operation 2!!!

Suggested solution

tedge launches the plugin
The plugin creates a background process which tries to connect to the endpoint given to the plugin via the arguments.
1. If the child process successfully connects to the endpoint, then it should signal back to the main process that the connection was successful.
2. If the child process is not able to successfully connect to the endpoint, then it should signal back to the main process that the connection failed (with a fitting failure reason)
The main process waits for a success or failure signal from the child process (also using a timeout if it does not receive anything from the service)
The main process exits with an exit code of 0 or non-zero, depending on the signal received from the child process. The child process should continue running after the main process exits.

Note

The tricky part might be the detection if the child process is "successful" or not...if the criteria is too difficult to define or observe, then we could always just send a "SUCCESSFUL" message back to tedge. But ideally having a SUCCESSFUL or FAILED status would be the most useful for users, as they know when something did not work correctly.

didier-wenzek · 2023-02-01T14:02:49Z

However we do need to change how/when the local proxy is being spawned and when the main process should return to tedge (so that the operation status is transitioned in the correct order).

I fully understand the need. However, this cannot be done easily/quickly as described because as of now there is no communication channel between the mapper and the plugin process. Adding this channel is beyond the scope of this PR.

What can be done, as a workaround for that PR, is to let the plugin itself send the executing status of the operation to C8Y.

albinsuresh · 2023-02-01T14:11:36Z

What can be done, as a o for that PR, is to let the plugin itself send the executing status of the operation to C8Y.

Wondering if this will lead to double reporting of the operation statuses, which would still lead to interference with other operations of the same kind. For eg: the plugin first sends the successful operation status and then the mapper also sends the same successful status on exit code 0. The second message sent by the mapper could then interfere with another "executing" operation. Unless there is some mechanism in the mapper with which the plugin can tell the mapper not to send the operation status.

If such a mechanism doesn't exist, I'd be in favour of merging this PR now and introducing that feature in the mapper in a follow-up task.

reubenmiller · 2023-02-01T14:17:53Z

However we do need to change how/when the local proxy is being spawned and when the main process should return to tedge (so that the operation status is transitioned in the correct order).

I fully understand the need. However, this cannot be done easily/quickly as described because as of now there is no communication channel between the mapper and the plugin process. Adding this channel is beyond the scope of this PR.

What can be done, as a workaround for that PR, is to let the plugin itself send the executing status of the operation to C8Y.

As a compromise I would then propose that main process exits as soon as the background has been spawned. As the operation is just asking to "establish" a connection, not wait until the connection has been closed again. That should be able to be quickly done.

jarhodes314 · 2023-02-01T14:36:46Z

I fully understand the need. However, this cannot be done easily/quickly as described because as of now there is no communication channel between the mapper and the plugin process. Adding this channel is beyond the scope of this PR.

What can be done, as a workaround for that PR, is to let the plugin itself send the executing status of the operation to C8Y.

I'm not sure this is necessarily that complicated to achieve. All the logic for this would be within the plugin. As far as the mapper is concerned, the plugin exits within a few seconds to indicate success or failure of the connection.

Communicating between the plugin and a child process can be done in a similar way to how the PoC implementation spawned tedge to handle MQTT communication, so I don't anticipate problems there.

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

reubenmiller · 2023-02-01T17:53:56Z

Tested the updated version and works like a dream :) The operation is transitioned to either SUCCESSFUL/FAILED after feedback from the child process is returned. So the operation does not stay in the EXECUTING state for long (which reduces the risk of updating the wrong operation).

We also have an failure reason (which is super useful for users!). Below is an example error when the ssh port is misconfigured (or not open)

And the happy case, when the operation is set to SUCCESSFUL but the connection is still alive :)

reubenmiller

Approved

didier-wenzek

Approved

didier-wenzek · 2023-02-01T18:07:31Z

plugins/c8y_remote_access_plugin/src/main.rs

+struct Unreachable<E: std::error::Error + 'static>(#[source] E, &'static str);
+
+async fn spawn_child(command: String) -> miette::Result<()> {
+    let mut command = tokio::process::Command::new("/usr/bin/c8y-remote-access-plugin")


It would be better to use the $0 argument of the process, rather than assuming the plugin installed at a very specific location.

gligorisaev · 2023-02-13T12:08:07Z

Checked using:
https://github.com/thin-edge/thin-edge.io/blob/main/docs/src/howto-guides/027_remote_access_with_cumulocity.md
and
https://cumulocity.com/guides/cloud-remote-access/using-cloud-remote-access/

jarhodes314 force-pushed the feature/remote-access-plugin branch from e5a7e13 to f043225 Compare January 25, 2023 15:43

Add Cumulocity remote access plugin

a8b60a8

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

jarhodes314 force-pushed the feature/remote-access-plugin branch from f043225 to a8b60a8 Compare January 25, 2023 16:51

didier-wenzek reviewed Jan 26, 2023

View reviewed changes

jarhodes314 and others added 2 commits January 26, 2023 11:30

Use workspace inheritance for new crate

7061f09

Co-authored-by: Didier Wenzek <didier.wenzek@free.fr>

Address review comments

586ef53

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

didier-wenzek reviewed Jan 26, 2023

View reviewed changes

Run clippy and formatter

dbd006d

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

Copy docs from existing plugin

b7a6000

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

PradeepKiruvale reviewed Jan 30, 2023

View reviewed changes

jarhodes-sag added 2 commits January 30, 2023 14:43

Use ? instead of .unwrap

b45f616

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

Correct typos and remove irrelevant sentence

71dca79

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

jarhodes314 force-pushed the feature/remote-access-plugin branch from a4aa8f8 to 71dca79 Compare January 30, 2023 14:44

didier-wenzek approved these changes Jan 30, 2023

View reviewed changes

albinsuresh reviewed Jan 30, 2023

View reviewed changes

reubenmiller reviewed Jan 30, 2023

View reviewed changes

plugins/c8y_remote_access_plugin/Cargo.toml Show resolved Hide resolved

reubenmiller requested changes Jan 30, 2023

View reviewed changes

Address remaining comments

67279d3

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

albinsuresh approved these changes Feb 1, 2023

View reviewed changes

Remove unnecessary config module and move Websocket

94781f8

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

jarhodes-sag added 2 commits February 1, 2023 17:12

Run the proxy in a child process

2fe7ab8

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

Fix warning

b33e055

Signed-off-by: James Rhodes <james.rhodes@softwareag.com>

reubenmiller approved these changes Feb 1, 2023

View reviewed changes

didier-wenzek approved these changes Feb 1, 2023

View reviewed changes

didier-wenzek merged commit a01187d into thin-edge:main Feb 1, 2023

reubenmiller mentioned this pull request Feb 1, 2023

Fix remove remote access install #1706

Merged

11 tasks

reubenmiller added this to the 0.10.0 milestone Feb 10, 2023

gligorisaev self-assigned this Feb 13, 2023

reubenmiller added the release:highlight label Mar 10, 2023

		@@ -0,0 +1,42 @@
		# Cumulocity RemoteAcessConnect plugin

		To access a device remotely that runs thin-edge.io, a plugin of the operation plugin concept is used. The tedge_agent is checking for cloud remote access operation and is triggering the particular plugin. You can use the remote access tab in device management to access the device via SSH or VNC.

	27. [How to connect to your thin-edge.io device with Cumulocity remote access](./027_remote_access_with_cumulocity.md)
	27. [How to remotely connect to your thin-edge.io device via SSH/VNC/Telnet with Cumulocity remote access](./027_remote_access_with_cumulocity.md)

Add Cumulocity remote access plugin #1694

Add Cumulocity remote access plugin #1694

Conversation

jarhodes314 commented Jan 25, 2023

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

didier-wenzek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reubenmiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reubenmiller commented Feb 1, 2023 • edited

didier-wenzek commented Feb 1, 2023

albinsuresh commented Feb 1, 2023

reubenmiller commented Feb 1, 2023

jarhodes314 commented Feb 1, 2023

reubenmiller commented Feb 1, 2023 • edited

reubenmiller left a comment

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gligorisaev commented Feb 13, 2023

reubenmiller commented Feb 1, 2023 •

edited

reubenmiller commented Feb 1, 2023 •

edited