Implement code signing for client ios SDK #974

ruslan-bikkinin · 2017-08-24T14:55:12Z

Basic support for bundle's signature verification for ios applications has been implemented.

By default, feature is disabled, so no need for clients not interested in code signing to worry about changing code-push sdk initialization code after update.

To use this feature, developers need to add PublicKey string row inInfo.plist with content of public key they want to use to verify bundles signature.

Update JWT library to 3.0.0-beta4 Implement signature verification mechanism

msftclas · 2017-08-24T14:55:17Z

@ruslan-bikkinin,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

max-mironov · 2017-08-25T07:56:47Z

ios/CodePush/CodePushConfig.m

    NSString *deploymentKey = [infoDictionary objectForKey:@"CodePushDeploymentKey"];
    NSString *serverURL = [infoDictionary objectForKey:@"CodePushServerURL"];
-
+    NSString *publicKey = [infoDictionary objectForKey:@"PublicKey"];


As the value is taken from plist file that can be shared between different libraries does it make sense to name it CodePushPublicKey to avoid confusion about the reference of this property? (the same as for deploymentKey/ServerUrl)

@max-mironov makes sense, I'll do it, tnx!

max-mironov · 2017-08-25T08:02:05Z

ios/CodePush/CodePushPackage.m

+                                   error:(NSError **)error
+{
+    CPLog(@"The update contents failed the data integrity check.");
+    if (!error || !*error) {


Not sure if !*error is needed here, I believe if (!error) should be enough.

~~Also don't understand why do we need at all err variable, seems that it is not used anywhere at all~~

Also don't understand why do we need at all err variable, seems that it is not used anywhere at all

this is a part of callback function signature

Not sure if !*error is needed here, I believe if (!error) should be enough.

Okay, i will refactor it to simplify conditions

max-mironov · 2017-08-25T08:26:10Z

ios/CodePush/CodePushPackage.m

-                                                                failCallback(error);
+                                                        if(isDiffUpdate){
+                                                            CPLog(@"Applying diff update.");
+                                                        }else{


Just style issue to use spaces between }else{ or probably use Ternary operator here

@max-mironov got it!

max-mironov · 2017-08-25T08:26:57Z

ios/CodePush/CodePushPackage.m

+                                                            BOOL isSignatureValid = [CodePushUpdateUtils verifySignatureFor:newUpdateFolderPath
+                                                                                      withPublicKey:publicKey
+                                                                                              error:&error];
+                                                            if(!isSignatureValid){


Same style issue, please use spaces (and please check all the places below)

@max-mironov got it!

max-mironov · 2017-08-25T08:36:32Z

ios/CodePush/CodePushUpdateUtils.m

 }

-@end
+ (NSString *)cleanPublicKey:(NSString *)publicKeyString


From my point of view Clean is not a good name here, probably preparePublicKeyForDecoding or something else could fit better

@max-mironov I'll fix it, thx!

max-mironov · 2017-08-25T08:38:39Z

ios/CodePush/CodePushUpdateUtils.m

+    return nil;
+}
+
+ (NSDictionary *) verifyJWT:(NSString *) signature


I can see that other verify methods here return BOOL value (that make sense!) but this one is not. Maybe parseJWT should better reflect the purpose of it.

@max-mironov I'll tried to make code on android and ios looks the same as possible, so that's why I call this function that way. If it is crucial, I can change it on both platforms, what do you think?

yep, looks like this can be modified for both SDK's

max-mironov · 2017-08-25T08:43:25Z

ios/CodePush/CodePushUpdateUtils.m

+    NSString *signature = [self getSignatureFor: folderPath
+                                          error: error];
+    if(signature == nil) {
+        if(error && *error){


Again please verify if we ever need this error && *error?

@max-mironov Okay, thx!

max-mironov · 2017-08-25T08:44:25Z

ios/CodePush/CodePushUpdateUtils.m

+        return false;
+    }
+
+    if(![envelopedPayload objectForKey:@"contentHash"]){


Probably could move @contentHash to constants

@max-mironov not so important for now, i think. May be later, if payload structure will become more complicated.

max-mironov · 2017-08-25T11:15:53Z

ios/CodePush/CodePushUpdateUtils.m

+    if ([[NSFileManager defaultManager] fileExistsAtPath:signatureFilePath]) {
+        return [NSString stringWithContentsOfFile:signatureFilePath encoding:NSUTF8StringEncoding error:error];
+    }else{
+        *error = [CodePushErrorUtils errorWithMessage:[NSString stringWithFormat: @"Cannot find signature at %@", signatureFilePath]];


Do we need to check for null here before assigning variable (e.g. if (error) {...})?

@max-mironov No, we don't, because we just send Error to method caller in order to it be able to process error. This is method's caller responsibility to provide initialized pointer for NSError*.

Well, that should be in most cases, but if someone will not do it we'll get an exception here. Although we do not expect that someone else should call it who knows how it would be changed in future? I'd prefer to stick to the arguments provided e.g. in this answer. But that's only my opinion, no pressure from my side :)

sergey-akhalkov · 2017-08-28T11:54:20Z

ios/CodePush/CodePush.h

                   expectedHash:(NSString *)expectedHash
                          error:(NSError **)error;

+ (NSString *)preparePublicKeyForDecoding:(NSString *)publicKeyString;


I believe we should try to find the better name for preparePublicKeyForDecoding method, probably something like this:
preparePublicKeyForDecoding -> GetKeyValueFromPublicKeyString // remove BEGIN / END tags and line breaks from public key string

@sergey-akhalkov thanks for suggestion, I'll do it!

sergey-akhalkov · 2017-08-28T12:01:27Z

ios/CodePush/CodePushPackage.m

+                                                                [self handleFailedDataIntegrityCheck:failCallback error:&error];
                                                                return;
+                                                            } else {
+                                                                CPLog(@"The update contents succueded the data integrity check.");


Typo here - succueded

@sergey-akhalkov, I'll fix it.

sergey-akhalkov · 2017-08-28T12:01:35Z

ios/CodePush/CodePushPackage.m

+                                                                    [self handleFailedDataIntegrityCheck:failCallback error:&error];
+                                                                    return;
+                                                                } else {
+                                                                    CPLog(@"The update contents succueded the data integrity check.");


Typo here - succueded

sergey-akhalkov · 2017-08-28T12:07:11Z

ios/CodePush/CodePushPackage.m

+                                                            CPLog(@"Applying diff update.");
+                                                        } else {
+                                                            CPLog(@"Applying full update.");
+                                                        }


I believe it would be more elegant to use ?: ternary operator here due to this block of code is used for logging purposes only:
CPLog((isDiffUpdate) ? @"Applying diff update." : @"Applying full update.")

@sergey-akhalkov thx, I'll do it.

sergey-akhalkov · 2017-08-28T12:24:09Z

ios/CodePush/CodePushPackage.m

+
+    failCallback(*error);
+    return;
+}


Should not we refactor it like this?

+ (void) handleFailedDataIntegrityCheck:(void (^)(NSError *err))failCallback error:(NSError **)error { NSString * const errorMessage = @"The update contents failed the data integrity check."; CPLog(errorMessage); if (!error) { *error = [CodePushErrorUtils errorWithMessage:errorMessage]; } failCallback(*error); return; }

Please also double check that you are operating with pointers and addresses of error parameter correctly while checking for nil and below.

sergey-akhalkov · 2017-08-28T12:27:40Z

ios/CodePush/CodePushUpdateUtils.m

+    NSError *signatureVerificationError;
+    NSString *signature = [self getSignatureFor: folderPath
+                                          error: &signatureVerificationError];
+    if(signatureVerificationError) {


Code style issue here and below - should be if ( instead of if(

@sergey-akhalkov I'll fix it

sergey-akhalkov · 2017-08-28T13:29:10Z

ios/CodePush/CodePushPackage.m

+                                                                                      withPublicKey:publicKey
+                                                                                              error:&error];
+                                                            if (!isSignatureValid) {
+                                                                [self handleFailedDataIntegrityCheck:failCallback error:&error];


I would prefer to avoid of using handleFailedDataIntegrityCheck method at all and implement error handling like this:

if (!error) { error = [CodePushErrorUtils errorWithMessage:@"Code Signing integrity check error..."]; } CPLog(error); failCallback(*error); return;

This will allow us to have different error messages for code signing integrity check and for general integrity check and determine in future where the issue occurs. Also it makes code cleaner due to we have no method where we pass error as parameter ones again.

@sergey-akhalkov nice proposition, I'll do it!

sergey-akhalkov · 2017-08-28T13:30:04Z

ios/CodePush/CodePushPackage.m

+                                                                                             expectedHash:newUpdateHash
+                                                                                                    error:&error]) {
+
+                                                                    [self handleFailedDataIntegrityCheck:failCallback error:&error];


The same as above is here:

if (!error) { error = [CodePushErrorUtils errorWithMessage:@"General integrity check error..."]; } CPLog(error); failCallback(*error); return;

Add additional checking for specific update situations Fix bugs Refactor method names

Fix log messages

ruslan-bikkinin added 4 commits August 15, 2017 18:56

Add JWT Library to ios project

98f9f32

Add new option PublicKey to Info.plist

ad9f8d4

Implement code signing mechanism

cc8f286

Update JWT library to 3.0.0-beta4 Implement signature verification mechanism

Undo redundant changes

146ecc1

ruslan-bikkinin added enhancement iOS labels Aug 24, 2017

ruslan-bikkinin requested review from max-mironov, patniko and sergey-akhalkov August 24, 2017 14:55

msftclas added the cla-already-signed label Aug 24, 2017

max-mironov reviewed Aug 25, 2017

View reviewed changes

max-mironov suggested changes Aug 25, 2017

View reviewed changes

max-mironov reviewed Aug 25, 2017

View reviewed changes

Minor improvements

9e5c8ca

ruslan-bikkinin force-pushed the code-signing-new-ios branch from c1ecd34 to 9e5c8ca Compare August 25, 2017 14:44

sergey-akhalkov reviewed Aug 28, 2017

View reviewed changes

ruslan-bikkinin added 2 commits August 29, 2017 15:28

Minor fixes

12f021b

Add additional checking for specific update situations Fix bugs Refactor method names

Add new dependencies to podspec

2bdaa43

ruslan-bikkinin force-pushed the code-signing-new-ios branch from 6121753 to 2bdaa43 Compare August 31, 2017 11:53

sergey-akhalkov approved these changes Sep 5, 2017

View reviewed changes

max-mironov approved these changes Sep 5, 2017

View reviewed changes

Minor improvements

b189119

Fix log messages

sergey-akhalkov merged commit 4ab0e5e into microsoft:master Sep 13, 2017

ruslan-bikkinin deleted the code-signing-new-ios branch September 19, 2017 14:08

nicholasess mentioned this pull request Dec 4, 2022

[Snyk] Security upgrade superagent from 1.8.5 to 2.0.0 nicholasess/react-native-code-push#29

Open

snyk-bot mentioned this pull request Jan 17, 2023

[Snyk] Security upgrade superagent from 1.8.5 to 2.0.0 nicholasess/react-native-code-push#34

Open

Implement code signing for client ios SDK #974

Implement code signing for client ios SDK #974

Uh oh!

Conversation

ruslan-bikkinin commented Aug 24, 2017

Uh oh!

msftclas commented Aug 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-mironov Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-mironov Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-mironov Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergey-akhalkov Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergey-akhalkov Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-mironov Aug 25, 2017 •

edited

Loading

max-mironov Aug 25, 2017 •

edited

Loading

max-mironov Aug 25, 2017 •

edited

Loading

sergey-akhalkov Aug 28, 2017 •

edited

Loading

sergey-akhalkov Aug 28, 2017 •

edited

Loading

sergey-akhalkov Aug 28, 2017 •

edited

Loading

sergey-akhalkov Aug 28, 2017 •

edited

Loading