Add OCW verifier, pass params between fabric bridge & admin #34209
+648
−79
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
axelnxp,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chapongatien,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
doru91,
fessehaeve,
harimau-qirex,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs and
jmeg-sfy
|
PR #34209: Size comparison from 82b1e1b to 1672d94 Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Jul 5, 2024
andy31415
reviewed
Jul 5, 2024
andy31415
reviewed
Jul 5, 2024
andy31415
reviewed
Jul 5, 2024
andy31415
requested changes
Jul 5, 2024
andy31415
reviewed
Jul 5, 2024
| CHIP_ERROR tlvError = DataModel::Decode(handlerContext.mPayload, commandData); | ||
| SuccessOrExit(tlvError); | ||
|
|
||
| commissioningTimeout = commandData.commissioningTimeout; |
There was a problem hiding this comment.
why do you separate declaration of variables from assignment? I assume because of successOrExit... can we avoid that pattern?
Do we need separate variables? why not use commandData.discriminator and such directly and not have separate variables?
There was a problem hiding this comment.
Fixed in d29e4fc
|
@andy31415 I intentionally held off on doing the argument refactor / params class introduction since that's already done by @yufengwangca in #33799. However, since that PR is blocked due to other reasons and this is requested here, I can pull in those changes here if that's okay. Additionally, I can go either way with breaking/maintaining ABI compatibility. Most of the callers of this function are currently passing around |
bzbarsky-apple
requested changes
Jul 5, 2024
| @@ -104,8 +106,8 @@ class CommissioningWindowOpener | |||
| */ | |||
| CHIP_ERROR OpenCommissioningWindow(NodeId deviceId, System::Clock::Seconds16 timeout, uint32_t iteration, | |||
| uint16_t discriminator, Optional<uint32_t> setupPIN, Optional<ByteSpan> salt, | |||
| Callback::Callback<OnOpenCommissioningWindow> * callback, SetupPayload & payload, | |||
| bool readVIDPIDAttributes = false); | |||
| Optional<ByteSpan> verifier, Callback::Callback<OnOpenCommissioningWindow> * callback, | |||
There was a problem hiding this comment.
This makes no sense. There is never a situation in which you would both pass in the PIN and pass in the verifier, so why do we have a method that lets you pass both?
It sounds like we need to have two separate methods, and the impl gets the verifier (provided or generated) and then calls a common impl method.
There was a problem hiding this comment.
With the params refactor, here's the new shape of the two functions I'm thinking of, let me know your feedback
struct CommissioningWindowParams
{
// The device Id of the node.
NodeId deviceId;
// The duration for which the commissioning window should remain open.
System::Clock::Seconds16 timeout;
// The PAKE iteration count associated with the PAKE Passcode ID and ephemeral PAKE passcode verifier to be used for this
// commissioning.
uint32_t iteration;
// The long discriminator for the DNS-SD advertisement.
uint16_t discriminator;
// The salt to use, or NullOptional to use a randomly-generated one. If provided, must be at least
// kSpake2p_Min_PBKDF_Salt_Length bytes and at most kSpake2p_Max_PBKDF_Salt_Length bytes in length.
Optional<ByteSpan> salt = NullOptional;
// Should the API internally read VID and PID from the device while opening the commissioning window. If this argument is
// `true`, the API will read VID and PID from the device and include them in the setup payload passed to the callback.
bool readVIDPIDAttributes = false;
Callback::Callback<OnOpenCommissioningWindow> *
callback; // The function to be called on success or failure of opening the commissioning window.
};
/**
* @brief
* Try to look up the device attached to our controller with the given
* node id and ask it to re-enter commissioning mode with a PASE verifier
* derived from the given information and the given discriminator. The
* device will exit commissioning mode after a successful commissioning,
* or after the given `timeout` time.
*
* @param[in] params The common parameters required to open the commissioning window.
* @param[in] setupPIN The setup PIN to use, or NullOptional to use a randomly-generated one.
* @param[out] payload The setup payload, not including the VID/PID bits,
* even if those were asked for, that is generated
* based on the passed-in information. The payload
* provided to the callback function, unlike this
* out parameter, will include the VID/PID bits if
* readVIDPIDAttributes is true.
*/
CHIP_ERROR OpenCommissioningWindow(const CommissioningWindowParams & params, Optional<uint32_t> setupPIN,
SetupPayload & payload);
/**
* @brief
* Try to look up the device attached to our controller with the given
* node id and ask it to re-enter commissioning mode with a PASE verifier
* derived from the given information and the given discriminator. The
* device will exit commissioning mode after a successful commissioning,
* or after the given `timeout` time.
*
* @param[in] params The common parameters required to open the commissioning window.
* @param[in] verifier The PAKE passcode verifier to use, or NullOptional to generate verifier based on randomly generated
* PIN and other parameters.
* @param[out] payload The setup payload, not including the VID/PID bits,
* even if those were asked for, that is generated
* based on the passed-in information. The payload
* provided to the callback function, unlike this
* out parameter, will include the VID/PID bits if
* readVIDPIDAttributes is true.
*/
CHIP_ERROR OpenCommissioningWindow(const CommissioningWindowParams & params, Optional<ByteSpan> verifier,
SetupPayload & payload);There was a problem hiding this comment.
I think there are a few issues. First, optional for salt (which means "use a random one" if not provided) does not make sense with a verifier, because the salt needs to match the verifier.
I think there are two ways of presenting the "things verifier will be derived from":
- Iteration count, optional passcode (mis-named PIN in this API), optional salt.
- Iteration count, salt, verifier that corresponds to the provided iteration count and salt and some not-provided passcode.
In terms of modeling this in the API, seems like the cleanest thing are two separate object classes to encapsulate those two separate pieces of information, right? Whether then we do some sort of static dispatch via overloads or have them inherit from a class that has a pure virtual method that outputs the iteration count, salt, and verifier is something to think about.
Second issue: there is no way to create a SetupPayload from a verifier: the whole point of a SetupPayload is to know the passcode so you can do PASE. So the API version that takes a verifier can't have a SetupPayload outparam.
Third issue: The version of the API with a verifier can't pass a SetupPayload to the callback function, for exactly the same reasons.
There was a problem hiding this comment.
Thanks for detailing these. Indeed I came across the second issue during the refactor and that led to digging in for the third one which seems to be even more problematic.
For the first one, I'm leaning towards just taking the salt out of common params class, and making it part of first API (with passcode) as optional, and second one (with verifier) as required.
Second: the setup payload doesn't need to be an outparam for the second API, and the current consumer of it (fabric-admin) just ignores that anyway so we can safely remove this.
Third: This is where I'm torn between either introducing two different callback functions or just making the setup payload optional in the existing one. Essentially either
typedef void (*OnOpenCommissioningWindowWithPasscode)(void * context, NodeId deviceId, CHIP_ERROR status, SetupPayload payload);
typedef void (*OnOpenCommissioningWindowWithVerifier)(void * context, NodeId deviceId, CHIP_ERROR status);
typedef void (*OnOpenBasicCommissioningWindow)(void * context, NodeId deviceId, CHIP_ERROR status);(we could also keep the default OnOpenCommissioningWindowWithPasscode as OnOpenCommissioningWindow to keep the diff small)
or
typedef void (*OnOpenCommissioningWindow)(void * context, NodeId deviceId, CHIP_ERROR status, Optional<SetupPayload payload>);
typedef void (*OnOpenBasicCommissioningWindow)(void * context, NodeId deviceId, CHIP_ERROR status);Callback function can then also be taken out of common params depending on the solution opted for (3)
There was a problem hiding this comment.
At that point, it seems like we might just want to have two different params classes (which perhaps have a common superclass for the shared bits if desired). Having different callback function types for the two cases makes the most sense to me.
Keeping the existing API working on top of the new one seems pretty easy and would reduce the amount of code churn SDK clients have to have.
There was a problem hiding this comment.
Addressed these changes in 5d4ce20, LMK if there's anything else missing or if it should be done differently.
samadDotDev
force-pushed
the
upstream/fs/ocw-params
branch
from
1672d94
to
d29e4fc
Compare
|
PR #34209: Size comparison from 5a634f5 to d29e4fc Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Jul 6, 2024
| @@ -32,13 +32,24 @@ constexpr uint16_t kFabricAdminServerPort = 33001; | |||
| */ | |||
| CHIP_ERROR InitRpcClient(uint16_t rpcServerPort); | |||
|
|
|||
| struct CommissioningWindowParams | |||
There was a problem hiding this comment.
please make this a builder pattern. It should read code like:
OCW(
CommisioningWindowParams()
.SetNodeId(1234)
.SetDiscriminator(123)
.SetIterations(100)
)- make sure all members have initializers (if you do not know or they are required, set to 0 just in case)
- make this fluent builders to make it readable and not require pre-declaration
- It seems that @bzbarsky-apple mentioned some things are not supposed to work together. Add tests for that in the setters (probably VerifyOrDie is fine to avoid mis-use ... but at a minimum class coments describing use should exist here)
There was a problem hiding this comment.
Added fluent builder pattern, initialized some of the member variables, and added some commentary in 16bf72c, LMK if we need to go further in this route.
| @@ -47,6 +48,61 @@ class CommissioningWindowOpener | |||
| mDeviceConnectionFailure(&OnDeviceConnectionFailureCallback, this) | |||
| {} | |||
|
|
|||
| struct CommissioningWindowCommonParams | |||
| { | |||
| // The device Id of the node. | |||
There was a problem hiding this comment.
This should not be called "deviceId" if it's a nodeId. Please call this nodeId.
There was a problem hiding this comment.
Remnants from previous args, updated in the new parameters class here: 16bf72c
| * out parameter, will include the VID/PID bits if | ||
| * readVIDPIDAttributes is true. | ||
| */ | ||
| CHIP_ERROR OpenCommissioningWindow(const CommissioningWindowPasscodeParams & params, SetupPayload & payload); |
There was a problem hiding this comment.
Why have a passcode version? The verifier version beloew is sufficient.
There was a problem hiding this comment.
This is to move off the previous function with growing number of args (for the passcode version) as requested in #34209 (comment). The existing API is there only to reduce the external SDK client churn (could potentially mark it as deprecated in some form, or note for consumers to move off that?)
| * @param[in] params The parameters required to open an enhanced commissioning window | ||
| * with the provided PAKE passcode verifier. | ||
| */ | ||
| CHIP_ERROR OpenCommissioningWindow(const CommissioningWindowVerifierParams & params); |
There was a problem hiding this comment.
Please add tests that cover the new methods.
There was a problem hiding this comment.
Added some tests for the new methods mostly focused on the args: 5a9ae85
|
PR #34209: Size comparison from 5a634f5 to 16bf72c Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
yufengwangca
reviewed
Jul 9, 2024
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
Co-authored-by: Andrei Litvin <andy314@gmail.com>
Co-authored-by: Andrei Litvin <andy314@gmail.com>
samadDotDev
force-pushed
the
upstream/fs/ocw-params
branch
from
1707b16
to
5a9ae85
Compare
|
PR #34209: Size comparison from ecfa07a to 5a9ae85 Full report (47 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, mbed, nrfconnect, nxp, psoc6, qpg, stm32, tizen)
|
|
PR #34209: Size comparison from ecfa07a to b154a1b Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
yufengwangca
approved these changes
Jul 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This introduces an optional PAKE passcode verifier arg in the commissioning window opener to support the pass-through of this parameter to the device via admin from the OCW request received on the bridge (
fabric-bridge -RPC-> fabric-admin --> device). The RPC contract update between fabric admin and fabric bridge app to pass the OCW params and use of the new function arg is done in a separate commit.Note that the OCW command which is being proxied here doesn't contain the setup pin code to compute the verifier with included iterations count.
Fixes #33763