Security http sections #364
Conversation
|
@benfrancis I hope I addressed your feedback. |
|
I'm afraid I don't agree with this change. Please see #6 (comment) I don't think any of the assertions in Common Constraints can be assumed to be applicable to all future profiles, only the current set of HTTP profiles. The current Common Constraints section is an HTTP Common Constraints section. I also don't see why some of the constraints you've moved to the separate HTTP Common Constraints section (e.g. Links) could not apply to other protocols in the future like CoAP. Splitting assertions into Common Constraints and HTTP Common Constraints implies that the former apply to profiles using multiple protocols, which I don't think we can assert at this point. I suggest leaving this as it is and re-assessing at such a point as we add non-HTTP Profiles. Only then will a separate section be necessary. |
|
60ef6cc can be omitted from the patchset without additional changes, is the rest fine for you? |
lu-zero
force-pushed
the
security-http-sections
branch
from
48ef253
to
dd1d43a
Compare
index.html
Outdated
| <p><span class="rfc2119-assertion" id="common-constraints-security-3"> | ||
| A Thing MAY implement multiple security schemes.</span> | ||
| A Thing MAY implement multiple security schemes, at least one of the above MUST be supported.</span> |
There was a problem hiding this comment.
As I understand it the tooling for extracting assertions doesn't support multiple RFC 2119 key words in a single assertion. You probably need to split this into two separate assertions with their own id.
index.html
Outdated
| The <a href="https://w3c.github.io/wot-thing-description/#basicsecurityscheme"> | ||
| <code>BasicSecurityScheme</code> | ||
| </a> SHOULD be avoided if the underlying transport is not | ||
| <a href="https://w3c.github.io/wot-architecture/#sec-security-consideration-secure-transport">secure</a>. |
There was a problem hiding this comment.
Whilst I don't disagree with this assertion (where it's possible to use TLS), I'm not sure this advice is unique to the Basic security scheme. A man-in-the-middle attack can be used to intercept tokens in the OAuth2 security scheme too.
Note that the Security Considerations section already says:
The security considerations of the WoT Architecture and WoT Thing Description MUST be adopted by compliant implementations.
This includes WoT Architecture's section on secure transport which you linked to, which goes into more detail and mentions that tokens are at risk as well as passwords.
Also note that assertions need to be wrapped in a <div> or a <span> with class="rfc2119-assertion" and a unique id.
There was a problem hiding this comment.
Some comments:
- We can change to MUST in the profile ?
- TLS should be for all security mechanisms indeed (except nosec).
There was a problem hiding this comment.
We can change to MUST in the profile ?
Regular reminder: TLS doesn't work on local networks. This would effectively forbid the use of profiles on local networks.
Use case: WebThings Gateway currently exposes its API via http://gateway.local, and optionally https://{subdomain}.webthings.io, and uses profiles on both.
|
Let's discuss this further in the Security call next week. I would like to clean up this PR by next week's Profile call. @benfrancis thanks for your review, please look for an update next week that I hope will address your concerns (may be a new PR, we'll see if this one can be repaired first). |
lu-zero
force-pushed
the
security-http-sections
branch
from
dd1d43a
to
8b72f9e
Compare
|
It is not here anymore but I think that each profile should explain the security scheme on its own. Is OAuth2 usable in Webhook? |
|
Hopefully now it is more acceptable. |
index.html
Outdated
| <p><span class="rfc2119-assertion" id="common-constraints-security-3"> | ||
| A Thing MAY implement multiple security schemes.</span> | ||
| A Thing MAY implement multiple security schemes</span>, | ||
| <span class="rfc2119-assertion" id="common-constraints-security-4"> at least one of the above MUST be supported.</span> |
There was a problem hiding this comment.
I suggest splitting this into two sentences. Once these assertions get extracted out into implementation reports the latter is not going to make any sense.
There was a problem hiding this comment.
I agree in principle to improve the language, however splitting into two sentences does not solve the problem. The 2nd sentence should contain the members of the list above.
However, since this MR does not contain any contentious content anymore, I suggest we be oragmatic, merge it and do the split into two sentences in another PR.
|
@egekorkan wrote:
That's a good question, I don't know. |
lu-zero
force-pushed
the
security-http-sections
branch
from
8b72f9e
to
5bbd014
Compare
| @@ -643,12 +643,15 @@ <h2>Security</h2> | |||
| </ul> | |||
| </div> | |||
| <p><span class="rfc2119-assertion" id="common-constraints-security-2"> | |||
| Conformant Consumers MUST support all of these security schemes.</span> | |||
| Conformant Consumers MUST support at least all of these security schemes.</span> | |||
There was a problem hiding this comment.
Do you mean...
Consumers MUST support at least one of these security schemes?
IF not.. "at least all" sounds strange, then I would rather stick to "all"
There was a problem hiding this comment.
Consumers must support all of the security schemes in order to guarantee interoperability with all conformant Things.
I agree the existing wording made more sense.
There was a problem hiding this comment.
@danielpeintner
We discussed it in today's call and agreed to merge it. If there's a need to improve the text, please create a new issue.
| @@ -643,12 +643,15 @@ <h2>Security</h2> | |||
| </ul> | |||
| </div> | |||
| <p><span class="rfc2119-assertion" id="common-constraints-security-2"> | |||
| Conformant Consumers MUST support all of these security schemes.</span> | |||
| Conformant Consumers MUST support at least all of these security schemes.</span> | |||
There was a problem hiding this comment.
Consumers must support all of the security schemes in order to guarantee interoperability with all conformant Things.
I agree the existing wording made more sense.
| </p> | ||
|
|
||
| <p><span class="rfc2119-assertion" id="common-constraints-security-3"> | ||
| A Thing MAY implement multiple security schemes.</span> | ||
| </p> | ||
| <p><span class="rfc2119-assertion" id="common-constraints-security-4"> | ||
| A Thing MUST support at least one of the above security schemes.</span> |
There was a problem hiding this comment.
I'm sorry for asking to keep asking for further changes but there are now two similar sounding assertions which say different Things:
Below is a list of security schemes which conformant Web Things MAY use
A Thing MUST support at least one of the above security schemes.
The text originally said "Below is a list of security schemes which conformant Web Things MAY use and conformant Consumers MUST support" but I believe it was split into two assertions because sentences can't include two RFC 2119 keywords.
The former assertion also uses the term "MAY" incorrectly, which is my fault.
Let's just stop dancing around grammatical structures and duplicate the list 🙂...
Conformant Web Things MUST use at least one of the following security schemes [[wot-thing-description]]:
- NoSecurityScheme
- BasicSecurityScheme
- OAuth2SecurityScheme with the code or client flow.
Conformant Consumers MUST support all of the following security schemes [[wot-thing-description]]:
- NoSecurityScheme
- BasicSecurityScheme
- OAuth2SecurityScheme with the code or client flow.
Initial wording for #6.
Preview | Diff