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

Support passing csr nonce into commissioning flow #7696

Conversation

ritikananda27
Copy link
Contributor

@ritikananda27 ritikananda27 commented Jun 16, 2021

Problem

Accepting a CSRNonce during the Commissioning process instead of auto generating it.

Solution

Passing the OpCSRNonce as part of RendezvousParameters.h to be used in the commissioning process. If the OpCSRNonce is not passed, a random value is generated and used.

@CLAassistant
Copy link

CLAassistant commented Jun 16, 2021

CLA assistant check
All committers have signed the CLA.

@woody-apple
Copy link
Contributor

/rebase

@todo
Copy link

todo bot commented Jun 18, 2021

(#5098) We should pass the Command Object and EndpointId to the cluster callbacks.

// TODO(#5098) We should pass the Command Object and EndpointId to the cluster callbacks.
wasHandled = emberAfThreadNetworkDiagnosticsClusterResetCountsCallback(apCommandObj);
break;
}
default: {
// Unrecognized command ID, error status will apply.
chip::app::CommandPathParams returnStatusParam = { aEndpointId,
0, // GroupId
ZCL_THREAD_NETWORK_DIAGNOSTICS_CLUSTER_ID, aCommandId,
(chip::app::CommandPathFlags::kEndpointIdValid) };
apCommandObj->AddStatusCode(returnStatusParam, Protocols::SecureChannel::GeneralStatusCode::kNotFound,


This comment was generated by todo based on a TODO comment in 4b68052 in #7696. cc @ritikananda27.

@todo
Copy link

todo bot commented Jun 18, 2021

Use Android keystore system instead of direct storage of private key and add specific errors to check if a specified item is not found in the keystore.

// TODO: Use Android keystore system instead of direct storage of private key and add specific errors to check if a specified item is not found in the keystore.
if (SyncGetKeyValue(kOperationalCredentialsIssuerKeypairStorage, &serializedKey, keySize) != CHIP_NO_ERROR)
{
// If storage doesn't have an existing keypair, create one and add it to the storage.
ReturnErrorOnFailure(mIssuer.Initialize());
ReturnErrorOnFailure(mIssuer.Serialize(serializedKey));
keySize = static_cast<uint16_t>(sizeof(serializedKey));
SyncSetKeyValue(kOperationalCredentialsIssuerKeypairStorage, &serializedKey, keySize);
}
else
{


This comment was generated by todo based on a TODO comment in 329efbb in #7696. cc @ritikananda27.

src/controller/CHIPDevice.h Outdated Show resolved Hide resolved
src/controller/CHIPDevice.h Outdated Show resolved Hide resolved
src/controller/CHIPDeviceController.cpp Outdated Show resolved Hide resolved
src/controller/java/AndroidDeviceControllerWrapper.cpp Outdated Show resolved Hide resolved
jbyteArray argument;
GetEnvForCurrentThread()->ExceptionClear();
N2J_ByteArray(GetEnvForCurrentThread(), csr.data(),csr.size(),argument);
GetEnvForCurrentThread()->CallVoidMethod(mJavaObjectRef, method, argument);
Copy link
Contributor

Choose a reason for hiding this comment

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

What if generateCert failed? is this still OK to be called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, the method CHIPCert.NewNodeOperationalX509Cert will generate a CHIP_Error if the cert generation fails and so will GenerateNodeOperationalCertificate. But the whole relevance of adding this method here for my use case is to trigger the generation of OpCSR using the passed nonce. The generated cert is not useful. Ideally the generated CSRNonce will be exchanged to get an OpCert which needs to be written to the device in a different api call (after the PairDevice completes and that api does not exist today).

It would have been ideal to have a separate api to just pass the CSRNonce and get the CSRResponse but with current sdk support this is the only possible path.

Lemme know if this makes sense.

src/controller/java/JniReferences.cpp Outdated Show resolved Hide resolved
@todo
Copy link

todo bot commented Jun 22, 2021

Refactor this API to match latest spec, so that GenerateNodeOperationalCertificate receives the full CSR Elements data payload.

// TODO Refactor this API to match latest spec, so that GenerateNodeOperationalCertificate receives the full CSR Elements data payload.
CHIP_ERROR AndroidDeviceControllerWrapper::GenerateNodeOperationalCertificate(const chip::PeerId & peerId, const chip::ByteSpan & csr,
int64_t serialNumber, uint8_t * certBuf,
uint32_t certBufSize, uint32_t & outCertLen)
{
jmethodID method;
CHIP_ERROR err = CHIP_NO_ERROR;
err = FindMethod(JniReferences::GetEnvForCurrentThread(), mJavaObjectRef, "onOpCSRGenerationComplete", "([B)V", &method);
if (err != CHIP_NO_ERROR)
{
ChipLogError(Controller, "Error invoking onOpCSRGenerationComplete: %d", err);


This comment was generated by todo based on a TODO comment in 2955ad9 in #7696. cc @ritikananda27.

@ritikananda27 ritikananda27 marked this pull request as ready for review June 22, 2021 00:29
src/controller/CHIPDevice.h Outdated Show resolved Hide resolved
src/controller/CHIPDevice.h Outdated Show resolved Hide resolved
src/controller/CHIPDeviceController.cpp Outdated Show resolved Hide resolved
@todo
Copy link

todo bot commented Jun 22, 2021

Abstract the CSRNonce generation from the CHIPDevice and let the controller always provide it.

// TODO Abstract the CSRNonce generation from the CHIPDevice and let the controller always provide it.
// This will entail, making the CSRNonce a mandatory init param and also fixing the flow which currently depends on the auto generation of the CSRNonce.
device->GenerateCSRNonce();
}
mIsIPRendezvous = (params.GetPeerAddress().GetTransportType() != Transport::Type::kBle);
err = mPairingSession.MessageDispatch().Init(mTransportMgr);


This comment was generated by todo based on a TODO comment in b04a179 in #7696. cc @ritikananda27.

src/controller/CHIPDevice.h Outdated Show resolved Hide resolved
env->SetByteArrayRegion(outArray, 0, inArrayLen, (jbyte *) inArray);
VerifyOrExit(!env->ExceptionCheck(), err = CHIP_JNI_ERROR_EXCEPTION_THROWN);

exit:
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer using Return macros instead of goto exit macros when there is no cleanup to be done in the exit block. Saves some scoping and helps readability.

@bzbarsky-apple
Copy link
Contributor

Needs to be rebased on top of #7933 once that lands...

@bzbarsky-apple
Copy link
Contributor

/rebase

@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 5ab12fe

File Section File VM
chip-lock.elf text 12 12
chip-lock.elf device_handles 4 4
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,1734
.debug_str,0,871
.debug_loc,0,193
.debug_ranges,0,80
.debug_line,0,50
text,12,12
device_handles,4,4

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

sections,vmsize,filesize


@github-actions
Copy link

Size increase report for "esp32-example-build" from 5ab12fe

File Section File VM
chip-all-clusters-app.elf .flash.text 8 8
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
.debug_info,0,1653
.debug_str,0,869
.debug_line,0,131
.debug_loc,0,43
.flash.text,8,8
.debug_ranges,0,-40

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

sections,vmsize,filesize


@github-actions
Copy link

Size increase report for "gn_qpg6100-example-build" from 5ab12fe

File Section File VM
chip-qpg6100-lighting-example.out .text 16 16
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,1734
.debug_str,0,871
.debug_loc,0,192
.debug_ranges,0,72
.debug_line,0,47
.text,16,16
[Unmapped],0,-16

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

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'


@bzbarsky-apple bzbarsky-apple merged commit 7e851e0 into project-chip:master Jun 26, 2021
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
* Retrieving OpCSR from the Chip Device after passing the CSRNonce.

* Retrieving OpCSR from the Chip Device after passing the CSRNonce.

Co-authored-by: Ritika Nanda <[email protected]>
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.

9 participants