-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consent support chapter. #483
Conversation
Signed-off-by: Ulf Bjorkengren <[email protected]>
A rendered version of the PR is here: |
Signed-off-by: Ulf Bjorkengren <[email protected]>
spec/VISSv2_Core.html
Outdated
@@ -1123,7 +1126,14 @@ <h2>Client</h2> | |||
<li>The <a href="#device-roles-def">device</a>. It is in charge of running the Apps that make requests to the VISSv2 server</li> | |||
<li>The <a href="#application-roles-def">app. </a>It runs requests on behalf of the user.</li> | |||
<li>The <a href="#user-roles-def">user</a>. It delegates access rights to the app.</li> | |||
</ul> | |||
</ul> | |||
Besides the three sub-acor roles the client is also characterized by a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error "sub-acor"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
spec/VISSv2_Core.html
Outdated
are deployed in-vehicle the VIN parameter may be omitted, in all other deployment scenarios it shall be present. | ||
are deployed in-vehicle the VIN parameter may be omitted, in all other deployment scenarios it shall be present.<br> | ||
The purpose of the Client Id is to provide additional information to the consent end point that takes the decision on | ||
granting consent or not. A too uninformative description may lead to consents being denied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it is a bit unclear what is meant with "description". We do not intended to have a free text description as part of the Client Id, or?
Or are we missing a sentence like "The consent end point may based on the client id show a dialog to the vehicle owner"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A link is added to the Client chapter, where more info about the Client Id can be found.
Regarding the possibly missing sentence, I think we should avoid saying anything about the ECF behavior.
spec/VISSv2_Core.html
Outdated
However, a VISSv2 vehicle server has a capability to enforce consent results, i. e. to allow or block access to requested data. | ||
This can be leveraged in a model where the server receives consent results from an “External Consent Framework” (ECF) and uses that information to either grant client requests, | ||
or not, for data that is consent protected. How the ECF obtains the consent status is out-of-scope in this specification. | ||
A secure, local communication channel exits between the in-vehicle ECF and the server as shown in the figure below, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error "exits"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
A secure, local communication channel exits between the in-vehicle ECF and the server as shown in the figure below, | ||
over which the server can inquire about the consent status for data requested by a client. | ||
<figure id="fig-consent-architecture"> | ||
<img src="images/consent-architecture.jpg" alt="Consent architecture."> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the source of the image? Would it be relevant to include the source format in the PR so that the image can be modified if needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The image should be found in the images directory. It was initially missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the file images/consent-architecture.jpg
is not the source format, or? Or have you used a tool that works directly on *.jpg files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created it in my favorite drawing tool, Paint :). So it is the source format.
My drawings belong to the naivism art direction, you are very welcome to redraw it in another art direction :).
@@ -1723,6 +1734,71 @@ <h2>Access Control Selection</h2> | |||
</section> | |||
</section> | |||
|
|||
<section id="consent-support"> | |||
<h2>Consent support</h2> | |||
<p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a "This section is non-normative." statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could very well be so. Let us discuss that in the next WG meeting (cancelled next week).
As I believe it is more likely than not, I add it for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Ulf Bjorkengren <[email protected]>
spec/VISSv2_Core.html
Outdated
</ul> | ||
Besides the three sub-actor roles the client is also characterized by a | ||
<ul> | ||
<li>client identity.</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might need to talk about client registration and define the ClientID as a pointer to the data associated to it during registration. That way, tokens just need to include the clientid and the client context could be retrieved from the corresponding server using the clientid. It could be a local registration, but decoupling this process will simplify the description of the tokens and make it more clear, at least this is my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with what you propose.
Would it be possible for you to provide an update with what you propose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can prepare another PR but before doing that it might be better to agree in the general changes. My proposal would be to state that "We assume all clients are registered in the ecosystem manager, providing information about the device and application, but also information need for consent management. Upon registration, all clients will receive a Client ID. All servers should be able get the client information from the Client ID. Client registration is out of the scope of this specification" If we do that, we could include in the tokens the client ID instead of the context. We will also need to change a bit the text. I think client registration should not involve the user, that leaves the question whether we need the user in the tokens or not. I think it is relevant for the access grant token and consent, but maybe not needed for the access token. This is something we may need to discuss in more detail ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@isaacagudo I think the statement makes things clear and concise, so I am backing adding that. Also then to replace the context with the client Id in the token.
I agree that the registration as described in the statement should not include the user, but that it is needed in the consent context. If it is needed in the consent, does that not imply that the access token would be an appropriate bearer of the information? The triggering of obtaining consent (if not already done and being valid) starts (as I see it?) with the server receiving a request containing an access token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion consent should be triggered when requesting the access token, not when accessing the actual signals using the token, that why I suggested not including the user in the access token. However, it might be interesting to keep it for logging purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When thinking more about it, I agree that consent triggering is more appropriate when requesting the access token. But maybe we should keep the user in it anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree we can keep the user anyway
spec/VISSv2_Core.html
Outdated
<li>The consent status.</li> | ||
<li>The expiry time of the consent status.</li> | ||
</ul> | ||
This communication shall be carried out on a confidentiality protected channel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the chapter about consent is ok. I only have a comment about the security of the communication. It's better to just say "communication shall be carried out using a secure channel (e.g. TLS)". If we want to go into more details, we would also need authentication in the sense that the VISS server needs to be sure it's talking to the right ECF and viceversa.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update accordingly to your proposal.
Signed-off-by: Ulf Bjorkengren <[email protected]>
Signed-off-by: Ulf Bjorkengren <[email protected]>
Signed-off-by: Ulf Bjorkengren <[email protected]>
No description provided.