Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support Cumulocity SmartREST 1.0 #3196

Merged

Conversation

rina23q
Copy link
Member

@rina23q rina23q commented Oct 21, 2024

Proposed changes

To support SmartREST1.0, this PR includes:

  • basic auth support for HTTP including c8y-remote-access-plugin
  • credentials file implementation
  • username/password support for MQTT bridge
  • username/password authentication for MQTT connections
  • SmartREST1.0 templates support in tedge config and MQTT bridges.
  • system tests, thanks to @reubenmiller

How to use basic auth instead of device certs and SmartREST1.0?

  1. Set the mode to "basic"
tedge config set c8y.auth_mode "basic"
  1. Give username and password to the credentials file. The location is /etc/tedge/credentials by default. Configurable by c8y.credentials_path.
[c8y]
username = "t5678/octocat"
password = "abcd1234"
  1. Give the SmartREST1.0 templates names to tedge config.
tedge config set c8y.smartrest1.templates "{template_xid}"
  1. Run tedge connect c8y! The appropriate bridge for Basic Auth and SmartREST1.0 will be created.

Note: if a device is already registered via certificate, you cannot switch to basic auth for the device.


Current limitations:

  • device.id is still referring the value from the device certificate.
  • the device will not be registered during tedge connect c8y. It expects to use bulk operation API to get credentials (username/password) beforehand.
  • the connection check and the tenant URL check in tedge connect c8y is disabled since it uses JWT token. However, the connection check is actually done by sending 100 message, and supposed to receive 71, device already exists.

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

#3036

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

@reubenmiller
Copy link
Contributor

@rina23q What about changing the default credential location to the following, keeping in mind that it would allow us to add additional credentials to it (maybe in the future adding the c8y device bootstrapping credentials might be necessary).

Below is an example of using a single credentials file with multiple secrets, where keys are used to classify the secrets:

File

/etc/tedge/credentials

Contents

[c8y]
username = "octocat"
password = "abcd1234"

Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

The switch between JWT versus BasicAuth is implemented in the correct place. i.e. under c8y_api which is used both by the mapper and the remote access plugin.

However, the C8yAuthRetriever can be simplified.

crates/core/c8y_api/src/http_proxy.rs Outdated Show resolved Hide resolved
@rina23q rina23q changed the title Support basic authentication for Cumulocity feat: support SmartREST1.0 for Cumulocity Oct 22, 2024
Copy link
Contributor

github-actions bot commented Oct 23, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
525 0 2 525 100 1h39m24.966774999s

@rina23q rina23q force-pushed the feature/3036/support-basic-auth-for-c8y branch from eaff09b to 9f634db Compare October 23, 2024 17:26
@rina23q rina23q force-pushed the feature/3036/support-basic-auth-for-c8y branch from 9f634db to 3b7b531 Compare October 23, 2024 17:31
@rina23q rina23q temporarily deployed to Test Pull Request October 23, 2024 17:31 — with GitHub Actions Inactive
@rina23q rina23q force-pushed the feature/3036/support-basic-auth-for-c8y branch from 3b7b531 to 1774e4f Compare October 23, 2024 17:53
@rina23q rina23q force-pushed the feature/3036/support-basic-auth-for-c8y branch from 1774e4f to 4caf2bb Compare October 23, 2024 18:10
@rina23q rina23q temporarily deployed to Test Pull Request October 23, 2024 18:10 — with GitHub Actions Inactive
@rina23q rina23q force-pushed the feature/3036/support-basic-auth-for-c8y branch from 1a5d441 to 4e41c4e Compare October 23, 2024 19:01
@rina23q rina23q force-pushed the feature/3036/support-basic-auth-for-c8y branch from 4e41c4e to f48924b Compare October 23, 2024 19:11
@rina23q rina23q temporarily deployed to Test Pull Request October 23, 2024 19:11 — with GitHub Actions Inactive
Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
@rina23q
Copy link
Member Author

rina23q commented Oct 25, 2024

Flaky test update (resolved)

I did 20 iterations in my local for the suite smartrest_one suite by using the flake-finder.

invoke flake-finder --iterations 20 --outputdir output_flake_finder_3 --suite smartrest_one

The result was 9 passed, 11 failed. So, the test is absolutely flaky.

Failed iterations are: [2, 4, 6, 8, 9, 10, 11, 13, 14, 19, 20]

There were two patterns of failures.

1. NotAuthorized error in tedge connect c8y.

failed iterations

  • build_in bridge: 4, 6, 9, 11, 13, 14,
  • mosquitto bridge: 10, 20
cmd: ['/bin/sh', '-c', 'tedge connect c8y'], exit code: 1
stdout:
Using basic authentication.

Checking if systemd is available.

Validating the bridge certificates.

Creating the device in Cumulocity cloud.


stderr:
Detected mosquitto version >= 2.0.0
ERROR: ConnectionRefused(NotAuthorized)
Error: failed to connect to Cumulocity cloud.

Caused by:
    Connection check failed

2. Didn't receive a message onto the SmartREST1.0 subscription topic.

failed iterations

  • build_in bridge:
  • mosquitto bridge: 2, 8, 19

example output:

Should Have MQTT Messages   c8y/s/dl/${TEMPLATE_XID}    message_pattern=^800,\\d+,${mo["id"]}    timeout=10

Matching messages on topic 'c8y/s/dl/TST_Template_glare_internal_shore' is less than minimum.
wanted: 1
got: 0

messages:
[]

Update 1 [25.10.2024]

Pushed a new commit 06acbff to improve the stability of the tests. However, still failing.

Failed iterations: [8, 9, 11, 12, 13, 15, 19, 20] out of 20.

  • Failed in tedge connect: 9, 12, 13, 15, 19, 20
  • Failed in receiving on c8y/d/dl/{template}: 8, 11,

Note: iteration 11 also failed in configuration operation check. Error was
Expected operation (id=3829) to be SUCCESSFUL, but got: EXECUTING (failureReason: ). It was first time in total 80 attempts.


Update 2 (final) [29.10.2024]

The authorization issue was fully resolved. When the password contains $ and it's not end, it was wrongly parsed because of double quotes, e.g. 3pcrTLlzGAO23JPKop4$@. Changed from "${CREDENTIALS.password}" to '${CREDENTIALS.password}' solved the problem.

    Execute Command
    ...    cmd=printf '[c8y]\nusername = "%s"\npassword = "%s"\n' '${CREDENTIALS.username}' '${CREDENTIALS.password}' > /etc/tedge/credentials

The other message delivery issue's root cause is not 100% clear. Added tedge reconnect c8y after template registration, then publish a message with QoS 1 seems solved the problem.

    Execute Command    cmd=tedge mqtt pub --qos 1 c8y/s/ul/${TEMPLATE_XID} '339,${DEVICE_SN}'

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
@rina23q rina23q force-pushed the feature/3036/support-basic-auth-for-c8y branch from 69db2d4 to 39f1354 Compare October 30, 2024 11:46
@rina23q rina23q temporarily deployed to Test Pull Request October 30, 2024 11:46 — with GitHub Actions Inactive
Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
@rina23q
Copy link
Member Author

rina23q commented Oct 30, 2024

LGTM.

A few other niggles that I observed during tedge connect:

Local MQTT publish has timed out.

Make sure mosquitto is running.
Failed to get the connected tenant URL from Cumulocity.

Users might get alarmed with such error messages. So, we'll have to hide or update some of these messages to account for basic auth based connections. BTW, since fetching the tenant URL from C8y failed, is that endpoint not supported while using basic auth?

This reported issue is resolved in b1ace67.

@rina23q rina23q added this pull request to the merge queue Oct 30, 2024
Merged via the queue into thin-edge:main with commit af7e9ce Oct 30, 2024
33 checks passed
@reubenmiller reubenmiller mentioned this pull request Nov 4, 2024
11 tasks
@reubenmiller reubenmiller changed the title feat: support SmartREST1.0 for Cumulocity feat: support Cumulocity SmartREST1.0 Dec 16, 2024
@reubenmiller reubenmiller changed the title feat: support Cumulocity SmartREST1.0 feat: support Cumulocity SmartREST 1.0 Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme:c8y Theme: Cumulocity related topics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants