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

[border-agent] mechanism to use ephemeral key #9435

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

abtink
Copy link
Member

@abtink abtink commented Sep 19, 2023

This commit adds a new mechanism in BorderAgent to allow the use of ephemeral key. New otBorderAgentSetEphemeralKey API is added to allow user to set an ephemeral key. The ephemeral key is used instead of PSKc from Operation Dataset for a given timeout duration. New API otBorderAgentClearEphemeralKey allows users to cancel the ephemeral key before its timeout expires. While the timeout interval is in effect, the ephemeral key can be used only once by an external commissioner to connect. Once the commissioner disconnects, the ephemeral key is cleared, and Border Agent reverts to using PSKc.

This commit adds a callback mechanism to signal changes related to the Border Agent's use of an ephemeral key. It is invoked when the BA starts/stops using the key, or when parameters (e.g., port number) change.

This commit also adds CLI command under ba ephemeralkey for the new APIs.


Related to SPEC-1216

@size-report
Copy link

size-report bot commented Sep 19, 2023

Size Report of OpenThread

Merging #9435 into main(88b2b5c).

name branch text data bss total
ot-cli-ftd main 465664 760 66332 532756
#9435 466784 760 66364 533908
+/- +1120 0 +32 +1152
ot-ncp-ftd main 435652 760 61544 497956
#9435 436124 760 61576 498460
+/- +472 0 +32 +504
libopenthread-ftd.a main 234945 0 40278 275223
#9435 235707 0 40310 276017
+/- +762 0 +32 +794
libopenthread-cli-ftd.a main 56919 0 8075 64994
#9435 57336 0 8075 65411
+/- +417 0 0 +417
libopenthread-ncp-ftd.a main 31855 0 5916 37771
#9435 31855 0 5916 37771
+/- 0 0 0 0
ot-cli-mtd main 364640 760 51220 416620
#9435 364640 760 51220 416620
+/- 0 0 0 0
ot-ncp-mtd main 347292 760 46448 394500
#9435 347292 760 46448 394500
+/- 0 0 0 0
libopenthread-mtd.a main 158177 0 25182 183359
#9435 158177 0 25182 183359
+/- 0 0 0 0
libopenthread-cli-mtd.a main 39743 0 8059 47802
#9435 39743 0 8059 47802
+/- 0 0 0 0
libopenthread-ncp-mtd.a main 24735 0 5916 30651
#9435 24735 0 5916 30651
+/- 0 0 0 0
ot-cli-ftd-br main 534248 768 130876 665892
#9435 535384 768 130900 667052
+/- +1136 0 +24 +1160
libopenthread-ftd-br.a main 298180 5 104798 402983
#9435 298910 5 104822 403737
+/- +730 0 +24 +754
libopenthread-cli-ftd-br.a main 70552 0 8099 78651
#9435 70969 0 8099 79068
+/- +417 0 0 +417
ot-rcp main 62248 564 20604 83416
#9435 62248 564 20604 83416
+/- 0 0 0 0
libopenthread-rcp.a main 9542 0 5052 14594
#9435 9542 0 5052 14594
+/- 0 0 0 0
libopenthread-radio.a main 18821 0 214 19035
#9435 18821 0 214 19035
+/- 0 0 0 0

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Attention: Patch coverage is 67.82609% with 37 lines in your changes are missing coverage. Please review.

Project coverage is 78.35%. Comparing base (41526d5) to head (6eaced0).

❗ Current head 6eaced0 differs from pull request most recent head 6c579be. Consider uploading reports for the commit 6c579be to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9435      +/-   ##
==========================================
+ Coverage   72.14%   78.35%   +6.21%     
==========================================
  Files         362      566     +204     
  Lines       37390    82181   +44791     
==========================================
+ Hits        26975    64394   +37419     
- Misses      10415    17787    +7372     
Files Coverage Δ
src/cli/cli.hpp 100.00% <ø> (+2.22%) ⬆️
src/core/api/border_agent_api.cpp 17.80% <70.00%> (+17.80%) ⬆️
src/core/meshcop/border_agent.hpp 45.45% <40.00%> (+20.45%) ⬆️
src/core/meshcop/border_agent.cpp 28.57% <78.57%> (+15.28%) ⬆️
src/cli/cli.cpp 76.76% <46.66%> (+30.70%) ⬆️

... and 528 files with indirect coverage changes

src/cli/README.md Outdated Show resolved Hide resolved
src/cli/README.md Outdated Show resolved Hide resolved
src/cli/README.md Outdated Show resolved Hide resolved
@nand-nor
Copy link

nand-nor commented Sep 20, 2023

I know this is just an initial POC but wondering if there are plans to allow the caller to specify the timeout, ideally dynamically or as a static compile time value in the build config? Having a dedicated port for this may also be beneficial assuming that does not add too much additional complexity

@abtink
Copy link
Member Author

abtink commented Sep 20, 2023

I know this is just an initial POC but wondering if there are plans to allow the caller to specify the timeout, ideally dynamically or as a static compile time value in the build config? Having a dedicated port for this may also be beneficial assuming that does not add too much additional complexity

We can set the timeout value in otBorderAgentSetEphemeralKey() API and if set to zero, we then use a default value. I set the default as 2 min for now (defined as a fixed constant).

About port number, I think it may be a good idea to allow caller to set the port as well in the API call. Can add this later.

@abtink
Copy link
Member Author

abtink commented Sep 20, 2023

The new push changes the API so that the input aPassphrase is directly used as the PSK to make it easier to use.

@nand-nor
Copy link

Agree with allowing the user to set the port. A future extension this enables would be to allow something like an optional rate limit policy on the port (enforced by the ot process) just for example. Plus other nice security auditing features and increased ability to isolate and debug problems

@abtink abtink force-pushed the ba/ephmral-psk branch 2 times, most recently from 2cac95e to 5980995 Compare September 21, 2023 00:04
@abtink
Copy link
Member Author

abtink commented Sep 21, 2023

In new pushed commit, added a aUdpPort input to otBorderAgentSetEphemeralKey() API to specify the UDP port to use with ephemeral key. If zero, an ephemeral port will be used by Agent and we can get the selected port from existing otBorderAgentGetUdpPort().

src/cli/README.md Outdated Show resolved Hide resolved
@EskoDijk
Copy link
Contributor

For the timeout value setting, should we apply a maximum timeout? Something in the order of 10 minutes? This is to ensure that accidentally a very long timeout is set for the ephemeral key then after some waiting time a Commissioner X is able to connect again to the BR. This Commissioner X may be using the regular PSKc saved in the app to connect.

The ephemeral key in this case was maybe not captured by the user (e.g. the process failed: phone couldn't scan QR code, or user mistyped a displayed code, or user got distracted halfway through the process and did not get back to it etc.) Now the user tries to connect back using the good old Commission X which suddenly doesn't work anymore.
On the other hand a restart of the BR would also fix the problem.

@abtink
Copy link
Member Author

abtink commented Sep 25, 2023

For the timeout value setting, should we apply a maximum timeout? Something in the order of 10 minutes? This is to ensure that accidentally a very long timeout is set for the ephemeral key then after some waiting time a Commissioner X is able to connect again to the BR. This Commissioner X may be using the regular PSKc saved in the app to connect.

Good idea. Will add 10 minute max timeout in next push. Would be good to define the max in spec as well.
If user gives longer value we clamp to the max .

@abtink
Copy link
Member Author

abtink commented Dec 6, 2023

In latest push updated the code to ensure ephemeral key can be used once. Updated the API/CLI documentation to mention this.

src/cli/README.md Outdated Show resolved Hide resolved
src/cli/README.md Outdated Show resolved Hide resolved
src/cli/README.md Outdated Show resolved Hide resolved
src/cli/README.md Outdated Show resolved Hide resolved
src/cli/README.md Outdated Show resolved Hide resolved
src/core/meshcop/border_agent.cpp Show resolved Hide resolved
src/core/meshcop/border_agent.cpp Outdated Show resolved Hide resolved
include/openthread/border_agent.h Show resolved Hide resolved
@abtink abtink force-pushed the ba/ephmral-psk branch 2 times, most recently from 993f603 to 6eaced0 Compare February 28, 2024 19:22
Copy link
Contributor

@librasungirl librasungirl left a comment

Choose a reason for hiding this comment

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

👍

src/core/meshcop/border_agent.hpp Outdated Show resolved Hide resolved
include/openthread/border_agent.h Outdated Show resolved Hide resolved
include/openthread/border_agent.h Outdated Show resolved Hide resolved
include/openthread/border_agent.h Outdated Show resolved Hide resolved
src/cli/README.md Outdated Show resolved Hide resolved
include/openthread/border_agent.h Outdated Show resolved Hide resolved
tests/toranj/cli/cli.py Outdated Show resolved Hide resolved
This commit adds a new mechanism in `BorderAgent` to allow the use of
ephemeral key. New `otBorderAgentSetEphemeralKey` API is added to
allow user to set an ephemeral key. The ephemeral key is used
instead of PSKc from Operation Dataset for a given timeout duration.
New API `otBorderAgentClearEphemeralKey` allows users to cancel the
ephemeral key before its timeout expires. While the timeout interval
is in effect, the ephemeral key can be used only once by an external
commissioner to connect. Once the commissioner disconnects, the
ephemeral key is cleared, and Border Agent reverts to using PSKc.

This commit adds a callback mechanism to signal changes related to the
Border Agent's (BA) use of an ephemeral key. It is invoked when the
BA starts/stops using the key, or when parameters (e.g., port number)
change.

This commit also adds CLI command under `ba ephemeralkey` for the new
APIs along with test script validating the new APIs.
@jwhui jwhui merged commit 44c3906 into openthread:main Mar 5, 2024
101 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants