Update

src/controller/python/OpCredsBinding.cpp

@@ -374,8 +374,9 @@ void pychip_OnCommissioningStatusUpdate(chip::PeerId peerId, chip::Controller::C
 }
 
 /**
- * Call of pychip_OpCreds_AllocateControllerFoCustomCommissioningFlow allows allocating a device commissioner that will perform
- * commissioning without using the auto-commissioning flow present in the Controller class
+ * Allocates a controller that does not use auto-commisioning.
+ *
+ * TODO(#25214): Need clean up API
  *
  */
 PyChipError pychip_OpCreds_AllocateControllerForPythonCommissioningFLow(chip::Controller::DeviceCommissioner ** outDevCtrl,
@@ -385,8 +386,6 @@ PyChipError pychip_OpCreds_AllocateControllerForPythonCommissioningFLow(chip::Co
                                                                         const uint8_t * ipk, uint32_t ipkLen,
                                                                         chip::VendorId adminVendorId, bool enableServerInteractions)
 {
-    CHIP_ERROR err = CHIP_NO_ERROR;
-
     ReturnErrorCodeIf(nocLen > Controller::kMaxCHIPDERCertLength, ToPyChipError(CHIP_ERROR_NO_MEMORY));
     ReturnErrorCodeIf(icacLen > Controller::kMaxCHIPDERCertLength, ToPyChipError(CHIP_ERROR_NO_MEMORY));
     ReturnErrorCodeIf(rcacLen > Controller::kMaxCHIPDERCertLength, ToPyChipError(CHIP_ERROR_NO_MEMORY));
@@ -408,7 +407,7 @@ PyChipError pychip_OpCreds_AllocateControllerForPythonCommissioningFLow(chip::Co
     initParams.permitMultiControllerFabrics         = true;
     initParams.hasExternallyOwnedOperationalKeypair = true;
 
-    err = Controller::DeviceControllerFactory::GetInstance().SetupCommissioner(initParams, *devCtrl);
+    CHIP_ERROR err = Controller::DeviceControllerFactory::GetInstance().SetupCommissioner(initParams, *devCtrl);
     VerifyOrReturnError(err == CHIP_NO_ERROR, ToPyChipError(err));
 
     // Setup IPK in Group Data Provider for controller after Commissioner init which sets-up the fabric table entry
@@ -433,6 +432,7 @@ PyChipError pychip_OpCreds_AllocateControllerForPythonCommissioningFLow(chip::Co
     return ToPyChipError(CHIP_NO_ERROR);
 }
 
+// TODO(#25214): Need clean up API
 PyChipError pychip_OpCreds_AllocateController(OpCredsContext * context, chip::Controller::DeviceCommissioner ** outDevCtrl,
                                               FabricId fabricId, chip::NodeId nodeId, chip::VendorId adminVendorId,
                                               const char * paaTrustStorePath, bool useTestCommissioner,

src/controller/python/chip/ChipDeviceCtrl.py

@@ -1330,7 +1330,10 @@ class ChipDeviceController(ChipDeviceControllerBase):
     '''
 
     def __init__(self, opCredsContext: ctypes.c_void_p, fabricId: int, nodeId: int, adminVendorId: int, catTags: typing.List[int] = [], paaTrustStorePath: str = "", useTestCommissioner: bool = False, fabricAdmin: FabricAdmin = None, name: str = None, keypair: p256keypair.P256Keypair = None):
-        super().__init__("caIndex(%x)/fabricId(0x%016X)/nodeId(0x%016X)" % (fabricAdmin.caIndex, fabricId, nodeId) if name is None else name)
+        super().__init__(
+            name or
+            f"caIndex({fabricAdmin.caIndex:x})/fabricId(0x{fabricId:016X})/nodeId(0x{nodeId:016X})"
+        )
 
         self._dmLib.pychip_DeviceController_SetIssueNOCChainCallbackPythonCallback(_IssueNOCChainCallbackPythonCallback)
 
@@ -1342,6 +1345,7 @@ def __init__(self, opCredsContext: ctypes.c_void_p, fabricId: int, nodeId: int,
             c_catTags[i] = item
 
         # TODO(erjiaqing@): Figure out how to control enableServerInteractions for a single device controller (node)
+        self._externalKeyPair = keypair
         self._ChipStack.Call(
             lambda: self._dmLib.pychip_OpCreds_AllocateController(c_void_p(
                 opCredsContext), pointer(devCtrl), fabricId, nodeId, adminVendorId, c_char_p(None if len(paaTrustStorePath) == 0 else str.encode(paaTrustStorePath)), useTestCommissioner, self._ChipStack.enableServerInteractions, c_catTags, len(catTags), None if keypair is None else keypair.native_object)
@@ -1367,7 +1371,18 @@ def caIndex(self) -> int:
     def fabricAdmin(self) -> FabricAdmin:
         return self._fabricAdmin
 
-    def Commission(self, nodeid):
+    def Commission(self, nodeid) -> bool:
+        '''
+        Start the auto-commissioning process on a node after establishing a PASE connection.
+        This function is intended to be used in conjunction with `EstablishPASESessionBLE` or
+        `EstablishPASESessionIP`. It can be called either before or after the DevicePairingDelegate
+        receives the OnPairingComplete call. Commissioners that want to perform simple
+        auto-commissioning should use the supplied "PairDevice" functions above, which will
+        establish the PASE connection and commission automatically.
+
+        Return:
+          bool: True if successful, False otherwise.
+        '''
         self.CheckIsActive()
         self._ChipStack.commissioningCompleteEvent.clear()
         self.state = DCState.COMMISSIONING
@@ -1376,10 +1391,7 @@ def Commission(self, nodeid):
             lambda: self._dmLib.pychip_DeviceController_Commission(
                 self.devCtrl, nodeid)
         )
-        if not self._ChipStack.commissioningCompleteEvent.isSet():
-            # Error 50 is a timeout
-            return False
-        return self._ChipStack.commissioningEventRes == 0
+        return (self._ChipStack.commissioningCompleteEvent.isSet() and (self._ChipStack.commissioningEventRes == 0))
 
     def CommissionThread(self, discriminator, setupPinCode, nodeId, threadOperationalDataset: bytes):
         ''' Commissions a Thread device over BLE
@@ -1518,10 +1530,12 @@ def __init__(self, operationalKey: p256keypair.P256Keypair, noc: bytes, icac: ty
             adminVendorId: The adminVendorId of the controller.
             name: The name of the controller, for debugging use only.
         '''
-        super().__init__(f"ctrl(v/{adminVendorId})" if name is None else name)
+        super().__init__(name or f"ctrl(v/{adminVendorId})")
 
         devCtrl = c_void_p(None)
 
+        # Device should hold a reference to the key to avoid it being GC-ed.
+        self._externalKeyPair = operationalKey
         nativeKey = operationalKey.create_native_object()
 
         self._ChipStack.Call(

src/controller/python/chip/commissioning/pase.py

@@ -55,7 +55,9 @@ def establish_session(devCtrl: ChipDeviceCtrl.ChipDeviceControllerBase, paramete
         selected_address = None
         for ip in device[0].addresses:
             if ipaddress.ip_address(ip).is_link_local:
-                # TODO(erjiaqing): The link local address in the discovery result does not contain a interface
+                # TODO(erjiaqing): To connect a device using link local address requires an interface identifier,
+                # however, the link local address returned from DiscoverCommissionableNodes does not have an
+                # interface identifier.
                 continue
             selected_address = ip
             break

src/controller/python/chip/crypto/p256keypair.cpp

@@ -1,7 +1,25 @@
+/*
+ *
+ *    Copyright (c) 2022 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
 
 #include <controller/python/chip/crypto/p256keypair.h>
 
 #include <cassert>
+#include <lib/support/Span.h>
 #include <lib/support/logging/CHIPLogging.h>
 
 using namespace chip;
@@ -14,11 +32,14 @@ pychip_P256Keypair::pychip_P256Keypair(void * aPyContext, pychip_P256Keypair_ECD
     mSignMsgFunct(aSignMsgFunct), mDeriveSecretFunct(aDeriveSecretFunct)
 {}
 
-pychip_P256Keypair::~pychip_P256Keypair() {}
+pychip_P256Keypair::~pychip_P256Keypair()
+{
+    // Just override the initialize routing to avoid calling the Initialize from the platform's code.
+}
 
 CHIP_ERROR pychip_P256Keypair::Initialize(Crypto::ECPKeyTarget key_target)
 {
-    // Just override the initialize routing of the platform implementation.
+    // Just override the initialize routing to avoid calling the Initialize from the platform's code.
     return CHIP_NO_ERROR;
 }
 
@@ -64,10 +85,9 @@ CHIP_ERROR pychip_P256Keypair::ECDH_derive_secret(const Crypto::P256PublicKey &
     return CHIP_NO_ERROR;
 }
 
-void pychip_P256Keypair::UpdatePubkey(uint8_t * publicKey)
+void pychip_P256Keypair::UpdatePubkey(const FixedByteSpan<kP256_PublicKey_Length> & aPublicKey)
 {
-    memcpy(mPublicKey.Bytes(), publicKey, mPublicKey.Length());
-
+    mPublicKey   = aPublicKey;
     mInitialized = true;
 }
 
@@ -79,7 +99,14 @@ chip::python::pychip_P256Keypair * pychip_NewP256Keypair(void * pyObject, pychip
     return res;
 }
 
-void pychip_P256Keypair_UpdatePubkey(chip::python::pychip_P256Keypair * this_, uint8_t * aPubKey)
+PyChipError pychip_P256Keypair_UpdatePubkey(chip::python::pychip_P256Keypair * this_, uint8_t * aPubKey, size_t aPubKeyLen)
+{
+    VerifyOrReturnError(aPubKeyLen == kP256_PublicKey_Length, ToPyChipError(CHIP_ERROR_INVALID_ARGUMENT));
+    this_->UpdatePubkey(FixedByteSpan<Crypto::kP256_PublicKey_Length>(aPubKey));
+    return ToPyChipError(CHIP_NO_ERROR);
+}
+
+void pychip_DeleteP256Keypair(chip::python::pychip_P256Keypair * this_)
 {
-    this_->UpdatePubkey(aPubKey);
+    delete this_;
 }

src/controller/python/chip/crypto/p256keypair.h

@@ -1,3 +1,21 @@
+/*
+ *
+ *    Copyright (c) 2022 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
 #pragma once
 
 #include <inttypes.h>
@@ -76,7 +94,7 @@ class pychip_P256Keypair : public Crypto::P256Keypair
      * @param publicKey A buffer of publicKey, should have exactly `kP256_PublicKey_Length` bytes.
      *
      **/
-    void UpdatePubkey(uint8_t * publicKey);
+    void UpdatePubkey(const FixedByteSpan<Crypto::kP256_PublicKey_Length> & aPublicKey);
 
     /** @brief Return public key for the keypair.
      **/
@@ -101,5 +119,6 @@ extern "C" {
 chip::python::pychip_P256Keypair * pychip_NewP256Keypair(void * pyObject, pychip_P256Keypair_ECDSA_sign_msg aSignMsgFunct,
                                                          pychip_P256Keypair_ECDH_derive_secret aDeriveSecretFunct);
 
-void pychip_P256Keypair_UpdatePubkey(chip::python::pychip_P256Keypair * this_, uint8_t * aPubKey);
+PyChipError pychip_P256Keypair_UpdatePubkey(chip::python::pychip_P256Keypair * this_, uint8_t * aPubKey, size_t aPubKeyLen);
+void pychip_DeleteP256Keypair(chip::python::pychip_P256Keypair * this_);
 }

src/controller/python/chip/crypto/p256keypair.py

@@ -1,3 +1,20 @@
+#
+#    Copyright (c) 2023 Project CHIP Authors
+#    All rights reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License");
+#    you may not use this file except in compliance with the License.
+#    You may obtain a copy of the License at
+#
+#        http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS,
+#    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#    See the License for the specific language governing permissions and
+#    limitations under the License.
+#
+
 import abc
 import hashlib
 from ctypes import *
@@ -15,7 +32,7 @@
 
 
 @ _pychip_P256Keypair_ECDSA_sign_msg_func
-def _pychip_ECDSA_sign_msg(self_: 'P256Keypair', message_buf: POINTER(c_uint8), message_size: int, signature_buf: POINTER(c_uint8), signature_buf_size: POINTER(c_uint32)) -> bool:
+def _pychip_ECDSA_sign_msg(self_: 'P256Keypair', message_buf: POINTER(c_uint8), message_size: int, signature_buf: POINTER(c_uint8), signature_buf_size: POINTER(c_size_t)) -> bool:
     res = self_.ECDSA_sign_msg(string_at(message_buf, message_size)[:])
     memmove(signature_buf, res, len(res))
     signature_buf_size.content = len(res)
@@ -31,24 +48,41 @@ def _pychip_ECDH_derive_secret(self_: 'P256Keypair', remote_pubkey: POINTER(c_ui
 
 
 class P256Keypair:
+    """Represented a P256Keypair, should live longer than the one using it.
+
+    Users are expected to hold a reference to the Keypair object.
+
+    """
+
     def __init__(self):
         self._native_obj = None
 
+    def __copy__(self):
+        raise NotImplementedError("P256Keypair should not be copied.")
+
+    def __deepcopy__(self, _=None):
+        raise NotImplementedError("P256Keypair should not be copied.")
+
     def _create_native_object(self) -> c_void_p:
         handle = native.GetLibraryHandle()
         if not handle.pychip_NewP256Keypair.argtypes:
             setter = native.NativeLibraryHandleMethodArguments(handle)
             setter.Set("pychip_NewP256Keypair", c_void_p, [py_object,
                        _pychip_P256Keypair_ECDSA_sign_msg_func, _pychip_P256Keypair_ECDH_derive_secret_func])
-            setter.Set("pychip_P256Keypair_UpdatePubkey", None, [c_void_p])
+            setter.Set("pychip_P256Keypair_UpdatePubkey", native.PyChipError, [c_void_p, POINTER(c_char), c_size_t])
+            setter.Set("pychip_DeleteP256Keypair", None, [c_void_p])
         self._native_obj = handle.pychip_NewP256Keypair(
             py_object(self), _pychip_ECDSA_sign_msg, _pychip_ECDH_derive_secret)
 
-        pythonapi.Py_IncRef(py_object(self))
-
         self.UpdatePublicKey()
         return self._native_obj
 
+    def __del__(self):
+        if self._native_obj is not None:
+            handle = native.GetLibraryHandle()
+            handle.pychip_DeleteP256Keypair(c_void_p(self._native_obj))
+            self._native_obj = None
+
     @property
     def native_object(self) -> c_void_p:
         if self._native_obj is None:
@@ -62,9 +96,9 @@ def UpdatePublicKey(self) -> None:
         generates a new keypair.
         '''
         handle = native.GetLibraryHandle()
-        handle.pychip_P256Keypair_UpdatePubkey(c_void_p(self.native_object), self.public_key)
+        handle.pychip_P256Keypair_UpdatePubkey(c_void_p(self.native_object), self.public_key, len(self.public_key)).raise_on_error()
 
-    @ abc.abstractproperty
+    @abc.abstractproperty
     def public_key(self) -> bytes:
         ''' Returns the public key of the key pair
 
@@ -76,11 +110,11 @@ def public_key(self) -> bytes:
         '''
         raise NotImplementedError()
 
-    @ abc.abstractmethod
+    @abc.abstractmethod
     def ECDSA_sign_msg(self, message: bytes) -> bytes:
         raise NotImplementedError()
 
-    @ abc.abstractmethod
+    @abc.abstractmethod
     def ECDH_derive_secret(self, remote_pubkey: bytes) -> bytes:
         ''' Derive shared secret from the local private key and remote public key.