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

nodeID should be created by the commissioner APIs but not by the controller apps #7540

Closed
cjandhyala opened this issue Jun 11, 2021 · 6 comments
Labels
commissioning Involves placing devices on the network, initial setup controller spec Mismatch between spec and implementation V1.0

Comments

@cjandhyala
Copy link
Contributor

Per 0.7 spec referred below (page 224) nodeID should be unique within the fabric, and fabricID is scoped to a RootCA.

Right now both python controller and chip-tool are hardcoding/randomly generating it in the app layer, while this should be left to SDK layer Commissioner APIs to make sure the nodeID generated is indeed unique to that fabric. Current implementation doesn't guarantee that.

Screen Shot 2021-06-10 at 5 22 00 PM

Screen Shot 2021-06-10 at 5 23 44 PM

Proposed Solution

<suggested fix, suggested enhancement>

@tcarmelveilleux
Copy link
Contributor

The fundamental issue is the OperationalCredentialsDelegate has the wrong public interface, which fundamentally assume a "test" flow which has the controller assign the NodeID. There is no possibility of having the actual device attestation as part of the CSR processing by a CA, based on the public interface signature:

/**
     * @brief
     *   This function processes the CSR sent by the device.
     *   (Reference: Specifications section 11.22.5.8. OpCSR Elements)
     *
     * @param[in] CSR             The Certificate Signing Request.
     * @param[in] CSRNonce        The Nonce sent by us when we requested the CSR.
     * @param[in] VendorReserved1 vendor-specific information that may aid in device commissioning.
     * @param[in] VendorReserved2 vendor-specific information that may aid in device commissioning.
     * @param[in] VendorReserved3 vendor-specific information that may aid in device commissioning.
     * @param[in] Signature       Cryptographic signature generated for all the above fields.
     */
    CHIP_ERROR ProcessOpCSR(const ByteSpan & CSR, const ByteSpan & CSRNonce, const ByteSpan & VendorReserved1,
                            const ByteSpan & VendorReserved2, const ByteSpan & VendorReserved3, const ByteSpan & Signature);

The DA signature is over the entire OpCSR elements, and having them split up prevents proper verification.

Similarly:

    /**
     * @brief
     *   This function generates an operational certificate for the given node.
     *   The API generates the certificate in X.509 DER format.
     *
     *   The delegate is expected to use the certificate authority whose certificate
     *   is returned in `GetIntermediateCACertificate()` or `GetRootCACertificate()`
     *   API calls.
     *
     * @param[in] peerId       Node ID and Fabric ID of the target device.
     * @param[in] csr          Certificate Signing Request from the node in DER format.
     * @param[in] serialNumber Serial number to assign to the new certificate.
     * @param[in] certBuf      The API will fill in the generated cert in this buffer. The buffer is allocated by the caller.
     * @param[in] certBufSize  The size of certBuf buffer.
     * @param[out] outCertLen  The size of the actual certificate that was written in the certBuf.
     *
     * @return CHIP_ERROR CHIP_NO_ERROR on success, or corresponding error code.
     */
    virtual CHIP_ERROR GenerateNodeOperationalCertificate(const PeerId & peerId, const ByteSpan & csr, int64_t serialNumber,
                                                          uint8_t * certBuf, uint32_t certBufSize, uint32_t & outCertLen) = 0;

The above API is called directly from inside the commissioner logic in CHIPDeviceController.cpp's ProcessOpCSR(), where the local commissioner assigns a node ID and serial number to the opcert, which is the reverse of the flow.

Suggested fix:

  • Separate OperationalCredentialsDelegate into more pieces, including a local sample CA interface.
  • If there is sample code, this should be delegate to a new CA sample interface called from the OperationalCredentialsDelegate, so that the core elements passed down are:
    <CSR Information, CSR attestation signature, Attestation Challenge, DAC cert chain>. Those are what is needed to generate a legitimate operational certificate, and do so with device attestation in the picture.

@thuannd4199
Copy link

Per 0.7 spec referred below (page 224) nodeID should be unique within the fabric, and fabricID is scoped to a RootCA.

Right now both python controller and chip-tool are hardcoding/randomly generating it in the app layer, while this should be left to SDK layer Commissioner APIs to make sure the nodeID generated is indeed unique to that fabric. Current implementation doesn't guarantee that.

Screen Shot 2021-06-10 at 5 22 00 PM

Screen Shot 2021-06-10 at 5 23 44 PM

Proposed Solution

<suggested fix, suggested enhancement>

Could you tell my how might I get the spec?

@msandstedt
Copy link
Contributor

This is trivially addressable for integrating commissioner apps with the approach introduced in #13294.

@woody-apple woody-apple linked a pull request Jan 25, 2022 that will close this issue
@woody-apple woody-apple added V1.0 spec Mismatch between spec and implementation labels Jan 25, 2022
@holbrookt
Copy link
Contributor

@msandstedt @cjandhyala is this still an issue? Does it need to be fixed for 1.0?

@holbrookt holbrookt added controller v1_secondary_triage commissioning Involves placing devices on the network, initial setup and removed v1_triage_split_9 labels Feb 1, 2022
@msandstedt
Copy link
Contributor

msandstedt commented Feb 1, 2022

I believe we can now declare this as fixed with #13801, for which I had opened #13500.

Most recently, commissioners could specify fabric and node IDs to signing infrastructure, which allows for one type of solution where a local entity manages IDs and prevents collisions on fabrics.

But now signing infrastructure can also specify fabric and node IDs to commissioners, as the commissioner code will now extract these from certificates passed back through the operational credentials delegate.

@cjandhyala,

Let me know if you have any questions about this solution.

@woody-apple
Copy link
Contributor

Marking as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commissioning Involves placing devices on the network, initial setup controller spec Mismatch between spec and implementation V1.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants