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

Security manager set for secondary protocol #1482

Merged
merged 21 commits into from Jan 7, 2020
Merged

Security manager set for secondary protocol #1482

merged 21 commits into from Jan 7, 2020

Conversation

justingluck93
Copy link
Contributor

@justingluck93 justingluck93 commented Dec 2, 2019 •
edited

Fixes #1476

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

Tests secondary transport to see if video encryption works

Summary

Set secondary transports security manager so RPC encryption will work.

Tasks Remaining:

  • Have Ford test with this branch

CLA

@justingluck93 justingluck93 self-assigned this Dec 2, 2019
@justingluck93 justingluck93 added the bug A defect in the library label Dec 2, 2019
@joeljfischer
Copy link
Contributor

@kshala-ford Can you please confirm that this PR fixes your issue?

joeljfischer
joeljfischer suggested changes Dec 2, 2019
View reviewed changes
SmartDeviceLink/SDLSecondaryTransportManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLSecondaryTransportManager.m Outdated Show resolved Hide resolved
@mrapitis
Copy link
Contributor

mrapitis commented Dec 3, 2019

@joeljfischer we will test on the bench and provide feedback. Thanks!

@mrapitis
Copy link
Contributor

mrapitis commented Dec 5, 2019

@justingluck93 we have performed some testing on the bench and unfortunately the problem has not been completely solved. The primary use case, when the app is placed into a streamable state for the first time still reports an error regarding the security manager not being set. However for secondary use cases, when the app is placed into a streamable state, followed by a not streamable, followed by a streamable state are working now. We will continue to investigate on our side, but it does seem like we are still missing a step in the flow.

@justingluck93
Copy link
Contributor Author

@mrapitis @kshala-ford Can you please try to test again with this PR!

@mrapitis
Copy link
Contributor

mrapitis commented Dec 6, 2019

@justingluck93 no resolution with the additional changes.

Upon further investigation it looks like a race condition is occurring between handleRegisterAppInterfaceResponse linked below and the SecondaryTransportManager. There is no synchronization between these two events which is causing an issue. Sometimes the RAI response will be received before the setup of the SecondaryTransportManager, othertimes the SecondaryTransport will be setup first -- however the security manager is not available until after the RegisterAppInterfaceResponse is received. It seems like the SecondaryTransportManager needs to be able to query SDLProxy if the SecurityManager is available and if so, set its value internally. Also, if the SecurityManager is updated in the handleRegisterAppInterfaceResponse, it should notify the SecondaryTransportManager to update its value for the SecurityManager.

https://github.com/smartdevicelink/sdl_ios/blob/master/SmartDeviceLink/SDLProxy.m#L534

adding a notification that the security manager has been set and to p…
1cfa262 
…roperly update the secondary protocol to use that security manager
joeljfischer
joeljfischer suggested changes Dec 9, 2019
View reviewed changes
SmartDeviceLink/SDLNotificationConstants.h Outdated Show resolved Hide resolved
SmartDeviceLink/SDLNotificationConstants.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLSecondaryTransportManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLSecondaryTransportManager.m Outdated Show resolved Hide resolved
joeljfischer
joeljfischer suggested changes Dec 9, 2019
View reviewed changes
SmartDeviceLink/SDLProxy.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLSecondaryTransportManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLSecondaryTransportManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLSecondaryTransportManager.m Show resolved Hide resolved
SmartDeviceLinkTests/ProxySpecs/SDLSecondaryTransportManagerSpec.m Outdated Show resolved Hide resolved
joeljfischer
joeljfischer suggested changes Dec 10, 2019
View reviewed changes
SmartDeviceLink/SDLProxy.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLSecondaryTransportManager.m Show resolved Hide resolved
SmartDeviceLinkTests/ProxySpecs/SDLSecondaryTransportManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/ProxySpecs/SDLSecondaryTransportManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/ProxySpecs/SDLSecondaryTransportManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/ProxySpecs/SDLSecondaryTransportManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/ProxySpecs/SDLSecondaryTransportManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/ProxySpecs/SDLSecondaryTransportManagerSpec.m Outdated Show resolved Hide resolved
Justin Gluck and others added 2 commits December 11, 2019 12:13
joeljfischer
joeljfischer suggested changes Dec 11, 2019
View reviewed changes
SmartDeviceLink/SDLProxy.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/ProxySpecs/SDLSecondaryTransportManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/ProxySpecs/SDLSecondaryTransportManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/ProxySpecs/SDLSecondaryTransportManagerSpec.m Show resolved Hide resolved
SmartDeviceLinkTests/ProxySpecs/SDLSecondaryTransportManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/ProxySpecs/SDLSecondaryTransportManagerSpec.m Show resolved Hide resolved
SmartDeviceLinkTests/ProxySpecs/SDLSecondaryTransportManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/ProxySpecs/SDLSecondaryTransportManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/ProxySpecs/SDLSecondaryTransportManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/ProxySpecs/SDLSecondaryTransportManagerSpec.m Show resolved Hide resolved
adding check for is appReady so we don't configure until we are, also…
2683202 
… made sure to add check in the iAP caee as well. Updated unit tests
Justin Gluck added 3 commits December 11, 2019 14:42
Merge branch 'bugfix/issue-1476-SecurityManager-Set-For-SecondaryProt…
20e48cb 
…ocol' of https://github.com/smartdevicelink/sdl_ios into bugfix/issue-1476-SecurityManager-Set-For-SecondaryProtocol

* 'bugfix/issue-1476-SecurityManager-Set-For-SecondaryProtocol' of https://github.com/smartdevicelink/sdl_ios:
  Update SmartDeviceLink/SDLProxy.m
reverting file
725b253
no message
41288d3
@joeljfischer
Copy link
Contributor

@mrapitis Could you test again with this branch?

@mrapitis
Copy link
Contributor

@joeljfischer, @justingluck93 initial test results are looking good with the updated PR. We will continue to test and provide feedback.

@theresalech
Copy link
Contributor

Hi @mrapitis, do you have any update on your additional testing? If possible, can you please let us know when you anticipate testing being completed? We are working to determine when we can include this fix in a library release. Thanks!

@joeljfischer joeljfischer added this to In progress in v6.4.1 via automation Jan 3, 2020
@joeljfischer joeljfischer moved this from In progress to Review in progress in v6.4.1 Jan 3, 2020
@joeljfischer joeljfischer removed this from Review in progress in v6.4.1 Jan 3, 2020
@joeljfischer joeljfischer added this to Review in progress in v6.5.0 Jan 3, 2020
@joeljfischer
Copy link
Contributor

@mrapitis Hi Markos, could you please provide us an update on this fix?

@mrapitis
Copy link
Contributor

mrapitis commented Jan 7, 2020

@joeljfischer Hi Joel, the changes look to be working well. Can we plan to include this fix in an upcoming release?

@joeljfischer
Copy link
Contributor

@mrapitis Thanks for the update. I will have a discussion with our PMs about including this in an upcoming release.

@joeljfischer joeljfischer merged commit 84b6518 into develop Jan 7, 2020
v6.5.0 automation moved this from Review in progress to Done Jan 7, 2020
@joeljfischer joeljfischer deleted the bugfix/issue-1476-SecurityManager-Set-For-SecondaryProtocol branch January 7, 2020 19:46
@joeljfischer joeljfischer mentioned this pull request Jan 7, 2020
Merged
5 tasks
@NicoleYarroch NicoleYarroch mentioned this pull request May 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joeljfischer joeljfischer joeljfischer requested changes

Assignees

@justingluck93 justingluck93

Labels
bug A defect in the library
Projects
No open projects
v6.5.0
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@justingluck93 @joeljfischer @mrapitis @theresalech