Skip to content
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

Security Metadata #144

Merged
merged 24 commits into from
Jun 7, 2018
Merged

Security Metadata #144

merged 24 commits into from
Jun 7, 2018

Conversation

mmccool
Copy link
Contributor

@mmccool mmccool commented May 31, 2018

Updates to TD information model to reflect full security metadata model.

@mmccool
Copy link
Contributor Author

mmccool commented May 31, 2018

Work in progress. I will note when the updated security information model is ready for review in these comments. My goal is to have a final draft ready for review by Friday, 2018-05-31. However I'm creating the PR now to let people know I'm working on this so we can hopefully avoid collisions and conflicts.

@mmccool
Copy link
Contributor Author

mmccool commented May 31, 2018

This version is mostly complete (it can be considered a first draft), but has a few problems still:

  1. Master TD UML figure has not been updated yet to reference the new Security class
  2. OAuth2 "scope" object has not been defined (using a "string" as a placeholder but it's actually an array of objects)
  3. Both securityDefinitions and security are defined
  4. I added "security" fields to all interactions, not just the forms and the top level.

For 3 and 4 I will add issues so we can discuss them in a structured fashion; they are design choices/tradeoffs. 1 and 2 I just need to finish. I will do both before the next TD TF meeting.

@mmccool
Copy link
Contributor Author

mmccool commented Jun 1, 2018

Issue 1 mentioned above, update to the core diagram, has been resolved. The diagram should now reflect the actual class structure. Note this was done AFTER I remove the securityDefinitions (see issue 145). So issue 3 is also resolved: there is now only "security", although it can be used at multiple levels (Thing, Interaction, Form; also, in Links.

The main superclass of all security scheme configurations is called "SecurityScheme" to be consistent with the old class structure, however, which will make it a little easier to add securityDefinitions back in later if we want them.

There are some red lines in the core diagram: these are additional locations where "security" definitions might be useful. The extra security fields in interactions support the case where security configurations are shared among multiple forms in an interaction (for example, several IP addresses or domain names for the same physical device). The extra security field in links supports security on external resources or "extra" access points on the Thing itself (eg the stream resource on an IP camera). The current ontology includes definitions supporting these additional uses.

@mmccool
Copy link
Contributor Author

mmccool commented Jun 1, 2018

Took out "security" in link; "scope" now defined appropriately (turns out a string is the right type, BUT it also has to appear in interactions...).

@mmccool
Copy link
Contributor Author

mmccool commented Jun 1, 2018

Issues identified during TD TF meeting resolved: "scheme" being 1..1 (mandatory) in figure; no "security" tag on links; missing OCFSecurityScheme section; invalid securityDefinitions example.
Added better "security" examples showing overriding (different security configs at different levels) and differentiation (different security configs at the same level, use case being different protocols).

Now ready for review. Please provide comments and I will fix issues on Monday, then we will review again in the next Security TF call and make final edits.

@sebastiankb
Copy link
Contributor

Many thanks.

Here is a link to see the new security inputs.

Two comments from my side:

  • I'm not sure if OCFSecurityScheme class should be part of the core ontology. Rather, I would like to see a guideline how individual security requirements such as from domain applications (e.g., OCF) can be extended within the TD security concept.
  • swap section of 5.4 with 5.3. (needs update of the render.js script)

@mmccool
Copy link
Contributor Author

mmccool commented Jun 4, 2018

Thanks for the feedback.

Regarding switching section 5.3 and 5.4: sure, no problem, I will make that change.

Regarding OCF: I share a more general concern, which is how future users can specify security schemes that are not part of the core ontology. It is likely that new security schemes (or new versions of existing schemes) will arise faster than the WoT standard can be updated; at the very least there will be an undesirable gap if users have to wait for an update to the WoT standard.

Presumably a custom vocabulary can be used to address this, but this assumption needs to be tested, and as you say, some guidelines issued. Regarding OCF in particular, it does seem odd to include it but not other, similar schemes such as those for LWM2M, ECHONET, etc. OCF’s security is also a specialization of the IETF standard for CoAP which, conversely, is not included (and probably should be...).

At issue is what criteria we should apply to decide which security schemes go into the core vocabulary and which should not. It seems logical to include schemes supported natively by the core protocols, eg HTTP, CoAP, and MQTT. Slightly more controversial are schemes like OAuth and tokens (and there is a related issue regarding whether those should be separate, since tokens are in fact used by OAuth; including “bearer” and “pop” is meant to cover approaches where access tokens are provided by some mechanism other than OAuth).

We will discuss this further in the Security TF call, but I propose doing the following:

  • Taking OCF out of the core vocabulary... but using it as a test case for “extended” security vocabularies. I will add a discussion of the use of extension vocabularies in the spec as well.
  • Leaving oauth2, bearer, and pop in the core for now but discussing whether they should also be taken out and put into separate vocabularies
  • Looking into specifying schemes for IETF-standard CoAP security; putting this into the core would be consistent with including HTTP’s basic and digest schemes in the core.
  • Ditto for MQTT security (assuming we plan to consider MQTT a “core protocol”)

@mmccool
Copy link
Contributor Author

mmccool commented Jun 4, 2018

The security TF met and suggested two changes:

  1. Extend last example to show "OR" of two security configurations for the same protocol
  2. Fix reference to Security and Privacy Considerations section.
    These have now been done, and based on the Security TF review, I can now recommend that this be merged.

@mmccool
Copy link
Contributor Author

mmccool commented Jun 5, 2018

Please hold off on merging. I need to add legal/default values to some of the items.

@mmccool
Copy link
Contributor Author

mmccool commented Jun 5, 2018

List of valid values now given for security parameters, as well as default values. I also expanded the explanations where appropriate. Ready for merging.

@mmccool
Copy link
Contributor Author

mmccool commented Jun 6, 2018

While I was ok with the last update, and felt it could have been merged, I noticed a few additional problems that I decided to fix:

  1. "form"->"forms" and "name"->"label" in various examples (including existing ones...)
  2. Missing security parameters. In particular, I added "pname" for use with the "query" value for "in", which is especially important for the apikey scheme. This corresponds to the "name" parameter used in OpenAPI but I decided not to use "name" to avoid a possible conflict/confusion.
  3. I also added "in" and "pname" to various other security configurations, such as bearer and pop tokens, to allow more flexibility (eg putting a token in a query parameter rather than in a header). One issue: some schemes may have more than one parameter, e.g. "basic" implies two, username and password. To discuss, but there are various ways around this. Note: it is also possible to use "in = header" and then use pname to specify an alternative header location for credentials.
  4. "scope" -> "scopes" to be consistent with the convention that values allowing array parameters should be plural.
  5. "parameter" -> "query" as a value for "in" for better consistency with OpenAPI (although there are still various other inconsistencies)

Note: "security" is also array-valued but I'm just going to leave it, as "security" can be considered a mass noun and so does not have a plural.

Ready for merging.

@mmccool
Copy link
Contributor Author

mmccool commented Jun 6, 2018

Reverted name->label change in examples at Thing level. Looking at the ontology it seems Things still have names, not labels. I thought I was just fixing a consistency issue but apparently not, and changing this does not belong in a security metadata PR, so left it... if it is to be changed, it should be changed in some other PR.

(Even more) ready to be merged.

@mmccool mmccool mentioned this pull request Jun 6, 2018
@mmccool
Copy link
Contributor Author

mmccool commented Jun 6, 2018

Request from Matthias: add pointer to OpenAPI 3.0.1 spec in an editor box for comparison: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.1.md#securitySchemeObject

@mmccool
Copy link
Contributor Author

mmccool commented Jun 7, 2018

Requested ednote added. Ready to merge.

@sebastiankb
Copy link
Contributor

Thank you for the updates.

@sebastiankb sebastiankb merged commit bc89060 into w3c:master Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants