[Accepted with Revisions] SDL 0317 - SDL Protocol Security Specification #1070
Comments
|
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:
It appears that the iOS library always assumes that it is the server. It sends the initial 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? |
|
1. Sorry I think a proper diff could have been better but it would be almost impossible to understand the context. Because
to
2. Please note:
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 5. The proposal states
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 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. |
|
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. |
|
1. Understood. Why was the name 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 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. |
|
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 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:
4. I think you're requesting multiple changes in this single item. I am okay changing occurrences of 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). 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
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. |
|
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. |
|
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 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. |
|
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
changed the title
|
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
changed the title
|
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). |
|
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. |
|
The Steering Committee voted to keep this proposal in review, to allow for further discussion on the review issue. |
|
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. 👍 |
|
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
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. |
jordynmackool
changed the title
|
Closing as inactive. The issue will remain in a |
theresalech
changed the title
|
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. |
|
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 |
|
|
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. |
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). |
@JackLivio
|
|
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. |
|
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. |
|
We have made appropriate update in Core in a specific branch and successfully tested it.
|
|
Please find below a summary of the revisions agreed to by the author and Project Maintainer:
|
theresalech
changed the title
|
The Steering Committee voted to accept this proposal with the following revisions:
|
|
@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! |
|
Proposal has been updated to reflect revisions, and implementation issue has been entered: smartdevicelink/protocol_spec#40 |
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:
#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:
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
The text was updated successfully, but these errors were encountered: