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

Agent device data item id prefix is non-sensical #368

Closed
robot-ranger opened this issue Nov 20, 2023 · 9 comments · Fixed by #372
Closed

Agent device data item id prefix is non-sensical #368

robot-ranger opened this issue Nov 20, 2023 · 9 comments · Fixed by #372
Labels

Comments

@robot-ranger
Copy link
Contributor

What is the prefix on the dataitems of the agent device? and how is it generated?

image

It looks like MQTT adapter uses a sensible prefix: mqtt client id:

image

Can we give the SHDR adapters a more sensible prefix? maybe the device uuid or name they are associated with?

@wsobel
Copy link
Member

wsobel commented Nov 20, 2023

It looks like there was a mistake in some of the code. The prefix is based on the identity of the adapter, which should be one of two versions based on SuppressIPAddress configuration variable:

  1. "_" + server + "_" + port example: _localhost_7878
  2. "_" + sha1digest("_" + server + "_" + port)[0..10]

The other option is to specify it directly using AdapterIdentity.

The reason was a security concern raised by some companies that didn't want the IP addresses of the adapter exposed. It was supposed to be optional and have effect all the way down. I'll add a fix in version 2.2.0.15 with the MQTT source changes.

@robot-ranger
Copy link
Contributor Author

robot-ranger commented Nov 20, 2023

Awesome.
I actually remember seeing that issue and the resolution about security, but i guess my keyword searching wasnt strong enough today.
AdapterIdentity is fine for me, just didnt know it existed.

ETA: i said i was going to close this, but ill keep it open and add more to the readme in my PR. these issues can close when the PR goes.

@robot-ranger
Copy link
Contributor Author

It looks like there was a mistake in some of the code. The prefix is based on the identity of the adapter, which should be one of two versions based on SuppressIPAddress configuration variable:

1. `"_" + server + "_" + port` example: `_localhost_7878`

2. `"_" + sha1digest("_" + server + "_" + port)[0..10]`

The other option is to specify it directly using AdapterIdentity.

The reason was a security concern raised by some companies that didn't want the IP addresses of the adapter exposed. It was supposed to be optional and have effect all the way down. I'll add a fix in version 2.2.0.15 with the MQTT source changes.

Added to my PR : #367

@robot-ranger
Copy link
Contributor Author

Maybe AdapterIdentity can be used to generate MqttClientId?

@wsobel
Copy link
Member

wsobel commented Nov 21, 2023 via email

@robot-ranger
Copy link
Contributor Author

robot-ranger commented Nov 21, 2023

I still have a task to verify if a random client id is being generated if not specified. If it is not generated, why not. And what mqtt_cpp is using as its generator. I thought it used the machine ip and port (unique). I’ll get back once I investigate the code a bit more.

...oh? turns out, host:port for both my sink and source are the same (localhost:1883) !, so they are the same client id? go figure.

I suppose a remote broker is the same issue - host:port is the same for both sink and adapter

@wsobel
Copy link
Member

wsobel commented Nov 21, 2023 via email

@rajwork9
Copy link
Collaborator

AdapterIdentity: in Mqtt it creates like serverIp:MqttPort or serverIp:1883.

MqttClientId: you can declare in config file or Generates unique id.

@wsobel
Copy link
Member

wsobel commented Nov 30, 2023

We need a better way of creating the ID. Unless we have one client per process (which we could do) we need to make each instance unique.

@wsobel wsobel added the bug label Dec 3, 2023
@wsobel wsobel linked a pull request Dec 4, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants