[OAUTH-WG] review draft-ietf-oauth-security-topics-13 [1/3]

[Hans Zandbelt <[e-mail]>]

Hi,

Please find my feedback on the first 10 pages below.

Hans.

Overall:
- grammar in the first sections: there's a lot of comma-separated sentences
that could/should be reworked by a native speaker
- perhaps readers guidance pointing developers straight to section 3. as
Torsten said on the call would be useful
- as also discussed on the call, perhaps it would be good to spell out how
this BCP relates to the original OAuth 2.0 security considerations in the
RFC(s)

P3 1. Introduction
Usage of OAuth vs. OAuth 2.0 across the document: perhaps explain that in
the remainder OAuth refers to OAuth 2.0.

First bullet:
- CSRF and referrer header are being mentioned without explaining the
abbreviation, context or reference. And btw, it is the “referer” header…
- “defense in depth” -> “in depth defense” ?
Third bullet:
- I believe those higher security use cases were considered from the start,
but they were not acted upon before (e.g. MAC spec was left abandoned).

P4 first paragraph, in the middle:
“This way the same..” -> “In this way the same…”
“serves as a frontend…” -> “serve as a frontend…”
“in a multi-tenancy.” -> “in multi-tenant environment.”

In general there are a lot of sentences broken up by comma’s that could be
rearranged and do without the comma’s.

1.1 the whole section is comprised of comma-split sentences that could be
re-arranged.

2.
In general: shouldn’t the OAuth 2.0 terms be capitalized everywhere e.g.
resource owner -> Resource Owner
authorization server -> Authorization Server
Etc. ?

- “was laid out that described…” -> “was laid out that describes…”
- “at an authorization server (AS)…” -> “at the authorization server (AS)…”

P5:
A1:
- first sentence ends the list with “etc.” but I don’t feel an enumeration
of attacks can be left up to the reader to complete.
- “be achieved through many ways” -> “be achieved in many ways”

A3 and A4 could very conveniently be bundled together, isn’t it, by
specifying “read but not write the authorization request and/or response”.

P6: 2.
The last paragraph of 2. is hand-wavy, almost scaring the implementor but
not giving him the guidance to act properly. I’m not sure this helps,
especially the usage of MUST will make implementers feel uncomfortable
because it does not tell them exactly what they MUST do. Something like “an
implementer SHOULD always consider other attack vectors specific to their
environment” may work better?

3.1
End of first paragraph “It also helps to detect mix-up attacks.” lacks an
explanation or a reference to a section where this is explained. As a
matter of fact I don’t immediately get why this would help to mitigate
mix-up attacks.

P7:
2nd paragraph: I don’t think [I-D.bradley-oauth-jwt-encoded-state] is going
anywhere and – even as a co-author - I’m not sure the reference should be
included here.

Last paragraph of 3.1:
“AS which redirect a request…” -> “ASes which redirect a request…” or “An
AS that redirects a request…”

P8:
3rd paragraph: I guess it was discussed in length but I would like to see
“clients SHOULD NOT use the implicit grant” change into “clients MUST NOT
use the implicit grant”, as done for ROPC further down. This is a security
BCP and implementers complying with it must really not support Implicit any
longer IMHO. It is still an option for implementers to not follow this BCP
and “just” do OAuth 2.0… or implement sender constrained tokens. The SHOULD
gives people a way out and an excuse that we should no longer allow.

P9:
3.3 missing text about not using access tokens for user authentication
(again?) plus reference to oauth.net/articles/authentication, A thing I
still often see in deployments and still a source of confusion for
developers and deployers

-- 
[e-mail]
ZmartZone IAM - www.zmartzone.eu