w3c / wot-thing-description

Web of Things (WoT) Thing Description
http://w3c.github.io/wot-thing-description/
Other
131 stars 63 forks source link

`name` and `in` fields for `BasicSecurityScheme` and `DigestSecurityScheme` needed? #1394

Open JKRhb opened 2 years ago

JKRhb commented 2 years ago

Dealing with the implementation of the defined SecuritySchemes, I noticed that the fields name and in for both theDigestSecurityScheme and BasicSecurityScheme seem to not correspond to RFC 7616 and RFC 7617 where only the inclusion in an HTTP header (whose name is supposed to be Authorization) is specified.

Is it intentional for the TD specification to deviate from the RFCs? In Open API, where this part of the specification seems to have originated from, these fields are not present, if I am not mistaken.

sebastiankb commented 2 years ago

from today's TD call:

j1y3p4rk commented 2 years ago

I am not sure which part is deviating RFCs. From my perspective, it's correct. If @JKRhb provides details (e.g., chapter number of RFCs, how it should be in your opinion) will help to understand better.

JKRhb commented 2 years ago

I am not sure which part is deviating RFCs. From my perspective, it's correct. If @JKRhb provides details (e.g., chapter number of RFCs, how it should be in your opinion) will help to understand better.

From my understanding, both RFCs only specify that you can include the credentials in an Authorization (or Proxy-Authorization) header field (also see RFC 7235, section 4.2). I couldn't find any information regarding alternative header names or the inclusion in a query, a cookie, the body etc. but I might be wrong.

I guess the solution for this issue would be to simply remove the name and the in field from the specification for these two SecuritySchemes or to explicitly state that these fields can be used to create a "custom" scheme that (probably) deviates from the two RFCs and the HTTP Authentication Framework in general. My assumption would be that this isn't really desirable, though.

j1y3p4rk commented 2 years ago

Thank you for the detailed info. Now I understand. Yes, somehow 'Authorization' field is missing for id/password. @mmccool could you please add this as a discussion item for the security call? thanks!

mmccool commented 2 years ago

Suggest we add a statement that "Usage of these fields MUST be in accordance with RFC-such-and-such". This should explicitly say "including overriding defaults if necessary". The problem seems to be additional possible values in schemes which are not allowed in certain protocols. In general, the values permitted in a scheme must match the protocols of the interactions with which they are associated.

We should, however, check if we make anything mandatory that is explicitly disallowed by a particular RFC.

In general, while we want to maintain backward compatibility if at all possible, fixing any security issues must take precedence. For TD 2.0 though we probably want to have a complete review and possible do-over.

mmccool commented 2 years ago

One issue is that to override the default for "in", we need a value appropriate for the scheme used. There is unfortunately no catch-all value. I propose we add "auto" meaning it should be automatic, depending on the protocol. Adding a new value should maintain backward compatibility. Unfortunately we can't make it the default, as this would break compatibility.

mmccool commented 2 years ago

Looked through the rest, and the other schemes where there are mandatory or with-default values are either related to a well-defined spec (e.g. OAuth) or are strings (with "e.g." allowing whatever, so we don't have to introduce a new normative value.).

mmccool commented 2 years ago

Actions:

PR to be done by @JKRhb

JKRhb commented 2 years ago

Another aspect related to this issue that I have just discovered is that https://github.com/w3c/wot-thing-description/pull/1032 actually introduced a uri value for all Security Schemes that currently support the in field. I think this probably wasn't desired?

egekorkan commented 2 years ago

@JKRhb it was meant only for the APIKey actually so that REST APIs like Hue can be described (/api/<username>/lights/<id> where username is the API Key obtained after onboarding)

mmccool commented 2 years ago

So right now, the spec allows multiple values for the "in" field for things like Basic (header, query, body, cookie, etc) BUT also cites the HTTP RFC defining BasicAuth, which does not allow use of this scheme for other protocols, namely CoAP. However, in general we haven't dealt with CoAP authentication properly (OSCORE, etc). For TD 2.0, we should revisit the entire organization and probably make security schemes protocol-specific, put them all in extensions (perhaps associated with the protocol binding), etc. But for now... we are in CR candidate phase and don't want to make major changes.

Also, the label "PR Available" was added (by me) for some reason but I don't see a PR that addresses this. @JKRhb if you know the PR can you add a reference to it here? In the meantime I am taking off the PR available label.

mmccool commented 2 years ago

I'd be a little more comfortable if the Basic and Digest schemes were updated to say something like "e.g." before the RFC reference (and maybe "for HTTP" after) to indicate that other similar schemes in other protocols might be used. That is, the RFC reference only applies to HTTP.

mmccool commented 1 year ago

When we reorganize the security schemes to move protocol-specific schemes to bindings we can clean this up. Really the Basic scheme for HTTP should always use the header specified in the standard and so an "in" field is not required (and should not be used, since redundant).

JKRhb commented 10 months ago

I think this issue is also rather related to bug fixing and the reorganization of the document structure than a use case.