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.

Sign up for GitHub

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

Add new API so it's possible to not leak a key #36299

Merged
merged 16 commits into from
Nov 1, 2024
+170 −41
Merged

Add new API so it's possible to not leak a key #36299

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
ac6953c
Initial checkin
woody-apple Oct 30, 2024
4789980
Fixing review feedback
woody-apple Oct 30, 2024
e3735b2
Adding braces
woody-apple Oct 30, 2024
680ae77
Restyled by clang-format
restyled-commits Oct 30, 2024
f75ad53
Fixing build
woody-apple Oct 30, 2024
c3c885c
Restyled by clang-format
restyled-commits Oct 30, 2024
f99b809
Fixing annotations
woody-apple Oct 30, 2024
3c5c598
Update src/darwin/Framework/CHIP/MTRKeypair.h
woody-apple Oct 30, 2024
6d6b952
separate statements involving _mPublicKey to make compiler happy
kiel-apple Oct 31, 2024
bb09a76
use `CFAutorelease` on CoreFoundation typed public key copies
kiel-apple Oct 31, 2024
a3ba7d2
fix indent
kiel-apple Oct 31, 2024
32e8525
implement `copyPublicKey` for `MTRTestKeys`; add TODO note about opti…
kiel-apple Oct 31, 2024
9c1d359
remove comment
kiel-apple Nov 1, 2024
7707afc
consistent formatting for `copyPublicKey` calls
kiel-apple Nov 1, 2024
c70e031
reformat `copyPublicKey` in `MTRTestKeys`
kiel-apple Nov 1, 2024
d5bfe8d
Update src/darwin/Framework/CHIP/MTRKeypair.h
woody-apple Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/darwin-framework-tool/commands/common/CHIPToolKeypair.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
@interface CHIPToolKeypair : NSObject <MTRKeypair>
- (BOOL)initialize;
- (NSData *)signMessageECDSA_RAW:(NSData *)message;
- (SecKeyRef)publicKey;
- (SecKeyRef)copyPublicKey;
kiel-apple marked this conversation as resolved.
Show resolved Hide resolved
- (CHIP_ERROR)Serialize:(chip::Crypto::P256SerializedKeypair &)output;
- (CHIP_ERROR)Deserialize:(chip::Crypto::P256SerializedKeypair &)input;
- (CHIP_ERROR)createOrLoadKeys:(id)storage;
Expand Down
9 changes: 7 additions & 2 deletions examples/darwin-framework-tool/commands/common/CHIPToolKeypair.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ - (NSData *)signMessageECDSA_RAW:(NSData *)message
return out_signature;
}

- (SecKeyRef)publicKey
- (SecKeyRef)copyPublicKey
kiel-apple marked this conversation as resolved.
Show resolved Hide resolved
{
if (_mPublicKey == nil) {
chip::Crypto::P256PublicKey publicKey = _mKeyPair.Pubkey();
Expand All @@ -79,7 +79,12 @@ - (SecKeyRef)publicKey
};
_mPublicKey = SecKeyCreateWithData((__bridge CFDataRef) publicKeyNSData, (__bridge CFDictionaryRef) attributes, nullptr);
}
return _mPublicKey;

if (_mPublicKey) {
return CFRetain(_mPublicKey);
kiel-apple marked this conversation as resolved.
Show resolved Hide resolved
}

return NULL;
}

- (CHIP_ERROR)Deserialize:(chip::Crypto::P256SerializedKeypair &)input
Expand Down
19 changes: 18 additions & 1 deletion src/darwin/Framework/CHIP/MTRCertificates.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,24 @@ + (MTRCertificateDERBytes _Nullable)createOperationalCertificate:(id<MTRKeypair>
+ (BOOL)keypair:(id<MTRKeypair>)keypair matchesCertificate:(NSData *)certificate
{
P256PublicKey keypairPubKey;
CHIP_ERROR err = MTRP256KeypairBridge::MatterPubKeyFromSecKeyRef(keypair.publicKey, &keypairPubKey);
SecKeyRef publicKey = NULL;

if ([keypair respondsToSelector:@selector(copyPublicKey)]) {
kiel-apple marked this conversation as resolved.
Show resolved Hide resolved
publicKey = [keypair copyPublicKey];
kiel-apple marked this conversation as resolved.
Show resolved Hide resolved
} else {
publicKey = [keypair publicKey];
if (publicKey) {
CFRetain(publicKey);
}
}

CHIP_ERROR err = MTRP256KeypairBridge::MatterPubKeyFromSecKeyRef(publicKey, &keypairPubKey);

if (publicKey != NULL) {
CFRelease(publicKey);
publicKey = NULL;
}

if (err != CHIP_NO_ERROR) {
MTR_LOG_ERROR("Can't extract public key from keypair: %s", ErrorStr(err));
return NO;
Expand Down
19 changes: 18 additions & 1 deletion src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -819,7 +819,24 @@ - (BOOL)findMatchingFabric:(FabricTable &)fabricTable
} else {
// No root certificate means the nocSigner is using the root keys, because
// consumers must provide a root certificate whenever an ICA is used.
CHIP_ERROR err = MTRP256KeypairBridge::MatterPubKeyFromSecKeyRef(params.nocSigner.publicKey, &pubKey);
SecKeyRef publicKey = NULL;

if ([params.nocSigner respondsToSelector:@selector(copyPublicKey)]) {
kiel-apple marked this conversation as resolved.
Show resolved Hide resolved
publicKey = [params.nocSigner copyPublicKey];
kiel-apple marked this conversation as resolved.
Show resolved Hide resolved
} else {
publicKey = [params.nocSigner publicKey];
if (publicKey) {
CFRetain(publicKey);
}
}

CHIP_ERROR err = MTRP256KeypairBridge::MatterPubKeyFromSecKeyRef(publicKey, &pubKey);

if (publicKey != NULL) {
CFRelease(publicKey);
publicKey = NULL;
}

if (err != CHIP_NO_ERROR) {
MTR_LOG_ERROR("Can't extract public key from MTRKeypair: %s", ErrorStr(err));
return NO;
Expand Down
10 changes: 8 additions & 2 deletions src/darwin/Framework/CHIP/MTRKeypair.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,15 @@ NS_ASSUME_NONNULL_BEGIN
@protocol MTRKeypair <NSObject>
@required
/**
* @brief Return public key for the keypair.
* @brief Return a copy of the public key for the keypair.
*/
- (SecKeyRef)publicKey;
@property (nonatomic, readonly, copy) SecKeyRef copyPublicKey MTR_NEWLY_AVAILABLE;
woody-apple marked this conversation as resolved.
Show resolved Hide resolved

/**
* @brief Return public key for the keypair. DEPRECATED
*/

@property (nonatomic, readonly, copy) SecKeyRef publicKey MTR_DEPRECATED("Please implement copyPublicKey", ios(16.4, 17.2), macos(13.3, 14.2), watchos(9.4, 10.2), tvos(16.4, 17.2));
kiel-apple marked this conversation as resolved.
Show resolved Hide resolved

@optional
/**
Expand Down
32 changes: 16 additions & 16 deletions src/darwin/Framework/CHIPTests/MTRCertificateTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ - (void)testGenerateIntermediateCert

__auto_type * intermediateCert = [MTRCertificates createIntermediateCertificate:rootKeys
rootCertificate:rootCert
intermediatePublicKey:intermediateKeys.publicKey
intermediatePublicKey:[intermediateKeys.copyPublicKey autorelease]
kiel-apple marked this conversation as resolved.
Show resolved Hide resolved
issuerID:nil
fabricID:nil
error:nil];
Expand Down Expand Up @@ -161,7 +161,7 @@ - (void)testGenerateIntermediateCertWithValidityPeriod

__auto_type * intermediateCert = [MTRCertificates createIntermediateCertificate:rootKeys
rootCertificate:rootCert
intermediatePublicKey:intermediateKeys.publicKey
intermediatePublicKey:[intermediateKeys.copyPublicKey autorelease]
issuerID:nil
fabricID:nil
validityPeriod:validityPeriod
Expand Down Expand Up @@ -198,7 +198,7 @@ - (void)testGenerateIntermediateCertWithInfiniteValidity

__auto_type * intermediateCert = [MTRCertificates createIntermediateCertificate:rootKeys
rootCertificate:rootCert
intermediatePublicKey:intermediateKeys.publicKey
intermediatePublicKey:[intermediateKeys.copyPublicKey autorelease]
issuerID:nil
fabricID:nil
validityPeriod:validityPeriod
Expand Down Expand Up @@ -238,7 +238,7 @@ - (void)testGenerateOperationalCertNoIntermediate

__auto_type * operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:[operationalKeys.copyPublicKey autorelease]
fabricID:@1
nodeID:@1
caseAuthenticatedTags:cats
Expand Down Expand Up @@ -277,7 +277,7 @@ - (void)testGenerateOperationalCertNoIntermediateWithValidityPeriod

__auto_type * operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:[operationalKeys.copyPublicKey autorelease]
fabricID:@1
nodeID:@1
caseAuthenticatedTags:cats
Expand Down Expand Up @@ -321,7 +321,7 @@ - (void)testGenerateOperationalCertNoIntermediateWithInfiniteValidity

__auto_type * operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:[operationalKeys.copyPublicKey autorelease]
fabricID:@1
nodeID:@1
caseAuthenticatedTags:cats
Expand Down Expand Up @@ -356,7 +356,7 @@ - (void)testGenerateOperationalCertWithIntermediate

__auto_type * intermediateCert = [MTRCertificates createIntermediateCertificate:rootKeys
rootCertificate:rootCert
intermediatePublicKey:intermediateKeys.publicKey
intermediatePublicKey:[intermediateKeys.copyPublicKey autorelease]
issuerID:nil
fabricID:nil
error:nil];
Expand All @@ -367,7 +367,7 @@ - (void)testGenerateOperationalCertWithIntermediate

__auto_type * operationalCert = [MTRCertificates createOperationalCertificate:intermediateKeys
signingCertificate:intermediateCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:[operationalKeys.copyPublicKey autorelease]
fabricID:@1
nodeID:@1
caseAuthenticatedTags:nil
Expand Down Expand Up @@ -415,7 +415,7 @@ - (void)testGenerateOperationalCertErrorCases
// Check basic case works
__auto_type * operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:[operationalKeys.copyPublicKey autorelease]
fabricID:@1
nodeID:@1
caseAuthenticatedTags:nil
Expand All @@ -425,7 +425,7 @@ - (void)testGenerateOperationalCertErrorCases
// CATs too long
operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:[operationalKeys.copyPublicKey autorelease]
fabricID:@1
nodeID:@1
caseAuthenticatedTags:longCats
Expand All @@ -435,7 +435,7 @@ - (void)testGenerateOperationalCertErrorCases
// Multiple CATs with the same identifier but different versions
operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:[operationalKeys.copyPublicKey autorelease]
fabricID:@1
nodeID:@1
caseAuthenticatedTags:catsWithSameIdentifier
Expand All @@ -445,7 +445,7 @@ - (void)testGenerateOperationalCertErrorCases
// CAT with invalid version
operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:[operationalKeys.copyPublicKey autorelease]
fabricID:@1
nodeID:@1
caseAuthenticatedTags:catsWithInvalidVersion
Expand All @@ -455,7 +455,7 @@ - (void)testGenerateOperationalCertErrorCases
// Signing key mismatch
operationalCert = [MTRCertificates createOperationalCertificate:operationalKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:[operationalKeys.copyPublicKey autorelease]
fabricID:@1
nodeID:@1
caseAuthenticatedTags:nil
Expand All @@ -465,7 +465,7 @@ - (void)testGenerateOperationalCertErrorCases
// Invalid fabric id
operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:[operationalKeys.copyPublicKey autorelease]
fabricID:@0
nodeID:@1
caseAuthenticatedTags:nil
Expand All @@ -475,7 +475,7 @@ - (void)testGenerateOperationalCertErrorCases
// Undefined node id
operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:[operationalKeys.copyPublicKey autorelease]
fabricID:@1
nodeID:@0
caseAuthenticatedTags:nil
Expand All @@ -485,7 +485,7 @@ - (void)testGenerateOperationalCertErrorCases
// Non-operational node id
operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:[operationalKeys.copyPublicKey autorelease]
fabricID:@1
nodeID:@(0xFFFFFFFFFFFFFFFFLLU)
caseAuthenticatedTags:nil
Expand Down
2 changes: 1 addition & 1 deletion src/darwin/Framework/CHIPTests/MTRCertificateValidityTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ - (void)initStack:(MTRTestCertificateIssuer *)certificateIssuer

__auto_type * controllerOperationalCert =
[certificateIssuer issueOperationalCertificateForNode:@(kControllerId)
operationalPublicKey:controllerOperationalKeys.publicKey];
operationalPublicKey:[controllerOperationalKeys.copyPublicKey autorelease]];
XCTAssertNotNil(controllerOperationalCert);

__auto_type * params = [[MTRDeviceControllerStartupParams alloc] initWithIPK:certificateIssuer.rootKey.ipk
Expand Down
2 changes: 1 addition & 1 deletion src/darwin/Framework/CHIPTests/MTRControllerAdvertisingTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ - (nullable MTRDeviceController *)startControllerWithRootKeys:(MTRTestKeys *)roo

__auto_type * operational = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:root
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:[operationalKeys.copyPublicKey autorelease]
fabricID:fabricID
nodeID:nodeID
caseAuthenticatedTags:nil
Expand Down
Loading
Loading