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

WIP: Simplified inline security definitions #945

Closed

Conversation

mmccool
Copy link
Contributor

@mmccool mmccool commented Aug 11, 2020

Following the discussion in #901, this PR permits anonymous SecurityScheme objects to be given directly in "security" fields and makes "securityDefinitions" optional. This considerably simplifies simple cases, i.e. when one security scheme applies to all forms. For example, if only "basic" security is needed, it can be defined very simply as follows:

"security": { "scheme": "basic" }

Then the more complex named definitions only need to be used for complex use cases.

One downside of this is that "security" is an array that may have either strings or objects in it. This is not ambiguous or illegal but is slightly harder to parse, and one of the suggested rules for profiles is to disallow arrays like this. However note that conceptually named definitions are substituted by the corresponding objects so in the end, internally, "security" can just be treated as an array of SecurityScheme objects.

To do: ontology, fix/provide examples, json schema


Preview | Diff

@mmccool mmccool marked this pull request as draft August 11, 2020 20:34
@mmccool mmccool changed the title Simplified inline security definitions WIP: Simplified inline security definitions Aug 11, 2020
@mmccool mmccool changed the title WIP: Simplified inline security definitions Simplified inline security definitions Aug 24, 2020
@mmccool mmccool marked this pull request as ready for review August 24, 2020 12:59
Copy link
Contributor

@egekorkan egekorkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see the td-vocab-security--Thing split to two assertions since we will notice that during testing, there will need to be two child assertions to test that.

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from @egekorkan 's comment and the little change below, I'd give a full review of the document itself. This new change touch also other points that might be updated. For example (introduction after the example):

This example also specifies the basic security scheme, requiring a username and password for access. Note that a security scheme is first given a name in securityDefinitions and then activated by specifying that name in a security section.

The underlined section is still true, however it can be relaxed.

6.3.4 securityDefinitions and security

A named security configuration must be given in the securityDefinitions map. That definition must be activated by including its JSON name in the security member, which can be of type string when only one definition is activated.

Same as above, the security configuration can now be expressed directly inside the security field.

Example 12 and the explainer text can be improved by discarding the securityDefinition field and declare no_sec scheme inside the security property :

{
    "@context": "https://www.w3.org/2019/wot/td/v1",
    "id": "urn:dev:ops:32473-Thing-1234",
    "title": "MyThing",
    "description": "Human readable information.",
    "support": "https://servient.example.com/contact",
    "security": { "scheme": "nosec" },
    "properties": {...},
    "actions": {...},
    "events": {...},
    "links": [...]
}

index.html Outdated
<tr class="rfc2119-table-assertion" id="td-vocab-security--Thing"><td><code>security</code></td><td>Set of security definition names, chosen from those defined in <code>securityDefinitions</code>. These must all be satisfied for access to resources.</td><td>mandatory</td><td><a href="http://www.w3.org/TR/2012/REC-xmlschema11-2-20120405/#string"><code>string</code></a> or <a>Array</a> of <a href="http://www.w3.org/TR/2012/REC-xmlschema11-2-20120405/#string"><code>string</code></a></td></tr>
<tr class="rfc2119-table-assertion" id="td-vocab-securityDefinitions--Thing"><td><code>securityDefinitions</code></td><td>Set of named security configurations (definitions only). Not actually applied unless names are used in a <code>security</code> name-value pair.</td><td>mandatory</td><td><a>Map</a> of <a href="#securityscheme"><code>SecurityScheme</code></a></td></tr></tbody></table>
<tr class="rfc2119-table-assertion" id="td-vocab-security--Thing"><td><code>security</code></td><td>Set of security definition names, chosen from those defined in <code>securityDefinitions</code>, or inline <code>SecurityScheme</code> objects, which are to be treated like anonymous security definitions. These must all be satisfied for access to resources.</td><td>mandatory</td><td><a href="http://www.w3.org/TR/2012/REC-xmlschema11-2-20120405/#string"><code>string</code></a> or <code>SecurityScheme</code> or <a>Array</a> of <a href="http://www.w3.org/TR/2012/REC-xmlschema11-2-20120405/#string"><code>string</code></a> or <code>SecurityScheme</code></td></tr>
<tr class="rfc2119-table-assertion" id="td-vocab-securityDefinitions--Thing"><td><code>securityDefinitions</code></td><td>Set of named security configurations (definitions only). Not actually applied unless names are used in a <code>security</code> value.</td><td>optional</td><td><a>Map</a> of <a href="#securityscheme"><code>SecurityScheme</code></a></td></tr></tbody></table>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd change this sentence: Not actually applied unless names are used in a <code>security</code> value to Not actually applied unless names are used as values in the <code>security</code> field.

@takuki
Copy link
Contributor

takuki commented Sep 2, 2020

In Sept 2 TD call, it was pointed out we need to test this in PlugFest with proxy implementations.
@mmccool will try to address comment from @egekorkan about assertion.

@danielpeintner
Copy link
Contributor

For example, if only "basic" security is needed, it can be defined very simply as follows:

"security": { "scheme": "basic" }

Then the more complex named definitions only need to be used for complex use cases.

I am wondering a bit why we shouldn't use a more straight forward approach without splitting security and securityDefinitions (given this is somehow a breaking change already).

I am just thinking/writing loud what I would do in retrospect

"security": { "basic_sc" : {"scheme": "basic", "in":"header"} }

Doing so allows:

  • doing inline security definitions
  • requiring a name such as "basic_sc" still allows referencing a given security scheme from forms
  • get rid of the problem that JSON schema cannot validate that a reference such as "basic_sc" is present in "securityDefinitions" (see example below and comment from Cristiano)
   "securityDefinitions": {
        "basic_sc": {"scheme": "basic", "in":"header"}
    },
    "security": ["basic_sc"],
  • get rid of "securityDefinitions" overall

I might miss something important...

@egekorkan
Copy link
Contributor

Regarding the previous comment from @danielpeintner after talking with him, I have the following remarks:

  1. If we do it like this but want to put two security definitions, how would we reference them in other layers like forms and interactions? We would need to say that if security term is an array or string, it is applying the terms provided, if it is an object of one key, it applies the terms provided, if it is an object of 2 or more keys, it is also describing possible ones.
  2. If there are two keys in the object, which would apply as the base security?

After thinking for a while, most TDs will be single security scheme. We should make sure that this is easy and straightforward to describe. If a TD has more security schemes needed, then what we have or what @mmccool is proposing should be used. The simple case should not be made complicated for the sake of supporting complex cases, which is what we are doing with the current spec.

@sebastiankb sebastiankb marked this pull request as draft September 9, 2020 15:51
@sebastiankb
Copy link
Contributor

@mmccool I would be good to convert this PR to a draft as long we need validation that there are implementations

@sebastiankb
Copy link
Contributor

from today's TD call:

  • no ready to merge
  • maybe ready for next week.
  • also need discussion if it is required for TD 1.1, or more related for TD 2.0?
  • there also need some consideration about backwards compatibility
  • We should wait if there enough implementations

@relu91
Copy link
Member

relu91 commented Sep 29, 2020

also need discussion if it is required for TD 1.1, or more related for TD 2.0?

I see this PR as a breaking change. Clients able to digest TD 1.0 will not be able to even parse a TD with this new feature. For example, a Java parse which expects that the security property can be only a string or an Array will throw a syntax error if it finds that the value of the security property is actually an Object. Although, some Javascript clients might still work since their are more permissive.

On the contrary, the combination scheme (PR #944) will not cause any parsing problem, since it is just another SecurityDefinition. However, TD 1.0 clients will not be able to use combination security and they might fail after JSON parsing or later (i.e. when trying to access an affordance). I think we can accept this level of incompatibility when moving to a minor version.

So my proposal is:

  • combination scheme TD 1.1
  • inline security TD 2.0 (sadly because it is a really nice feature to have)

Base automatically changed from master to main February 24, 2021 15:22
@mmccool mmccool mentioned this pull request May 28, 2021
11 tasks
@mmccool
Copy link
Contributor Author

mmccool commented Sep 22, 2021

So this has recently been revived since it would be useful to have for signing. To update with the above:

  • Need to rebase/fix up various other references above
  • I would like to strictly separate definitions and usages; it also keeps the simple case simple, and also enables "combo" expressions, etc.
  • We of course need to resolve the compatibility/versioning issue
  • Still need to deal with the open items in the description, e.g. updating the ontology and JSON schema, etc.

@mmccool mmccool changed the title Simplified inline security definitions WIP: Simplified inline security definitions Sep 22, 2021
@mmccool
Copy link
Contributor Author

mmccool commented Sep 22, 2021

So, I rebased, but now it seems like the changes I did make got clobbered by the render script. Anyway, I will have to make the changes in the ontology anyway. So... not ready to merge. In fact not much here any more, I will have to redo things.

@egekorkan
Copy link
Contributor

We will close this but it is clearly referenced at https://github.com/w3c/wot/blob/main/planning/ThingDescription/td-next-work-items/usability-and-design.md#inline-security . Can be reopened if needed but probably a new discussion should be done on how to incorporate this.

@egekorkan egekorkan closed this Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants