[Accepted] SDL 0333 - Handle Scenario Where no Valid Cert is Available

theresalech commented 3 years ago

Hello SDL community,

The review of the revised proposal "SDL 0333 - Handle Scenario Where no Valid Cert is Available" begins now and runs through July 13, 2021. Previous reviews of this proposal took place May 12 - May 25, 2021 and June 2 - June 22, 2021. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0333-handle-scenario-where-no-valid-cert-is-available.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

https://github.com/smartdevicelink/sdl_evolution/issues/1140

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

Is the problem being addressed significant enough to warrant a change to SDL?
Does this proposal fit well with the feel and direction of SDL?
If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study? Please state explicitly whether you believe that the proposal should be accepted into SDL.

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you, Theresa Lech

Program Manager - Livio theresa@livio.io

joeljfischer commented 3 years ago

1. In the motivation, you state:

Currently, if an app requests a protected session while SDL Core does not have a valid certificate the request will be pending until Core either is able to retrieve a valid certificate through PTU or if the whole PTU retry sequence fails.

I think the last part of this sentence is missing.

2. A very minor clarification point. The next sentence in the motivation states:

In the default configuration, the PTU retry sequence can take up to 17 minutes, which means that if an app attempts to start a protected service early in Core's lifecycle it will take quite a while for the app to receive a response. This could cause a poor user experience.

I think that "it will take quite a while" should state "it may take quite a while".

Overall, I think this proposal is a good change.

joeygrover commented 3 years ago

3. This might be out of scope for this proposal specifically, but in the case that a PTU is updated after a NAK has been sent to the app, is there any mechanism to let the app know that it could try again with more success?

iCollin commented 3 years ago

1.

I think the last part of this sentence is missing.

Perhaps I should move Core after either, this was my intended communication:

Currently, if an app requests a protected session while SDL Core does not have a valid certificate the request will be pending until Core either a. is able to retrieve a valid certificate through PTU or b. if the whole PTU retry sequence fails.

2.

I think that "it will take quite a while" should state "it may take quite a while".

I agree.

3.

Currently there is no mechanism to let an app know when a PTU completes successfully. I could see a new notification such as OnCertificateUpdated being useful even without this proposal.

Jack-Byrne commented 3 years ago

I agree we could add this new type of notification in this proposal. The goal of this proposal is to improve the handshake process for encrypted services. @joeygrover's comment highlights another potential improvement for the current handshake process. Having a callback notification on a successful cert update seems appropriate.

joeljfischer commented 3 years ago

1. I'm sorry, I understood the last part of that sentence "if the whole PTU retry sequence fails" as incomplete and missing the result. I see what was intended now.

Jack-Byrne commented 3 years ago

Instead of creating a new notification lets just add a new parameter to OnPermissionsChanged. Open to suggestions for the description.

    <function name="OnPermissionsChange" functionID="OnPermissionsChangeID" messagetype="notification" since="2.0">
        <description>Provides update to app of which policy-table-enabled functions are available</description>
        <param name="permissionItem" type="PermissionItem" minsize="0" maxsize="500" array="true" mandatory="true">
            <description>Change in permissions for a given set of RPCs</description>
        </param>
        <param name="requireEncryption" type="Boolean" mandatory="false" since="6.0"/>
+      <param name="encryptionReady" type="Boolean" mandatory="false" since="x.x">
+        <description>If true, encryption is ready. If false, encryption is not ready. If omitted, mobile assumes encryption ready.</description>
+        </param>

    </function>

theresalech commented 3 years ago

The Steering Committee voted to keep this proposal in review to allow for the conversation to continue on the open item 3. It was noted that there is no action needed for item 1, and the agreed upon revision for item 2 is to change "it will take quite a while" to "it may take quite a while" in the Motivation section of the proposal.

theresalech commented 3 years ago

The Steering Committee voted to return this proposal for the following revisions: change "it will take quite a while" to "it may take quite a while" in the Motivation section, and add new encryptionReady parameter to OnPermissionsChange as outlined in this comment.

theresalech commented 3 years ago

The author has updated this proposal based on the Steering Committee's agreed upon revisions, and the revised proposal is now in review until June 8, 2021.

The specific items that were updated since the last review can be found in this merged pull request: #1148.

joeljfischer commented 3 years ago

Restarting numbering

1. The impacted platforms is no longer correct. All app libraries are now impacted based on the RPC change and associated behavior change to account for retrying encryption setup when the permission changes (probably a change to the SDLEncryptionLifecycleManager in the iOS context, for example.

iCollin commented 3 years ago

1. The impacted platforms should be updated to include RPC Spec, iOS, Java Suite, and JavaScript Suite.

Some language should be added to the section Changes to OnPermissionsChange explaining when encryptionReady will be sent. When an app tries to start a protected service and Core does not have a valid certificate Core will dispatch OnPermissionsChange with encryptionReady=false. When Core receives a valid certificate Core will dispatch OnPermissionsChange with encryptionReady=true.

Under the section Changes to OnPermissionsChange an additional section should be added with the following changes to app libraries.

This chart describes what new action the app libraries should take upon receiving an OnPermissionChange based on the value of parameter encryptionReady.

encryptionReady Value	App Action
true	Retry unsuccessful StartService encrypted requests (see below)
false	no action
null	no action

In order to retry any unsuccessful StartService encrypted requests, app libraries should implement the following changes:

a. The EncryptionLifecycleManager should be modified to watch for the encryptionReady parameter of the OnPermissionsChange notification. Whenever OnPermissionsChange is received with encryptionReady=true the manager will check for an RPC StartService encrypted request that was not ACK'd with encryption enabled. If any such service is found, the manager will try to enable encryption on it by sending another StartService with encryption enabled. b. The StreamingMediaManager should be modified to watch for the OnPermissionsChange notification. Whenever OnPermissionsChange is received with encryptionReady=true the manager will ask its relevant sub-managers to check for StartService encrypted requests that were not ACK'd with encryption enabled. If any such streams are found, the sub-managers will try to enable encryption on them by sending another StartService with encryption enabled.

theresalech commented 3 years ago

During the 2021-06-08 Steering Committee meeting, the Steering Committee voted to keep this proposal in review to allow the discussion to continue on the review issue.

theresalech commented 3 years ago

The Steering Committee voted to keep this proposal in review to allow for further discussion on the review issue.

bilal-alsharifi commented 3 years ago

The StreamingMediaManager should be modified to watch for the OnPermissionsChange notification. Whenever OnPermissionsChange is received with encryptionReady=true the manager will ask its relevant sub-managers to check for StartService encrypted requests that were not ACK'd with encryption enabled. If any such streams are found, the sub-managers will try to enable encryption on them by sending another StartService with encryption enabled.

One little thing. The media manager where the app should retry starting failed services in iOS is called StreamingMediaManager However in Java Suite, it is called VideoStreamManager.

Other than that, the suggested changes look good.

theresalech commented 3 years ago

The Steering Committee voted to return this proposal for the revisions described by the author in this comment. Additionally, it should be specified within the revisions that while the StreamingMediaManager will retry starting failed services in iOS, in Java Suite, this will be done by both the VideoStreamManager and AudioStreamManager.

theresalech commented 3 years ago

The author has updated this proposal based on the Steering Committee's agreed upon revisions, and the revised proposal is now in review until July 13, 2021.

The specific items that were updated since the last review can be found in this merged pull request: #1159.

theresalech commented 3 years ago

The Steering Committee fully agreed to accept this proposal.

theresalech commented 3 years ago

Implementation Issues Entered:

smartdevicelink / sdl_evolution

[Accepted] SDL 0333 - Handle Scenario Where no Valid Cert is Available #1140