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

Remove Peer/local nodeid dependency for PASE #4945

Merged
merged 6 commits into from
Feb 23, 2021

Conversation

andy31415
Copy link
Contributor

Problem

At the time of PASE establishment, the peer does not yet belong to a fabric. Having node ids passeed around results in a larger coupling than necessary.

Summary of Changes

  • Removes nodeid references from PASE.
  • Removes checks for source/destination nodeids within rendezvous (remote may have no nodeid)
  • Remove the setting of destination node id if it is undefined during setting up the header for secure session management

Tested that chip-tool pairing ble can still pair devices.

@todo

This comment has been minimized.

@todo
Copy link

todo bot commented Feb 19, 2021

Once Operational credentials are implemented, node id assignment should be done during opcreds configuration.

// TODO: Once Operational credentials are implemented, node id assignment should be done during opcreds configuration.
// - can use internal node ids (0xFFFF_FFFE_xxxx_xxx - spec still being defined) if a temporary
// node id is required for indexing
// - should only assign a final node id as part of setting operational credentials
if (!mParams.GetRemoteNodeId().HasValue())
{
ChipLogError(Ble, "Missing node id in rendezvous parameters. Node ID is required until opcerts are implemented");
}
mPairingSession.PeerConnection().SetPeerNodeId(mParams.GetRemoteNodeId().ValueOr(kUndefinedNodeId));
CHIP_ERROR err = mSecureSessionMgr->NewPairing(


This comment was generated by todo based on a TODO comment in f8354fe in #4945. cc @andy31415.

@andy31415 andy31415 marked this pull request as draft February 19, 2021 16:41
@andy31415
Copy link
Contributor Author

Marked as draft for now - pairing succeeds, however remote device does not seem to be controllable. I believe we store pairings on the remote server as well and since we stopped transmitting the node id, the pairing is not available (chip-tool was reporting a correct node id).

@andy31415 andy31415 marked this pull request as ready for review February 19, 2021 17:00
@andy31415
Copy link
Contributor Author

Removed back from draft - manual testing of 'chip-tool provision + esp32 toggle (without reboot)' seems to work. So ready for review, I will debug cirque.

…nfo. Fixes cirque but now logic is highlighted as subject to change
@todo
Copy link

todo bot commented Feb 19, 2021

Admin information and node ID shouold be set during operation credential configuration

// TODO: Admin information and node ID shouold be set during operation credential configuration
// This setting of header properties is a hack to transmit the local node id to our peer
// so that admin configurations are preserved. Generally PASE sesions should not need to send
// node-ids as the peers may not be part of a fabric.
PacketHeader headerWithNodeIds = header;
if (mParams.HasLocalNodeId())
{
headerWithNodeIds.SetSourceNodeId(mParams.GetLocalNodeId().Value());
}


This comment was generated by todo based on a TODO comment in b05b895 in #4945. cc @andy31415.

@todo
Copy link

todo bot commented Feb 19, 2021

when operational certificates are in use, Rendezvous should not rely on destination node id.

// TODO: when operational certificates are in use, Rendezvous should not rely on destination node id.
// This marks internal key settings to identify the underlying key by the value that the
// remote node has sent.
// Generally such things should be saved as part of a CASE session, where operational credentials
// would be authenticated and matched against a remote node id.
// Also unclear why 'destination node id' is used here - it seems to be better ot use
// the source node id, which would identify the peer (rather than 'self' - rendezvous should
// already know the local node id).
if (packetHeader.GetDestinationNodeId().HasValue())
{
ChipLogProgress(Ble, "Received pairing message for %llu", packetHeader.GetDestinationNodeId().Value());


This comment was generated by todo based on a TODO comment in b05b895 in #4945. cc @andy31415.

@andy31415
Copy link
Contributor Author

@pan-apple - PTAL

@github-actions
Copy link

Size increase report for "gn_qpg6100-example-build" from 2d11bc4

File Section File VM
chip-qpg6100-lock-example.out .heap 0 16
chip-qpg6100-lock-example.out .bss 0 -16
chip-qpg6100-lock-example.out .text -464 -464
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lock-example.out and ./pull_artifact/chip-qpg6100-lock-example.out:

sections,vmsize,filesize
[Unmapped],0,464
.debug_abbrev,0,20
.heap,16,0
.shstrtab,0,1
.debug_aranges,0,-8
.bss,-16,0
.symtab,0,-48
.strtab,0,-113
.debug_frame,0,-136
.debug_ranges,0,-184
.debug_str,0,-310
.text,-464,-464
.debug_line,0,-1016
.debug_loc,0,-1082
.debug_info,0,-1820


@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 2d11bc4

File Section File VM
chip-lock.elf rodata 32 28
chip-lock.elf bss 0 -9
chip-lock.elf [LOAD #3 [RW]] 0 -23
chip-lock.elf text -480 -480
chip-lighting.elf rodata 24 28
chip-lighting.elf shell_root_cmds_sections -4 -4
chip-lighting.elf bss 0 -13
chip-lighting.elf [LOAD #3 [RW]] 0 -19
chip-lighting.elf text -476 -476
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
rodata,28,32
.debug_abbrev,0,24
.shstrtab,0,-3
bss,-9,0
[LOAD #3 [RW]],-23,0
.debug_ranges,0,-112
.strtab,0,-113
.debug_frame,0,-144
.debug_loc,0,-241
.debug_str,0,-311
text,-480,-480
.debug_line,0,-857
.debug_info,0,-1223

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
rodata,28,24
.debug_abbrev,0,24
.shstrtab,0,-3
shell_root_cmds_sections,-4,-4
bss,-13,0
[LOAD #3 [RW]],-19,0
.debug_ranges,0,-112
.strtab,0,-113
.debug_frame,0,-144
.debug_loc,0,-241
.debug_str,0,-311
text,-476,-476
.debug_line,0,-857
.debug_info,0,-1223


@github-actions
Copy link

Size increase report for "esp32-example-build" from 2d11bc4

File Section File VM
chip-all-clusters-app.elf .flash.rodata 32 32
chip-all-clusters-app.elf .dram0.bss 0 -16
chip-all-clusters-app.elf .flash.text -300 -300
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.xt.lit._ZNK4chip8OptionalIyE5ValueEv,0,80
.shstrtab,0,76
.xt.prop._ZN4chip8OptionalIyEaSERKS1_,0,76
.debug_abbrev,0,52
.symtab,0,48
.xt.prop._ZN4chip8Encoding22PacketBufferWriterBaseINS0_12BufferWriterEEC2EONS_6System18PacketBufferHandleE,0,40
.flash.rodata,32,32
.xt.prop._ZTVN4chip11DeviceLayer37DeviceNetworkProvisioningDelegateImplE,0,2
.dram0.bss,-16,0
[Unmapped],0,-32
.xt.prop._ZNK4chip20RendezvousParameters12IsControllerEv,0,-40
.xt.prop._ZNK4chip8OptionalIyE5ValueEv,0,-40
.strtab,0,-84
.debug_ranges,0,-224
.flash.text,-300,-300
.debug_str,0,-311
.debug_loc,0,-860
.debug_line,0,-1727
.debug_info,0,-1868

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize


Copy link
Contributor

@pan-apple pan-apple left a comment

Choose a reason for hiding this comment

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

Thanks @andy31415, this was a much needed cleanup!

Copy link
Contributor

@mspang mspang left a comment

Choose a reason for hiding this comment

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

We don't have CASE sessions yet. What is mDNS going to advertise if we don't have a node id ?

@andy31415
Copy link
Contributor Author

We don't have CASE sessions yet. What is mDNS going to advertise if we don't have a node id ?

I could not fully pull out nodeID - mdns advertisement will remain the same. The bits that I did take out were passing in nodeid within PASE session classes - generally less nodeids.

@mspang
Copy link
Contributor

mspang commented Feb 23, 2021

We don't have CASE sessions yet. What is mDNS going to advertise if we don't have a node id ?

I could not fully pull out nodeID - mdns advertisement will remain the same. The bits that I did take out were passing in nodeid within PASE session classes - generally less nodeids.

What node id do we advertise in mDNS in that case?

Here's what we need

  1. Device initially advertises as uncommissioned (in BLE or mDNS)
  2. Provisioning step assigns the operational id
  3. Device switches to advertising as commissioned with the id assigned in 2)

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.

5 participants