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

Recommended Security #6

Open
mmccool opened this issue Aug 30, 2019 · 15 comments
Open

Recommended Security #6

mmccool opened this issue Aug 30, 2019 · 15 comments

Comments

@mmccool
Copy link
Contributor

mmccool commented Aug 30, 2019

In section 4.1.9.1 5.4, Recommended Practice for Security, it currently states

"When using the "no security" or "Basic Auth" security schemes it is strongly recommended to use transport layer encryption."

This should instead state, normatively:

"When using the "Basic Auth" or "Digest Auth" security schemes it is REQUIRED to use transport layer encryption. For all other schemes use of transport layer encryption is RECOMMENDED."

This is because Basic and Digest Auth are basically useless without transport encryption, they are designed to be used with it: they basically transmit passwords in the clear otherwise. Worse, using these without encryption gives people a false sense of confidence.

I will have to look at the other schemes, but in general we should say encryption is at least RECOMMENDED for all of them.

For "nosec" while you are not controlling access you might still want to control confidentiality with transport security, but on balance I think for this we can leave it up to the user; this mode should however be primarily used only for development. I think it's probably fine to lump it under RECOMMENDED.

Note however that using transport encryption with HTTP in systems not connected to the internet can be a bit troublesome, but oh, well.

@benfrancis
Copy link
Member

Adding a note here based on feedback on #78 that the final list of mandatory security schemes for Core Profile compliant Consumers should be reviewed based on implementation experience.

My view is that this list should be kept relatively short. I advocate for NoSecurityScheme and OAuth2SecurityScheme being on the list.

@benfrancis
Copy link
Member

See also: #220 and #221.

@mmccool
Copy link
Contributor Author

mmccool commented Feb 6, 2023

Note: I am fine with removing Digest. It's not any better than basic when TLS is used, basic is easier to implement, and without TLS digest is not secure, either.

@mmccool
Copy link
Contributor Author

mmccool commented Feb 13, 2023

Discussion in Security TF 2023.02.13:

  • It seems the doc was reorganized so we are assuming this issue is referring to section 5.4.
  • It seems Digest has been removed, and the Security TF is ok with that change.
  • List of assertions that MAY be supported by Things is redundant, since TDs already allow these. If, however, the intent is that these are the ONLY security schemes a Web Thing conformant to a profile CAN use, then the assertion should be worded as "A Web Thing MUST use one of the following...". This makes sense for interoperability.
  • Secure transport is not discussed. It is covered in Architecture, so the only question is whether it would be redundant to mention it here. Perhaps at least an informative note if it is covered in Architecture.
  • Probably want to further limit some of the options to target best practices, i.e. using Basic with the password embedded in the URL query is insecure and should be avoided. We should also limit "in" to be "header" (which is the default value, so it could/should just be omitted).
  • "Basic" means something specific for HTTP (i.e. putting it in header). Putting this under common constraints perhaps limits other protocols inappropriately. Suggest that we move the "Security" section under HTTP Basic and move it out of Common Constraints. In general suggest deferring generalization until we have more than one protocol to worry about.
  • For OAuth2, clarify that the standard should be followed strictly; should also bound to HTTP. Other schemes are used for other protocols, so also not appropriate to put under Common Constraints.

@mmccool
Copy link
Contributor Author

mmccool commented Feb 13, 2023

Actions (generation PR assigned to @lu-zero):

  1. Move section under HTTP Basic.
  2. Modify assertions to ONLY allow the specified schemes.
  3. Add assertions disallowing non-default values for "in" for Basic (e.g. mandating "header").
  4. Add an informative statement about secure transport (referencing the assertions in Architecture which should be sufficient).

@benfrancis
Copy link
Member

benfrancis commented Feb 13, 2023

Actions (generation PR assigned to @lu-zero):

  1. Move section under HTTP Basic.

Note that the HTTP SSE Profile and HTTP Webhook Profile do not depend on the HTTP Basic Profile, they can be used independently. That means that if you move the security assertions from Common Constraints to the HTTP Basic Profile, they will no longer apply to the other two profiles. You would instead have to duplicate the text in all three profiles or refer to the section of the HTTP Basic Profile, which then makes it another Common Constraints section.

Since this version of the specification only defines HTTP-based profiles, I see no need to make this change.

  1. Modify assertions to ONLY allow the specified schemes.

I suspect what you really mean is that a Web Thing MUST support at least one of these security schemes (including NoSecurityScheme). Restricting Web Things to only support the specified schemes prevents them from supporting other schemes in addition (which should be allowed), and may prevent a Thing from conforming to multiple profiles in the future.

  1. Add assertions disallowing non-default values for "in" for Basic (e.g. mandating "header").

Please be careful here. There are some browser APIs (notably WebSocket and EventSource) which don't support setting an Authorization header and which force Consumers to include credentials in the URL. As a minimum the HTTP SSE Profile needs to allow Bearer tokens to be provided in a query string for the OAuth2 scheme, but possibly something similar for the Basic scheme as well if you want that security scheme to be supported by that profile.

  1. Add an informative statement about secure transport (referencing the assertions in Architecture which should be sufficient).

The recommendation to use TLS where possible is already repeated in many places, but I don't have a problem with repeating/referencing it again here if people think that's necessary. Note there are already references in the Security Considerations section which reference the existing assertions (though doesn't mention that one specifically).

@lu-zero
Copy link
Contributor

lu-zero commented Feb 13, 2023

Note that the HTTP SSE Profile and HTTP Webhook Profile do not depend on the HTTP Basic Profile, they can be used independently. That means that if you move the security assertions from Common Constraints to the HTTP Basic Profile, they will no longer apply to the other two profiles. You would instead have to duplicate the text in all three profiles or refer to the section of the HTTP Basic Profile, which then makes it another Common Constraints section.

I added a HTTP Common constraints by splitting what should apply for any protocol and what is HTTP-specific, then I'd iterate to add a security section per profile and address your other points.

@benfrancis
Copy link
Member

@lu-zero wrote:

I added a HTTP Common constraints by splitting what should apply for any protocol and what is HTTP-specific, then I'd iterate to add a security section per profile and address your other points.

I feel like we're going round in circles here.

I argued for a long time that there should not be a Common Constraints section at all, because there are no assertions which we can be certain will apply to all future Profiles. I tried to include everything in a single HTTP profile instead. In the end we had to split the HTTP Profile into three separate profiles because we couldn't agree on a single eventing mechanism, so I agreed to have a Common Constraints section in which to include assertions which apply to all three HTTP profiles. This was originally nested inside an "HTTP Profiles" section but ended up being moved up to the top level to reduce unnecessary levels of nesting in the document.

Now you are suggesting creating a second Common Constraints section specifically for HTTP Profiles, even though the current specification only contains HTTP profiles. The current Common Constraints section is an HTTP Profiles Common Constraints section. We could rename it if you really think it's necessary, but it seems a bit pointless until such a time as there are non-HTTP profiles.

@lu-zero
Copy link
Contributor

lu-zero commented Feb 13, 2023

I assumed that the changes were made in order to be ready for non-http profiles (e.g. CoAP + OSCORE + ACE matching HTTPS + OAuth2), I wasn't aware of the past iterations.

I prepared the patchset so should be easy to drop the split if it is deemed unnecessary.

@mlagally
Copy link
Contributor

mlagally commented Feb 22, 2023

I think (#364) has been updated to take the discussion into account and should be merged.

@egekorkan
Copy link
Contributor

@mmccool just moving my comment here:

It is not here anymore but I think that each profile should explain the security scheme on its own. Is OAuth2 usable in Webhook?

@mmccool
Copy link
Contributor Author

mmccool commented Mar 6, 2023

Notes from Security TF meeting 6 March:

  • Current text still does not have limitations on use of "in" (should be limited to default, "header") and "name" (should be limited to default, "Authorization" (EDIT: actually, no default for name, see below!)). Suggest in the short term we at least add a PR that adds an assertion: 'The "basic" scheme MUST use the default values for "in" and "name" of "header" and "Authorization" as defined in [[wot-thing-description11]]"'. Note that "basic" is the only scheme that needs this clarification. @mmccool will do a PR. Note: default values are defined in section 5.4 of the TD spec, but don't suggest we do an actual section reference. "Use" means you can put them in or leave them out, but in the end only the default values are allowed.
  • Let's focus on cleaning up the internal content of the section first, later on we can discuss whether or not it belongs inside a specific profile. As Ben points out, since we only do HTTP in this document right now it doesn't matter, but in the long run we WILL have to restructure things and have a security section for each profile. Also, as Ege points out, we may have to support different options for the Web Hook profile.
  • As for WebHooks, see Security Requirements for WebHook Consumer #222 and the recommendations there. A similar but possibly different set of security schemes may be applicable. In particular we might want to use "bearer" instead of "oauth2". But I suggest we follow up on that in the above issue.

@mmccool
Copy link
Contributor Author

mmccool commented Mar 6, 2023

Hmmm... one technical issue I noted is that "name" does not actually have a default. So technically you MUST include it with an appropriate value whenever you use "basic". Lots of examples using "basic" leave it out, since it's optional, but technically that means it is undefined. It really should have been given a default value of "Authorization" in the TD spec... but it's too late now. But I'll tweak the above assertion to say that "name" MUST be included with the value "Authorization".

@mmccool
Copy link
Contributor Author

mmccool commented Mar 6, 2023

Made a PR: #374

I addition to the issue I noted about about the lack of default value for "name", I also realized that in HTTP the header name has two possible values, based on whether or not a "proxy" value is provided. So in the end my PR adds three assertions: one for "in", and two for "name" (for two different cases: without proxy, and with proxy).

@mmccool
Copy link
Contributor Author

mmccool commented Oct 2, 2023

Suspect a lot of the above will be resolved once we reorganize security schemes to only be applicable/defined in particular bindings. So should wait for that to be done for HTTP at least, then restrict from there. For TD 2.0 I also suspect we will get rid of the unnecessary "in" definition for Basic, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants