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

Jump to bottom

Implement SDL-0207 RPC Message Protection #1320

Merged
merged 77 commits into from Oct 10, 2019
Merged

Implement SDL-0207 RPC Message Protection #1320

merged 77 commits into from Oct 10, 2019

Conversation

SatbirTanda
Copy link
Contributor

@SatbirTanda SatbirTanda commented Jun 25, 2019 •
edited

Implements #1163

This PR is ready for review.

Risk

This PR makes minor API changes.

Testing Plan

Add unit tests

Summary

Add a public payloadProtected property to RPC Messages. Also automatically check the requireEncryption flag of RPC being sent in the sendRPC method on SDLManager for automatically handling encryption if needed.

CLA

@SatbirTanda
Parse requireEncryption in OnPermissionsChange and PermissionItem
b9448e6 
Add result code ENCRYPTION_NEEDED
@SatbirTanda
Add Test for parsing requireEncryption
acce708 
Update SDLOnPermissionsChangeSpec & SDLPermissionItemSpec & SDLResultSpec
@SatbirTanda
Add requiresEncryption property to permission manager
9aa1d9a
@SatbirTanda
Setup SDLEncryptionConfiguration class
f25aa2f
@SatbirTanda
Add security manager to proxy
515e3a5
@SatbirTanda
Create new sendEncryptedRPC method in SDLManager.h
ee6805a 
Also handles auto-matically encrypting RPCs if sent in the sendRPC method where the permssionManager can check the requireEncryption flag
@SatbirTanda
Add error checking for encryption
dcef157 
Check permissions in O(1) time
@SatbirTanda
Update project.pbxproj
f0eba6d
@SatbirTanda
Update project.pbxproj
9d9f3c2 
Properly add new files
@SatbirTanda
Update project.pbxproj
71e53f2 
Make SDLEncryptionConfiguration.h public
@SatbirTanda
Fix warnings
f44fee4
@joeljfischer joeljfischer added proposal Accepted SDL Evolution Proposal enhancement labels Jun 26, 2019
@NicoleYarroch NicoleYarroch added this to the Future milestone Jun 26, 2019
@NicoleYarroch NicoleYarroch added this to In progress in v6.4 via automation Jun 26, 2019
@SatbirTanda SatbirTanda changed the base branch from master to develop July 2, 2019 22:40
@joeljfischer joeljfischer self-requested a review July 9, 2019 18:08
joeljfischer
joeljfischer suggested changes Jul 9, 2019
View reviewed changes
SmartDeviceLink/SDLAsynchronousEncryptedRPCRequestOperation.h Outdated Show resolved Hide resolved
SmartDeviceLink/SDLAsynchronousEncryptedRPCRequestOperation.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLEncryptionLifecycleManager.h Show resolved Hide resolved
SmartDeviceLink/SDLEncryptionManager.h Outdated Show resolved Hide resolved
SmartDeviceLink/SDLEncryptionLifecycleManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLProxy.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLProxy.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLProxy.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLProxy.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/DevAPISpecs/SDLConfigurationSpec.m Show resolved Hide resolved
v6.4 automation moved this from In progress to Review in progress Jul 9, 2019
@SatbirTanda
Remove duplicated code
861d397
@SatbirTanda
Do not add security manager twice to proxy
55d3b1e 
Deprecate initializers in SDLStreamingMediaConfig
Add security manager to SDLStreamingMediaConfig after SDLEncryptionConfig has been initialized.
@SatbirTanda
Remove SDLAsynchronousEncryptedRPCRequestOperation
0fb009f 
Add flag to SDLAsynchronousRPCRequestOperation
@joeljfischer joeljfischer changed the title Implement RPC message protection Implement SDL-0207 RPC Message Protection Jul 15, 2019
@theresalech
Copy link
Contributor

theresalech commented Jul 15, 2019 •
edited

@SatbirTanda can you please advise when this is ready for re-review? It looks like there are still items to be addressed.

@SatbirTanda
Copy link
Contributor Author

@theresalech ready for review

NicoleYarroch
NicoleYarroch suggested changes Sep 10, 2019
View reviewed changes
Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

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

Left some comments

SmartDeviceLink/SDLLifecycleManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLLifecycleManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLServiceEncryptionDelegate.h Show resolved Hide resolved
SmartDeviceLink/SDLServiceEncryptionDelegate.h Show resolved Hide resolved
SmartDeviceLink/SDLEncryptionLifecycleManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLEncryptionLifecycleManager.m Show resolved Hide resolved
@NicoleYarroch
Copy link
Contributor

@NicoleYarroch I disagree with If the encryption lifecycle manager fails to setup the encrypted service, the manager should retry the encrypted service one more time before failing. The Audio and Video manager do not attempt to retry starting a secure service, there is no need to. It would be making the same request twice, there is not really a timing issue here. In the case for the proxy transport connect it seems necessary because of the nature of iAP / TCP.

A new proposal will be submitted to handle this case for the next release

@jordynmackool
Copy link
Contributor

@SatbirTanda Please see the powerpoint attached from our alignment meeting on 2019-09-10.

Livio Feedback 2019-09-10; RPC Message Protection.pdf

@SatbirTanda
Update SDLLifecycleManager.m
c65cdf2 
Fix appId
@SatbirTanda
Update podspec and SmartDeviceLink.h
1d99902 
Expose SDLProtocolConstants.h and SDLServiceEncryptionDelegate.h
@SatbirTanda
Update SDLEncryptionLifecycleManager.m
43f1cd6 
Append sdl_ to private methods
@SatbirTanda
Update SDLEncryptionLifecycleManager.m
a3c684e 
Add delegate callback in endservice ACK and NACK methods
@SatbirTanda
Add public rpcRequiresEncryption to SDLPermissionManager
2943037 
Pass in SDLRPCMessage instead of functionID
@SatbirTanda
Copy link
Contributor Author

@theresalech ready for review

@NicoleYarroch NicoleYarroch mentioned this pull request Sep 11, 2019
Closed
NicoleYarroch
NicoleYarroch suggested changes Sep 12, 2019
View reviewed changes
Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

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

Testing went well. I found one issue that I elaborated on in the comments.

SmartDeviceLink/SDLPermissionManager.h Outdated Show resolved Hide resolved
SmartDeviceLink/SDLEncryptionLifecycleManager.m Show resolved Hide resolved
SmartDeviceLink/SDLEncryptionLifecycleManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLError.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLErrorConstants.h Outdated Show resolved Hide resolved
SmartDeviceLink/SDLError.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLPermissionManager.m Show resolved Hide resolved
SmartDeviceLink/SDLLifecycleManager.m Show resolved Hide resolved
@SatbirTanda
Copy link
Contributor Author

@theresalech ready for review

@SatbirTanda
Copy link
Contributor Author

@theresalech ready for review

NicoleYarroch
NicoleYarroch suggested changes Sep 16, 2019
View reviewed changes
Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

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

Request changes

SmartDeviceLink/SDLStreamingMediaConfiguration.h Outdated Show resolved Hide resolved
SmartDeviceLink/SDLStreamingMediaConfiguration.h Show resolved Hide resolved
SmartDeviceLink/SDLStreamingMediaConfiguration.h Outdated Show resolved Hide resolved
SmartDeviceLink/SDLEncryptionLifecycleManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLEncryptionLifecycleManager.m Show resolved Hide resolved
SmartDeviceLink/SDLPermissionManager.m Show resolved Hide resolved
@SatbirTanda
Copy link
Contributor Author

@theresalech ready for review

NicoleYarroch
NicoleYarroch suggested changes Sep 17, 2019
View reviewed changes
Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

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

Minor SDLPermissionsManagerSpec test case fixes needed.

@SatbirTanda
Copy link
Contributor Author

@theresalech ready for review

@joeljfischer joeljfischer merged commit af56e0a into smartdevicelink:develop Oct 10, 2019
v6.4 automation moved this from Review in progress to Done Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joeljfischer joeljfischer joeljfischer requested changes

@joeygrover joeygrover joeygrover left review comments

@NicoleYarroch NicoleYarroch NicoleYarroch approved these changes

Assignees

@SatbirTanda SatbirTanda

Labels
proposal Accepted SDL Evolution Proposal
Projects
No open projects
v6.4
  
Done
Milestone
Future
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@SatbirTanda @theresalech @joeljfischer @NicoleYarroch @jordynmackool @joeygrover @standa1-ford