-
Notifications
You must be signed in to change notification settings - Fork 44
Support for sink provisioning #305
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
Conversation
If a too large message is sent, weight value exceeding 255 can be truncated to something less than 16. WPC_send_data_with_options returns an error if total size is too large anyway, so this is not a problem currently. Also initialize the variable to suppress maybe-uninitialized warning.
| except KeyError: | ||
| # App config not defined in new config | ||
| logging.debug("Missing key app_config key in config: %s", config) | ||
| logging.debug("Missing app config related key in config; will not set app config") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (94 > 79 characters)
| data = config["app_config_data"] | ||
|
|
||
| logging.info("Set app config with %s", config) | ||
| logging.info(f"Set app config with seq: {seq}, diag: {diag}, data: {data}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (87 > 79 characters)
| self.proxy.SetManagementSecurityKeys(cipher, auth, seq) | ||
| except GLib.Error as e: | ||
| error_code = ReturnCode.error_from_dbus_exception(str(e)) | ||
| logging.error("Error when setting management keys: %s", error_code.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (88 > 79 characters)
python_transport/requirements.txt
Outdated
| @@ -1,5 +1,5 @@ | |||
| # wirepas | |||
| wirepas_mesh_messaging==1.3.0rc1 | |||
| wirepas_mesh_messaging @ git+https://github.com/wirepas/wirepas-mesh-messaging-python/@feature/sink-key-management-dev | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO this should be changed before merging this. A new RC for that library should be made once wirepas/wirepas-mesh-messaging-python#33 is merged.
| self.orig_request = request | ||
|
|
||
| def __str__(self): | ||
| allowed_fields = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be also other way around, i.e. masked_fields, so that possible new fields does not require change in here? I guess anyways most of the fields will be allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be, and it would even be easier and simpler to do. But I thought this is safer in case a new field is added in the future and we forget to add it to the filter to hide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap, in theory I agree, in practice it then means that we need to always add fields to this (which may not be that obvious). I guess there should not be that many secrets that we pass through this API that I would maybe still lean towards turning this around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright I can change it, I agree probably there won't be such fields coming anytime soon anyway.
| FetchContent_Declare( | ||
| c-mesh-api | ||
| GIT_REPOSITORY https://github.com/wirepas/c-mesh-api/ | ||
| GIT_TAG c566fdbd693eb3fb6027286679f53db9f9958e0a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether this should be extracted from here to some "config" file? Also should we use some explicit tags instead of commit hashes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think tags would be good too. There are some tags/releases for c-mesh-api from the past(2022) which were using the same version as gateway. I don't know if there would be an advantage to use the same version numbers, but we can start creating releases for c-mesh-api again.
| memcpy((void*)security_keys->key_pair.authentication, key_bytes, sizeof(security_keys->key_pair.authentication)); | ||
|
|
||
| r = sd_bus_message_read(message, "y", &security_keys->sequence_number); | ||
| if (r < 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also here validate that the sequence falls into the supported range [1..255]? Or does we let sink node itself to handle that (or will the APIs just silently crop the value)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dbus method here would return an error if the number is too large to be 8 bits (caught as OverflowError on python side). For value 0, sink should return an error code.
Debug log for SetConfigRequest is updated to exclude disallowed config parameters by printing them as "<HIDDEN>". When the wrapper class MaskedRequest is passed as a string parameter for a formatted log (like in current example), the __str__ method only executes if the log will be printed. Additionally: - Error logs under _set_param are modified to completely ommit parameter values. - Fix logging for app config by only printing related parameters.
0f21a98 to
ff50011
Compare
Adds support for setting network and management keys using the new API on the sink.
Additionally: