[Accepted with Revisions] SDL 0317 - SDL Protocol Security Specification

[theresalech]

Hello SDL community,

The review of the revised proposal "SDL 0317 - SDL Protocol Security Specification" begins now and runs through June 1, 2021. Previous reviews of this proposal took place August 19 - September 1, 2020, and October 21 - November 17, 2020. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0317-sdl-protocol-security-specification.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/1070

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]

1. Change 1 seems a bit unclear to me. What is meant by "single frame" in the context of frame info? The frame type is already "single frame", why is the frame info "single frame"? Would something like "Security Query" be better, or could you add some additional clarification?

2. This section doesn't seem to be correct to me:

The handshake is designed as a client server communication which is configurable in the system settings. An application can take the role of a server where the system is the client or vice versa. This setting is not dynamic which means an SDL integrator must agree on one setup and avoid Client/Client or Server/Server connections. The client entity will initiate a TLS handshake with the corresponding security manager of the server. The client will do this only if the server was not authenticated before in the current transport connection.

It appears that the iOS library always assumes that it is the server. It sends the initial startService message with encryption enabled, and the code for TLS handling seems to assume it's the server and the HU is the client.

3. What service is the "Error Handling" query supposed to be sent on? Control? This should be explicitly specified.

4. 4.7.1's title is "Security Query" which doesn't seem very self-explanatory. Perhaps "Security Query Binary Header Definition"? I feel like it could possibly be combined into a different sub-section.

5. Can you clarify 4.7.5.1 and 4.7.5.2? 4.5.7.1 states that the binary data has a single byte error code, but 4.5.7.2 seems to say that the error code is in the JSON data.

6. This proposal doesn't make it explicit, but is "implementation" of this proposal merely adding it to the protocol_spec, or does it also include aligning all implementations to implement error handling if it's not, etc.? The latter seems to be the case based on the "Impact to Existing Code" section.

7. In "Impact to Existing Code" #3, you note that there's an existing known SDL Core issue around receiving an error code. Is this an existing Github issue, and if so, could it be linked here?

[kshala-ford]

@joeljfischer

1. Sorry I think a proper diff could have been better but it would be almost impossible to understand the context. Because Frame Info is specific per frame type the spec needs to change to meet existing Core behavior. The section proposes to change

Field	Size	Description
Frame Info	8 bit	... Frame Type = 0x01 (Single Frame) 0x00 - 0xFF Reserved ...

to

Field	Size	Description
Frame Info	8 bit	... Frame Type = 0x01 (Single Frame) 0x00 Single Frame 0x01 - 0xFF Reserved ...

2\. Please note: > The baseline for this section is **reverse engineered from SDL Core and the currently implemented behavior of the security manager** but also existing documentation from SDL Core and the app libraries Core supports both modes from the configuration. I took a look at the other repos. The iOS library and the security library lacks of specific server support. The Java Suite is most likely in the same situation. The mobile security manager should provide information to the SDL libraries which mode is chosen by the OEM. With regards to security, Core has the best implementation across all SDL repositories. This proposal is filling the gap of what is possible with Core. This proposal does not include detailed changes required in each repository. This would increase the proposal significantly. The goal is to review the other repos and identify gaps in the specification once this proposal is accepted. I think it's a good idea to take a look at the other repos now to have a better understanding of the possible effort. 3\. Any security related SDL protocol frame is based on this header https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0317-sdl-protocol-security-specification.md#4731-sdl-protocol-frame-header. They all are single frames on the control service. 4\. I followed the structure and format of the current protocol spec. See https://github.com/smartdevicelink/protocol_spec/blob/master/README.md#2-frames I would agree renaming to `Security Query Header` 5\. The proposal states > If an error occurs during the TLS handshake a notification is sent with an error code and error text as JSON data. Additionally the error code is added as a single byte binary data. The error code appears twice in the error frame body. In the JSON data and attached as a single byte. This is the way how Core works. 6\. The proposal explicitly states > The solution is to provide a new section into the protocol spec around security and protection. The result of adding the proposed additions are described in the impact of existing code as new spec have impact to existing implementations. See my comment to 2. 7\. To be specific a GitHub issue for the described problem has no backing by requirements. This is why the specification update is so important.

theresalech commented 4 years ago

The Steering Committee voted to keep this proposal in review until our next meeting, to allow for additional time for discussion on the review issue.

joeljfischer commented 4 years ago

1. Understood. Why was the name Single Frame chosen, and can we choose something else specific to security?

2. This and 6 and clearly related. I'm still having trouble understanding what you believe that "fully implemented" for this proposal is. In other words, for this proposal to be marked "implemented", is all that needs to happen an update to the protocol spec (and then bugs would be opened on the other platforms and fixed as possible), or do all platforms need to be aligned on the implementation of the new protocol spec. The latter seems to be implied by your statements here and in 6 (and in the "impacted platforms" section), but whichever you believe, I think that it should be stated in clear and unequivocal terms. The former requires only a days worth of effort, while the latter would require weeks for many platforms.

3. 👍

4. Why not "Security Query Binary Header"; since there are multiple different headers (protocol header, binary header) it seems like we should specify which header is being described. I'm not sure that the link you added helps your case. It starts by talking about the protocol header, not the binary header. It seems like 4.7.1 should be a sub-point of 4.7.3, and it's strange that 4.7.2 and 4.7.4 are divided by a different type of header. I think that the flow of information can be improved here.

Basically my suggestion is, move 4.7.2 to be 4.7.1, move 4.7.4 to be 4.7.2, move 4.7.1 to be 4.7.3.1.

5. Ah, okay. I thought that was an oversight. Maybe we can put a comment there saying "The JSON data and one-byte binary data contain the same data"?

6. See 2.

7. I don't see a requirement in this document that the client (I assume that's what's being referenced here rather than Core) is supposed to send a NAK to an error message. All I see is 4.7.5.4, which notes that the connection to be reset. (Note: That section also doesn't show how that reset should occur, should one side or the other send an EndService?).

8. I think that 4.7.2 (Start Service) should describe that the RPC service needs to be started as unencrypted, then moved to encrypted by sending another Start Service at a later point, and that other services can do the same thing to move from unencrypted to encrypted. I apologize if this is described and I missed it.

kshala-ford commented 4 years ago

1. I took a closer look at sdl_core code again and I think I need to remove this change from the proposal and leave frame info reserved. This is the difficulty of reverse engineering. You fumble around in the dark not knowing why things are done in a certain way. At least sdl_ios sets frame info to Single Frame serverMessageHeader.frameData = SDLFrameInfoSingleFrame; and after further investigation I saw that sdl_ios trying to keep frame info zero for single frames independently of the service type though it's not specified.

So my suggestion is to textually describe that frame info is reserved but the communication partners should set the field to 0 (instead of an undefined value).

2. The latter is the case. I don't know why you believe the former would be my desired approach. You may have skipped the impacted platforms of this proposal. With this list, the proposal acceptance will result in GitHub issues for each of this proposal. The proposal is therefore not "fully implemented" as long as not all issues are addressed.

I can help you out by adding following statement:

The solution is to provide a new section into the protocol spec around security and protection. The baseline for this section is reverse engineered from SDL Core and the currently implemented behavior of the security manager but also existing documentation from SDL Core and the app libraries. The acceptance of this section will require a code review in other platforms and aligning existing code with the specification. None of the specifications should touch any public API in other platforms however if the changes on a single platform are severe/major a proposal revision or a subsequent proposal describing the change should be performed for SDLC to review and accept.

4. I think you're requesting multiple changes in this single item. I am okay changing occurrences of Query Header or Binary Query Header to Security Query Binary Header as I found I mixed this naming up. This name aligns with the title Security Query of 4.7.1

4.b. I am strongly against moving sub-sections around!! In alignment of the existing protocol spec, this security sections follows the same red line.

a. Describe the header and size (aligns with section 2 of the protocol_spec). b. Describe the header fields and possible values (aligns with section 2.3 of the protocol_spec). c. Describe how the header is being used (aligns with section 4 of the protocol_spec)

the red line may not be as per your personal taste but it follows a purpose of. You can't start specifying things without providing all information to the specification.

5. Make them one sentence? Sure.

7. See

If the authentication fails for some reason the system will reset the TLS connection and return a StartService NAK frame.

But for more visibility this should definitely be describe in error handling. Sorry for that.

8. This sequence is not described in this proposal and it doesn't seem part of the existing protocol spec. I would like to include @yang1070 here who can help providing a description for RPC message protection.

Jack-Byrne commented 4 years ago

The proposed solution is confusing because it is creating a new documentation section that is mixed with details that are reversed engineered with details that should be considered new features. There is little indication in the proposed solution on what is reversed engineered and what is to be added/changed.

Livio submitted a pr to add missing documentation for security query to the protocol spec on June 9th here: https://github.com/smartdevicelink/protocol_spec/pull/30/files. This contains the information we found was missing from the protocol spec and needed to be documented.

In our opinion adding missing documentation is a bug and does not require a proposal. We could have had this discussion/review on the protocol spec pr we created. If you disagree with that then we would still suggest separating this proposal into two sections. First section would document the existing behavior. Second section includes the additions and changes this proposal wants to make to existing behavior.

joeljfischer commented 4 years ago

1. 👍

2\6. Thank you for clarifying. Given that that is the case, I believe there may be an issue that I see with this proposal. The proposal is implying that the app libraries need to support features like being the TLS client. However, I don't believe that that is a supportable case, because the app library has to be the one to send the StartService requests, and therefore must be the TLS server. So, even though Core currently supports being client or server, I think that the reverse engineering of the mobile platforms should reduce the scope of required implementation changes and we should make that clear in the proposal.

4.b. I'm afraid I'm not following you. Based on your "red line", your proposal seems to be out of alignment with the protocol_spec. Section 2 is talking about the protocol header, which you don't talk about until 4.7.3.1, while you're talking about the binary header in 4.7.1. Section 2.3 is still talking about the protocol header, so I don't see how that follows either. Then Section 4, where you talk about "how it's used" doesn't seem to have a direct analog, but is best covered by 4.7.2, 4.7.4, and 4.7.5. Basically, what I'm saying is that talking about the binary header before the protocol frame header doesn't make sense to me, and seems like it should be related to or after shown after 4.7.3.2. Additionally, splitting the StartService and its ACK response "how it's used" documentation doesn't make sense to me either and seems like it should be either all before or after the protocol header.

5. 👍

7. I understand now. The flow is Mobile send start service. Mobile send handshake error frame. Core should send StartServiceNAK. I though that there was supposed to be a NAK to the handshake error, and that's why I got confused.

8. This is not described in the current protocol_spec, but this is how it works already and seems to me to be an important piece that needs to be added to this proposal for it to accurately describe the security spec.

ashwink11 commented 4 years ago

Kujtim is on vacation. We will respond to open items when he is back. Request you to please defer this proposal until then. Thank you.

theresalech commented 4 years ago

The author has advised that he is now available to respond to comments on the review issue, so the Steering Committee can vote to bring this back into review when time allows.

theresalech commented 3 years ago

The Steering Committee has voted to bring this proposal back into review. The review will take place until our next meeting, on October 27, 2020 (2020-10-27).

kshala-ford commented 3 years ago

1. We agree to the changes stated in this comment.

2. 6. TL;DR we will need to detail out the library changes in this proposal.

You're right. Accepting this proposal just to add a section in the protocol spec doesn't make sense as most of the code would be violating the "extended" protocol spec. I don't want to go into details but the libraries can be client security. The library would need to send the client hello and process the handshake prior to send StartService. This is also the case for RPC message protection when switching to protected service.

I would like to return the proposal for revision which is to allow the author to detail out the library changes. Otherwise the proposal need to be deferred as I need time to write the suggestion and comment to this conversation.

3. According to this comment all questions were answered and no action is required.

4. I need more time to look into reordering. Keep in mind that the protocol header definition is already described in a previous section. I could only link to the previous section but this is not the case anywhere in the prot spec. This is why this proposal only needs to describe the security query binary header format, types and enums. Then I only need to describe how the protocol frame header is used to encapsulate security query headers. I get your point but you must have the big picture in mind as section 2 describes the frame header. It's not necessary to describe it again.

5. We agree to changes stated in this comment.

7. According to this comment the question was answered and no action is required.

8. still an open item. The proposal should consider describing RPC message protection capabilities to lift existing service sessions to become protected. Also RPC service must start unprotected. I think we can agree here with this to be a revision.

9. @JackLivio I had no idea about the PR but I want to note that I worked on the security specification in May and created the PR in sdl_evolution June 10. The protocol change clearly requires the steering committee agreement as the change is quite large affecting almost all platforms. I can review the PR and make sure that all pieces are covered by the proposal.

theresalech commented 3 years ago

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

joeljfischer commented 3 years ago

2\6. I'm a little skeptical that the right way to go is to extend iOS / Android to be TLS clients instead of reducing the scope of Core to only be a client, but I'm willing to wait to see what changes you want to make to the proposal for iOS / Android changes before having that discussion in detail. It's hard without concrete statements to address.

4. Sounds good. Perhaps I'm missing some context by not looking at the entire new protocol spec in context. I do think that we can improve the flow of information to be more top-down of the entire protocol packet. I'll wait for you to do your investigation though.

8. 👍

kshala-ford commented 3 years ago

2/6. well in that case before I start to spend time and specify client TLS support for SDL libraries I would like to as the steering committee to decide whether

• A: Client and Server TLS is desired or
• B: Server TLS only is sufficient.

The former would require a decent amount of implementation and testing effort to add Client TLS to mobile libraries (incl. sdl_security repositories). The latter is much easier but would mean to remove Client TLS support in sdl_core and OEMs won't have the ability to choose the mode.

Unless someone insists to have Client TLS, my suggestion is to remove Client TLS because it's obviously not used by anyone.

theresalech commented 3 years ago

Since the PM and author are in agreement on most proposed revisions, I've provided a summary of the items in discussion below. Regarding Item 2, if any Steering Committee representatives desire both Client and Server TLS, please advise by leaving a comment on this review issue or raising the concern during our next meeting.

Open Items

4. Author to investigate and respond.

9. PM believes contents from https://github.com/smartdevicelink/protocol_spec/pull/30 are included in proposal, and is awaiting the proposal author's final confirmation.

Author to Revise

1. Remove Single Frame and describe that frame info is reserved but the communication partners should set the field to 0 instead of an undefined value.

2/6. Remove Client TLS support in SDL Core and note that Server TLS only is sufficient.

5. Add "The JSON data and one-byte binary data contain the same data" to this section:

If an error occurs during the TLS handshake a notification is sent with an error code and error text as JSON data. Additionally the error code is added as a single byte binary data.

8. In 4.7.2 (Start Service), describe RPC message protection capabilities to lift existing service sessions to become protected. Also RPC service must start unprotected.

Closed - No Action Required: 3, 7

kshala-ford commented 3 years ago

@theresalech thank you for the summary. I think there's an offset of -1 to your numbers 6-8 which should be 7-9 (e.g. closed should be 3 and 7 not 6).

10. As 2. is reduced in scope I still would like to add one change to the security library and also to the mobile libraries. At the moment the libs assume the security level for audio & video services by the presence of a make-matching security manager. The RPC service security level is determined by content in the OnPermissionChange. Because an OEM controls Core configuration and the security manager build, the OEM should be able to match the security configurations in both ends. Mainly ForceProtectedServices and ForceUnprotectedServices should be added to the security libraries allowing the mobile libraries to know how to start services.

theresalech commented 3 years ago

Hi @kshala-ford, you're right, thank you for pointing that out. I've edited my summary comment to have correct numbering.

theresalech commented 3 years ago

The Steering Committee voted to keep this proposal in review to allow for further discussion on the following open items: 4, 9, and 10.

kshala-ford commented 3 years ago

4. I reordered the sections moving defining content to the Control Service section 5.1. Also adding a new section 7 for secured communication. The reason is that defining sections must come before describing the behavior. Therefore section 4 cannot be used for this anymore. If you see a possibility please let me know

  1. Overview
  2. Frames
  3. Frame Types
  4. Establishing Communication
  5. Services
  5.1 Control Service
+ 5.1.1 Security Query
+ 5.1.1.1 Payload (include hybrid query payload)
+ 5.1.1.2 Binary Header
+ 5.1.1.2.1 Binary Header Fields
+ 5.1.2 Error Frames
+ 5.1.2.1 Payload
+ 5.1.2.2 JSON structure
+ 5.1.2.3 Error codes
  5.2. RPC Service
  5.2.1 Binary Header
  5.2.1.1 Binary Header Fields
  6. Ending Communication
+ 7. Secured Communication
+ 7.1 Authentication
+ 7.2 Handshake Frames
+ 7.2.1 SDL Protocol Frame Header
+ 7.2.2 Security Query Binary Header
+ 7.3 Error handling

9. I share the opinion of the PM. The proposal includes the contents of protocol_spec#30

10. Looking forward to feedback from others.

theresalech commented 3 years ago

Please find an updated summary of the items in discussion below, including the PM's response to Item 10. Regarding Item 2, if any Steering Committee representatives desire both Client and Server TLS, please advise by leaving a comment on this review issue or raising the concern during our next meeting.

Open Items

10. To maintain the scope of the current proposal, we think a separate proposal describing the public API changes necessary to encapsulate this hardcoded forced security would be best.

Author to Revise

1. Remove Single Frame and describe that frame info is reserved but the communication partners should set the field to 0 instead of an undefined value.

2/6. Remove Client TLS support in SDL Core and note that Server TLS only is sufficient.

4. Update the protocol spec sections to the following:

  1. Overview
  2. Frames
  3. Frame Types
  4. Establishing Communication
  5. Services
  5.1 Control Service
+ 5.1.1 Security Query
+ 5.1.1.1 Payload (include hybrid query payload)
+ 5.1.1.2 Binary Header
+ 5.1.1.2.1 Binary Header Fields
+ 5.1.2 Error Frames
+ 5.1.2.1 Payload
+ 5.1.2.2 JSON structure
+ 5.1.2.3 Error codes
  5.2. RPC Service
  5.2.1 Binary Header
  5.2.1.1 Binary Header Fields
  6. Ending Communication
+ 7. Secured Communication
+ 7.1 Authentication
+ 7.2 Handshake Frames
+ 7.2.1 SDL Protocol Frame Header
+ 7.2.2 Security Query Binary Header
+ 7.3 Error handling

5. Add "The JSON data and one-byte binary data contain the same data" to this section:

If an error occurs during the TLS handshake a notification is sent with an error code and error text as JSON data. Additionally the error code is added as a single byte binary data.

8. In 4.7.2 (Start Service), describe RPC message protection capabilities to lift existing service sessions to become protected. Also RPC service must start unprotected.

Closed - No Action Required: 3, 7, 9

jordynmackool commented 3 years ago

The Steering Committee voted to keep this proposal in review to allow further discussion on the open item 10 called out in this comment.

kshala-ford commented 3 years ago

10. I think the question is if the protocol specification should contain requirements to when and how a secured communication should be established. In that case it's safe to add a section to describe the security library change. It's a similar situation as for 8. to describe RPC message protection when to use secured communication even though this overlaps to areas beyond the protocol spec. Describing the security library change is actually very small almost not worth a separate proposal in my opinion.

joeljfischer commented 3 years ago

10. I think that because these changes are additive, they should be discussed in a separate proposal. Additionally, these changes need to be explained in detail and can't just have a passing mention. What happens if an old security library is paired to a new SDL lib and visa versa, etc. What happens if the security manager says "start force protected" but the head unit doesn't support protection? Because there's no longer a handshake, these changes make it impossible to have different security requirements for different OEM vehicles, which is a significant problem.

I really think we're extending the scope of the changes beyond the title to add those changes here. It's no longer the security specification, but "adding the ability to start services force protected / unprotected the first time in the libraries by updating the security library communication interface." That's a very different thing. To keep this proposal moving, I would strongly suggest putting your requested changes in a separate proposal.

kshala-ford commented 3 years ago

10. As I mentioned previously this proposal is not meant to only include sections in the protocol spec but also implement the spec properly through the other platforms. If we would only add content to the spec, then many of the platforms are violating that spec. If the spec implementation is rather major and needs discussion we will see this proposal being returned for revisions for the platform anyways. Also, I don't see a reason to not include the security libraries.

I'm not asking for just a passing mention but to add a section describing the change for the security library because it would be a change to the public API library. I am only considering adding two getters returning a serviceID list to reflect the ini file. According to the list, the SDL libs should start the service either protected or unprotected. As the security library and the connected IVI are OEM paired by the make list this addition should work just fine. In fact the java version already includes a getter for service lists. Up to you to consider this a library misalignment or a platform specific behavior.

I will not be able to create a new proposal anytime soon and go through the whole process for approval. I barely have enough time to get my four proposals moving forward. I will leave it up to you if you want to improve the security library as I want or if you rather like to keep it as it is.

joeljfischer commented 3 years ago

10. To keep this proposal scoped and discussion on one topic, I would strongly suggest keeping it as is and opening a new proposal for your new changes. No matter if the information is in a new proposal or this one, it will take the same amount of time to discuss. If it's added to this proposal, to make any of the changes in this proposal, we'd have to wait for you to add that info, argue about it for months (you know it'll happen lol), and wait for the full implementation. I think it's wiser to split it up and wait until you have the time to get a new proposal through the process.

kshala-ford commented 3 years ago

10. I am sorry that you see it this way. Creating a new proposal will add a lot of time for this change until it'll be accepted and I believe it'll be more than having it the suggestion be part of another proposal which is in the same context. Not only would this new proposal be excluded from 2021 roadmap, I agree, it'll be waiting for month to pass the whole SC process. I can't imagine that we would argue for months for something that is already included in the java repository.

jordynmackool commented 3 years ago

The Steering Committee voted to return this proposal for revisions. The author is to update the proposal to include the agreed-upon revisions in this comment. Regarding open item number 10, the PM advised that a new proposal separate from this one should be created for describing the public API changes necessary to handle changes to the security manager and its API.

jordynmackool commented 3 years ago

Closing as inactive. The issue will remain in a returned for revisions state. The author should notify the Project Maintainer when a revisions PR has been submitted.

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 1, 2021.

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

joeljfischer commented 3 years ago

I'm going to restart numbering.

1. In "Change 1: Update Frame Header Fields description", the "Frame Info" table section, you note: "Communication partners should set this field to zero", can you please define "Communication partners"?

2. I think it would be clearer to change "See "Secured Communication" section for more details." to "See "7. Secured Communication" section for more details."

3. Under Section 7, you write, "It is possible to establish a secured and encrypted communication with the system by setting the frame header encryption flag to 1 when starting a new service."

I think this should be expanded to say something like this: "It is possible to establish a secured and encrypted communication with the system by setting the frame header encryption flag to 1 when starting a new service or by sending another StartService with the encryption flag to 1 when the service is already established (this the required flow for the RPC service)."

dboltovskyi commented 3 years ago

1. Propose to replace the sentence with "Communication partners" by "Field reserved, mobile libraries should set it to zero by default"
2. Ok
3. Ok

joeljfischer commented 3 years ago

1. Does Core ignore this value or will something go wrong if the value isn't 0? If the former, I don't think we need this sentence. If the latter, then we either need to change Core to accept and ignore any value, or we need to change the spec to require 0 as the value from the app libraries.

Jack-Byrne commented 3 years ago

@dboltovskyi

4. Question about the impact on existing code section. I was under the impression that this proposal is to align the documentation of the protocol spec with the existing behavior of SDL Core. The proposed solution section mentions that these updates were reversed engineered. The proposed solution section provides no explanation for "improved error handling" in sdl core or the changes to SDLProtocolSecurity class in the mobile libraries.

The language in the Impact to the existing code section implies these changes are optional? I believe the impact section needs to remove these code changes to Core/mobile libraries, or the proposed solution section should be updated to include more details for the Core and mobile library changes (if applicable).

dboltovskyi commented 3 years ago

@joeljfischer

1. Good point. We have checked the current behavior of Core and Mobile libraries and would agree to change the spec to require 0 as the value from the app libraries

@JackLivio

4. Thank you for this catch. Since this proposal doesn't require any changes except the updates of documentation in protocol spec, we agree to:
   • update "Impact on existing code" section and to remove code changes related to Core/Mobile libraries
   • update "Impacted Platforms" and retain only the "Protocol"

theresalech commented 3 years ago

The Steering Committee voted to keep this proposal in review to allow the PM to investigate the impact of the suggested change for Item 1, and for additional discussion about the item on the review issue. It was acknowledged that the PM and Author have agreed to the changes described in Items 2, 3, and 4.

joeljfischer commented 3 years ago

1. I think that's the wrong way to go. It's a major spec change (and therefore major version change). It would be a bugfix to have Core ignore the value if it currently matters. The spec currently says that it doesn't matter, therefore if this is supposed to be a clarification and not a major version change, I think we really only have one option.

dboltovskyi commented 3 years ago

@joeljfischer

1. We have made appropriate update in Core in a specific branch and successfully tested it. The required changes are rather small. So we agree with the suggestion.

• Protocol specification wouldn't require an update on this point
• New issue against Core will be raised and appropriate fix is provided

theresalech commented 3 years ago

Please find below a summary of the revisions agreed to by the author and Project Maintainer:

1. Remove "Note: Communication partners should set this field to zero" from the "Frame Info" and "Data Size" descriptions in the Change 1 table (https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0317-sdl-protocol-security-specification.md#change-1-update-frame-header-fields-description).
2. In Proposed solution section (4.2.5 Start Service), change "See "Secured Communication" section for more details." to "See "7. Secured Communication" section for more details."
3. In Proposed solution section (7. Secured Communication), change "It is possible to establish a secured and encrypted communication with the system by setting the frame header encryption flag to 1 when starting a new service." to "It is possible to establish a secured and encrypted communication with the system by setting the frame header encryption flag to 1 when starting a new service or by sending another StartService with the encryption flag to 1 when the service is already established (this the required flow for the RPC service)."
4. Update Impact on existing code section to remove code changes related to Core/Mobile libraries, and update Impacted Platforms to only include "Protocol".

theresalech commented 3 years ago

The Steering Committee voted to accept this proposal with the following revisions:

1. Remove "Note: Communication partners should set this field to zero" from the "Frame Info" and "Data Size" descriptions in the Change 1 table (https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0317-sdl-protocol-security-specification.md#change-1-update-frame-header-fields-description).
2. In Proposed solution section (4.2.5 Start Service), change "See "Secured Communication" section for more details." to "See "7. Secured Communication" section for more details."
3. In Proposed solution section (7. Secured Communication), change "It is possible to establish a secured and encrypted communication with the system by setting the frame header encryption flag to 1 when starting a new service." to "It is possible to establish a secured and encrypted communication with the system by setting the frame header encryption flag to 1 when starting a new service or by sending another StartService with the encryption flag to 1 when the service is already established (this the required flow for the RPC service)."
4. Update Impact on existing code section to remove code changes related to Core/Mobile libraries, and update Impacted Platforms to only include "Protocol".

theresalech commented 3 years ago

@AKalinich-Luxoft @dboltovskyi please advise when a new PR has been entered to update the proposal to reflect the agreed upon revisions. I'll then merge the PR so the proposal is up to date, and enter issues in impacted repositories for implementation. Thanks!

theresalech commented 3 years ago

Proposal has been updated to reflect revisions, and implementation issue has been entered: https://github.com/smartdevicelink/protocol_spec/issues/40

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[theresalech]

The Steering Committee voted to keep this proposal in review until our next meeting, to allow for additional time for discussion on the review issue.

[joeljfischer]

1. Understood. Why was the name Single Frame chosen, and can we choose something else specific to security?

2. This and 6 and clearly related. I'm still having trouble understanding what you believe that "fully implemented" for this proposal is. In other words, for this proposal to be marked "implemented", is all that needs to happen an update to the protocol spec (and then bugs would be opened on the other platforms and fixed as possible), or do all platforms need to be aligned on the implementation of the new protocol spec. The latter seems to be implied by your statements here and in 6 (and in the "impacted platforms" section), but whichever you believe, I think that it should be stated in clear and unequivocal terms. The former requires only a days worth of effort, while the latter would require weeks for many platforms.

3. 👍

4. Why not "Security Query Binary Header"; since there are multiple different headers (protocol header, binary header) it seems like we should specify which header is being described. I'm not sure that the link you added helps your case. It starts by talking about the protocol header, not the binary header. It seems like 4.7.1 should be a sub-point of 4.7.3, and it's strange that 4.7.2 and 4.7.4 are divided by a different type of header. I think that the flow of information can be improved here.

Basically my suggestion is, move 4.7.2 to be 4.7.1, move 4.7.4 to be 4.7.2, move 4.7.1 to be 4.7.3.1.

5. Ah, okay. I thought that was an oversight. Maybe we can put a comment there saying "The JSON data and one-byte binary data contain the same data"?

6. See 2.

7. I don't see a requirement in this document that the client (I assume that's what's being referenced here rather than Core) is supposed to send a NAK to an error message. All I see is 4.7.5.4, which notes that the connection to be reset. (Note: That section also doesn't show how that reset should occur, should one side or the other send an EndService?).

8. I think that 4.7.2 (Start Service) should describe that the RPC service needs to be started as unencrypted, then moved to encrypted by sending another Start Service at a later point, and that other services can do the same thing to move from unencrypted to encrypted. I apologize if this is described and I missed it.

[kshala-ford]

1. I took a closer look at sdl_core code again and I think I need to remove this change from the proposal and leave frame info reserved. This is the difficulty of reverse engineering. You fumble around in the dark not knowing why things are done in a certain way. At least sdl_ios sets frame info to Single Frame serverMessageHeader.frameData = SDLFrameInfoSingleFrame; and after further investigation I saw that sdl_ios trying to keep frame info zero for single frames independently of the service type though it's not specified.

So my suggestion is to textually describe that frame info is reserved but the communication partners should set the field to 0 (instead of an undefined value).

2. The latter is the case. I don't know why you believe the former would be my desired approach. You may have skipped the impacted platforms of this proposal. With this list, the proposal acceptance will result in GitHub issues for each of this proposal. The proposal is therefore not "fully implemented" as long as not all issues are addressed.

I can help you out by adding following statement:

The solution is to provide a new section into the protocol spec around security and protection. The baseline for this section is reverse engineered from SDL Core and the currently implemented behavior of the security manager but also existing documentation from SDL Core and the app libraries. The acceptance of this section will require a code review in other platforms and aligning existing code with the specification. None of the specifications should touch any public API in other platforms however if the changes on a single platform are severe/major a proposal revision or a subsequent proposal describing the change should be performed for SDLC to review and accept.

4. I think you're requesting multiple changes in this single item. I am okay changing occurrences of Query Header or Binary Query Header to Security Query Binary Header as I found I mixed this naming up. This name aligns with the title Security Query of 4.7.1

4.b. I am strongly against moving sub-sections around!! In alignment of the existing protocol spec, this security sections follows the same red line.

a. Describe the header and size (aligns with section 2 of the protocol_spec). b. Describe the header fields and possible values (aligns with section 2.3 of the protocol_spec). c. Describe how the header is being used (aligns with section 4 of the protocol_spec)

the red line may not be as per your personal taste but it follows a purpose of. You can't start specifying things without providing all information to the specification.

5. Make them one sentence? Sure.

7. See

If the authentication fails for some reason the system will reset the TLS connection and return a StartService NAK frame.

But for more visibility this should definitely be describe in error handling. Sorry for that.

8. This sequence is not described in this proposal and it doesn't seem part of the existing protocol spec. I would like to include @yang1070 here who can help providing a description for RPC message protection.

[Jack-Byrne]

The proposed solution is confusing because it is creating a new documentation section that is mixed with details that are reversed engineered with details that should be considered new features. There is little indication in the proposed solution on what is reversed engineered and what is to be added/changed.

Livio submitted a pr to add missing documentation for security query to the protocol spec on June 9th here: https://github.com/smartdevicelink/protocol_spec/pull/30/files. This contains the information we found was missing from the protocol spec and needed to be documented.

In our opinion adding missing documentation is a bug and does not require a proposal. We could have had this discussion/review on the protocol spec pr we created. If you disagree with that then we would still suggest separating this proposal into two sections. First section would document the existing behavior. Second section includes the additions and changes this proposal wants to make to existing behavior.

[joeljfischer]

1. 👍

2\6. Thank you for clarifying. Given that that is the case, I believe there may be an issue that I see with this proposal. The proposal is implying that the app libraries need to support features like being the TLS client. However, I don't believe that that is a supportable case, because the app library has to be the one to send the StartService requests, and therefore must be the TLS server. So, even though Core currently supports being client or server, I think that the reverse engineering of the mobile platforms should reduce the scope of required implementation changes and we should make that clear in the proposal.

4.b. I'm afraid I'm not following you. Based on your "red line", your proposal seems to be out of alignment with the protocol_spec. Section 2 is talking about the protocol header, which you don't talk about until 4.7.3.1, while you're talking about the binary header in 4.7.1. Section 2.3 is still talking about the protocol header, so I don't see how that follows either. Then Section 4, where you talk about "how it's used" doesn't seem to have a direct analog, but is best covered by 4.7.2, 4.7.4, and 4.7.5. Basically, what I'm saying is that talking about the binary header before the protocol frame header doesn't make sense to me, and seems like it should be related to or after shown after 4.7.3.2. Additionally, splitting the StartService and its ACK response "how it's used" documentation doesn't make sense to me either and seems like it should be either all before or after the protocol header.

5. 👍

7. I understand now. The flow is Mobile send start service. Mobile send handshake error frame. Core should send StartServiceNAK. I though that there was supposed to be a NAK to the handshake error, and that's why I got confused.

8. This is not described in the current protocol_spec, but this is how it works already and seems to me to be an important piece that needs to be added to this proposal for it to accurately describe the security spec.

[ashwink11]

Kujtim is on vacation. We will respond to open items when he is back. Request you to please defer this proposal until then. Thank you.

[theresalech]

The author has advised that he is now available to respond to comments on the review issue, so the Steering Committee can vote to bring this back into review when time allows.

[theresalech]

The Steering Committee has voted to bring this proposal back into review. The review will take place until our next meeting, on October 27, 2020 (2020-10-27).

[kshala-ford]

1. We agree to the changes stated in this comment.

2. 6. TL;DR we will need to detail out the library changes in this proposal.

You're right. Accepting this proposal just to add a section in the protocol spec doesn't make sense as most of the code would be violating the "extended" protocol spec. I don't want to go into details but the libraries can be client security. The library would need to send the client hello and process the handshake prior to send StartService. This is also the case for RPC message protection when switching to protected service.

I would like to return the proposal for revision which is to allow the author to detail out the library changes. Otherwise the proposal need to be deferred as I need time to write the suggestion and comment to this conversation.

3. According to this comment all questions were answered and no action is required.

4. I need more time to look into reordering. Keep in mind that the protocol header definition is already described in a previous section. I could only link to the previous section but this is not the case anywhere in the prot spec. This is why this proposal only needs to describe the security query binary header format, types and enums. Then I only need to describe how the protocol frame header is used to encapsulate security query headers. I get your point but you must have the big picture in mind as section 2 describes the frame header. It's not necessary to describe it again.

5. We agree to changes stated in this comment.

7. According to this comment the question was answered and no action is required.

8. still an open item. The proposal should consider describing RPC message protection capabilities to lift existing service sessions to become protected. Also RPC service must start unprotected. I think we can agree here with this to be a revision.

9. @JackLivio I had no idea about the PR but I want to note that I worked on the security specification in May and created the PR in sdl_evolution June 10. The protocol change clearly requires the steering committee agreement as the change is quite large affecting almost all platforms. I can review the PR and make sure that all pieces are covered by the proposal.

[theresalech]

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

[joeljfischer]

2\6. I'm a little skeptical that the right way to go is to extend iOS / Android to be TLS clients instead of reducing the scope of Core to only be a client, but I'm willing to wait to see what changes you want to make to the proposal for iOS / Android changes before having that discussion in detail. It's hard without concrete statements to address.

4. Sounds good. Perhaps I'm missing some context by not looking at the entire new protocol spec in context. I do think that we can improve the flow of information to be more top-down of the entire protocol packet. I'll wait for you to do your investigation though.

8. 👍

[kshala-ford]

2/6. well in that case before I start to spend time and specify client TLS support for SDL libraries I would like to as the steering committee to decide whether

• A: Client and Server TLS is desired or
• B: Server TLS only is sufficient.

The former would require a decent amount of implementation and testing effort to add Client TLS to mobile libraries (incl. sdl_security repositories). The latter is much easier but would mean to remove Client TLS support in sdl_core and OEMs won't have the ability to choose the mode.

Unless someone insists to have Client TLS, my suggestion is to remove Client TLS because it's obviously not used by anyone.

[theresalech]

Since the PM and author are in agreement on most proposed revisions, I've provided a summary of the items in discussion below. Regarding Item 2, if any Steering Committee representatives desire both Client and Server TLS, please advise by leaving a comment on this review issue or raising the concern during our next meeting.

Open Items

4. Author to investigate and respond.

9. PM believes contents from https://github.com/smartdevicelink/protocol_spec/pull/30 are included in proposal, and is awaiting the proposal author's final confirmation.

Author to Revise

1. Remove Single Frame and describe that frame info is reserved but the communication partners should set the field to 0 instead of an undefined value.

2/6. Remove Client TLS support in SDL Core and note that Server TLS only is sufficient.

5. Add "The JSON data and one-byte binary data contain the same data" to this section:

If an error occurs during the TLS handshake a notification is sent with an error code and error text as JSON data. Additionally the error code is added as a single byte binary data.

8. In 4.7.2 (Start Service), describe RPC message protection capabilities to lift existing service sessions to become protected. Also RPC service must start unprotected.

Closed - No Action Required: 3, 7

[kshala-ford]

@theresalech thank you for the summary. I think there's an offset of -1 to your numbers 6-8 which should be 7-9 (e.g. closed should be 3 and 7 not 6).

10. As 2. is reduced in scope I still would like to add one change to the security library and also to the mobile libraries. At the moment the libs assume the security level for audio & video services by the presence of a make-matching security manager. The RPC service security level is determined by content in the OnPermissionChange. Because an OEM controls Core configuration and the security manager build, the OEM should be able to match the security configurations in both ends. Mainly ForceProtectedServices and ForceUnprotectedServices should be added to the security libraries allowing the mobile libraries to know how to start services.

[theresalech]

Hi @kshala-ford, you're right, thank you for pointing that out. I've edited my summary comment to have correct numbering.

[theresalech]

The Steering Committee voted to keep this proposal in review to allow for further discussion on the following open items: 4, 9, and 10.

[kshala-ford]

4. I reordered the sections moving defining content to the Control Service section 5.1. Also adding a new section 7 for secured communication. The reason is that defining sections must come before describing the behavior. Therefore section 4 cannot be used for this anymore. If you see a possibility please let me know

  1. Overview
  2. Frames
  3. Frame Types
  4. Establishing Communication
  5. Services
  5.1 Control Service
+ 5.1.1 Security Query
+ 5.1.1.1 Payload (include hybrid query payload)
+ 5.1.1.2 Binary Header
+ 5.1.1.2.1 Binary Header Fields
+ 5.1.2 Error Frames
+ 5.1.2.1 Payload
+ 5.1.2.2 JSON structure
+ 5.1.2.3 Error codes
  5.2. RPC Service
  5.2.1 Binary Header
  5.2.1.1 Binary Header Fields
  6. Ending Communication
+ 7. Secured Communication
+ 7.1 Authentication
+ 7.2 Handshake Frames
+ 7.2.1 SDL Protocol Frame Header
+ 7.2.2 Security Query Binary Header
+ 7.3 Error handling

9. I share the opinion of the PM. The proposal includes the contents of protocol_spec#30

10. Looking forward to feedback from others.

[theresalech]

Please find an updated summary of the items in discussion below, including the PM's response to Item 10. Regarding Item 2, if any Steering Committee representatives desire both Client and Server TLS, please advise by leaving a comment on this review issue or raising the concern during our next meeting.

Open Items

10. To maintain the scope of the current proposal, we think a separate proposal describing the public API changes necessary to encapsulate this hardcoded forced security would be best.

Author to Revise

1. Remove Single Frame and describe that frame info is reserved but the communication partners should set the field to 0 instead of an undefined value.

2/6. Remove Client TLS support in SDL Core and note that Server TLS only is sufficient.

4. Update the protocol spec sections to the following:

  1. Overview
  2. Frames
  3. Frame Types
  4. Establishing Communication
  5. Services
  5.1 Control Service
+ 5.1.1 Security Query
+ 5.1.1.1 Payload (include hybrid query payload)
+ 5.1.1.2 Binary Header
+ 5.1.1.2.1 Binary Header Fields
+ 5.1.2 Error Frames
+ 5.1.2.1 Payload
+ 5.1.2.2 JSON structure
+ 5.1.2.3 Error codes
  5.2. RPC Service
  5.2.1 Binary Header
  5.2.1.1 Binary Header Fields
  6. Ending Communication
+ 7. Secured Communication
+ 7.1 Authentication
+ 7.2 Handshake Frames
+ 7.2.1 SDL Protocol Frame Header
+ 7.2.2 Security Query Binary Header
+ 7.3 Error handling

5. Add "The JSON data and one-byte binary data contain the same data" to this section:

If an error occurs during the TLS handshake a notification is sent with an error code and error text as JSON data. Additionally the error code is added as a single byte binary data.

8. In 4.7.2 (Start Service), describe RPC message protection capabilities to lift existing service sessions to become protected. Also RPC service must start unprotected.

Closed - No Action Required: 3, 7, 9

[jordynmackool]

The Steering Committee voted to keep this proposal in review to allow further discussion on the open item 10 called out in this comment.

[kshala-ford]

10. I think the question is if the protocol specification should contain requirements to when and how a secured communication should be established. In that case it's safe to add a section to describe the security library change. It's a similar situation as for 8. to describe RPC message protection when to use secured communication even though this overlaps to areas beyond the protocol spec. Describing the security library change is actually very small almost not worth a separate proposal in my opinion.

[joeljfischer]

10. I think that because these changes are additive, they should be discussed in a separate proposal. Additionally, these changes need to be explained in detail and can't just have a passing mention. What happens if an old security library is paired to a new SDL lib and visa versa, etc. What happens if the security manager says "start force protected" but the head unit doesn't support protection? Because there's no longer a handshake, these changes make it impossible to have different security requirements for different OEM vehicles, which is a significant problem.

I really think we're extending the scope of the changes beyond the title to add those changes here. It's no longer the security specification, but "adding the ability to start services force protected / unprotected the first time in the libraries by updating the security library communication interface." That's a very different thing. To keep this proposal moving, I would strongly suggest putting your requested changes in a separate proposal.

[kshala-ford]

10. As I mentioned previously this proposal is not meant to only include sections in the protocol spec but also implement the spec properly through the other platforms. If we would only add content to the spec, then many of the platforms are violating that spec. If the spec implementation is rather major and needs discussion we will see this proposal being returned for revisions for the platform anyways. Also, I don't see a reason to not include the security libraries.

I'm not asking for just a passing mention but to add a section describing the change for the security library because it would be a change to the public API library. I am only considering adding two getters returning a serviceID list to reflect the ini file. According to the list, the SDL libs should start the service either protected or unprotected. As the security library and the connected IVI are OEM paired by the make list this addition should work just fine. In fact the java version already includes a getter for service lists. Up to you to consider this a library misalignment or a platform specific behavior.

I will not be able to create a new proposal anytime soon and go through the whole process for approval. I barely have enough time to get my four proposals moving forward. I will leave it up to you if you want to improve the security library as I want or if you rather like to keep it as it is.

[joeljfischer]

10. To keep this proposal scoped and discussion on one topic, I would strongly suggest keeping it as is and opening a new proposal for your new changes. No matter if the information is in a new proposal or this one, it will take the same amount of time to discuss. If it's added to this proposal, to make any of the changes in this proposal, we'd have to wait for you to add that info, argue about it for months (you know it'll happen lol), and wait for the full implementation. I think it's wiser to split it up and wait until you have the time to get a new proposal through the process.

[kshala-ford]

10. I am sorry that you see it this way. Creating a new proposal will add a lot of time for this change until it'll be accepted and I believe it'll be more than having it the suggestion be part of another proposal which is in the same context. Not only would this new proposal be excluded from 2021 roadmap, I agree, it'll be waiting for month to pass the whole SC process. I can't imagine that we would argue for months for something that is already included in the java repository.

[jordynmackool]

The Steering Committee voted to return this proposal for revisions. The author is to update the proposal to include the agreed-upon revisions in this comment. Regarding open item number 10, the PM advised that a new proposal separate from this one should be created for describing the public API changes necessary to handle changes to the security manager and its API.

[jordynmackool]

Closing as inactive. The issue will remain in a returned for revisions state. The author should notify the Project Maintainer when a revisions PR has been submitted.

[theresalech]

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 1, 2021.

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

[joeljfischer]

I'm going to restart numbering.

1. In "Change 1: Update Frame Header Fields description", the "Frame Info" table section, you note: "Communication partners should set this field to zero", can you please define "Communication partners"?

2. I think it would be clearer to change "See "Secured Communication" section for more details." to "See "7. Secured Communication" section for more details."

3. Under Section 7, you write, "It is possible to establish a secured and encrypted communication with the system by setting the frame header encryption flag to 1 when starting a new service."

I think this should be expanded to say something like this: "It is possible to establish a secured and encrypted communication with the system by setting the frame header encryption flag to 1 when starting a new service or by sending another StartService with the encryption flag to 1 when the service is already established (this the required flow for the RPC service)."

[dboltovskyi]

1. Propose to replace the sentence with "Communication partners" by "Field reserved, mobile libraries should set it to zero by default"
2. Ok
3. Ok

[joeljfischer]

1. Does Core ignore this value or will something go wrong if the value isn't 0? If the former, I don't think we need this sentence. If the latter, then we either need to change Core to accept and ignore any value, or we need to change the spec to require 0 as the value from the app libraries.

[Jack-Byrne]

@dboltovskyi

4. Question about the impact on existing code section. I was under the impression that this proposal is to align the documentation of the protocol spec with the existing behavior of SDL Core. The proposed solution section mentions that these updates were reversed engineered. The proposed solution section provides no explanation for "improved error handling" in sdl core or the changes to SDLProtocolSecurity class in the mobile libraries.

The language in the Impact to the existing code section implies these changes are optional? I believe the impact section needs to remove these code changes to Core/mobile libraries, or the proposed solution section should be updated to include more details for the Core and mobile library changes (if applicable).

[dboltovskyi]

@joeljfischer

1. Good point. We have checked the current behavior of Core and Mobile libraries and would agree to change the spec to require 0 as the value from the app libraries

@JackLivio

4. Thank you for this catch. Since this proposal doesn't require any changes except the updates of documentation in protocol spec, we agree to:
   • update "Impact on existing code" section and to remove code changes related to Core/Mobile libraries
   • update "Impacted Platforms" and retain only the "Protocol"

[theresalech]

The Steering Committee voted to keep this proposal in review to allow the PM to investigate the impact of the suggested change for Item 1, and for additional discussion about the item on the review issue. It was acknowledged that the PM and Author have agreed to the changes described in Items 2, 3, and 4.

[joeljfischer]

1. I think that's the wrong way to go. It's a major spec change (and therefore major version change). It would be a bugfix to have Core ignore the value if it currently matters. The spec currently says that it doesn't matter, therefore if this is supposed to be a clarification and not a major version change, I think we really only have one option.

[dboltovskyi]

@joeljfischer

1. We have made appropriate update in Core in a specific branch and successfully tested it. The required changes are rather small. So we agree with the suggestion.

• Protocol specification wouldn't require an update on this point
• New issue against Core will be raised and appropriate fix is provided

[theresalech]

Please find below a summary of the revisions agreed to by the author and Project Maintainer:

1. Remove "Note: Communication partners should set this field to zero" from the "Frame Info" and "Data Size" descriptions in the Change 1 table (https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0317-sdl-protocol-security-specification.md#change-1-update-frame-header-fields-description).
2. In Proposed solution section (4.2.5 Start Service), change "See "Secured Communication" section for more details." to "See "7. Secured Communication" section for more details."
3. In Proposed solution section (7. Secured Communication), change "It is possible to establish a secured and encrypted communication with the system by setting the frame header encryption flag to 1 when starting a new service." to "It is possible to establish a secured and encrypted communication with the system by setting the frame header encryption flag to 1 when starting a new service or by sending another StartService with the encryption flag to 1 when the service is already established (this the required flow for the RPC service)."
4. Update Impact on existing code section to remove code changes related to Core/Mobile libraries, and update Impacted Platforms to only include "Protocol".

[theresalech]

The Steering Committee voted to accept this proposal with the following revisions:

1. Remove "Note: Communication partners should set this field to zero" from the "Frame Info" and "Data Size" descriptions in the Change 1 table (https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0317-sdl-protocol-security-specification.md#change-1-update-frame-header-fields-description).
2. In Proposed solution section (4.2.5 Start Service), change "See "Secured Communication" section for more details." to "See "7. Secured Communication" section for more details."
3. In Proposed solution section (7. Secured Communication), change "It is possible to establish a secured and encrypted communication with the system by setting the frame header encryption flag to 1 when starting a new service." to "It is possible to establish a secured and encrypted communication with the system by setting the frame header encryption flag to 1 when starting a new service or by sending another StartService with the encryption flag to 1 when the service is already established (this the required flow for the RPC service)."
4. Update Impact on existing code section to remove code changes related to Core/Mobile libraries, and update Impacted Platforms to only include "Protocol".

[theresalech]

@AKalinich-Luxoft @dboltovskyi please advise when a new PR has been entered to update the proposal to reflect the agreed upon revisions. I'll then merge the PR so the proposal is up to date, and enter issues in impacted repositories for implementation. Thanks!

[theresalech]

Proposal has been updated to reflect revisions, and implementation issue has been entered: https://github.com/smartdevicelink/protocol_spec/issues/40