Hi all,
Sorry for having been quite slow with this, but I had a bunch of travel recently. Anyway, my AD comments on -22 are attached. I think that the first list has the ones that need some change before we push this out for IETF LC, there might or might not be something to change as a result of the 2nd list of questions and the rest are really nits can be handled either now or later. Thanks for all your work on this so far - its nearly there IMO and we should be able to get the IETF LC started once these few things are dealt with. Cheers, S.
List 1 - Fairly sure these need changes: ---------------------------------------- (1) In 2.3.1 MUST the AS support both HTTP basic and the client_id and client_secret in the request schemes? You say it MAY "allow" credentials in the request body, but that's different from what the AS coder has to implement. Saying an AS that issues passwords MUST support "basic over TLS" and MAY support including credentials in the body would seem right here. (2) In 2.3.1 (and more generally) what does "MUST require the use of a transport-layer security mechanism" really mean? I think you need to say explicitly what this means in terms of TLS and which version of TLS and which ciphersuites etc. Doing that once is fine and you could then have a defined phrase for it, but it needs to be specified for each place where TLS is used. The text at the end of p15 is almost exactly what's needed, and would be if you said which ciphersuites are MUSTs. I think you might need to pick ciphersuites for each version if you want some combination of tls1.0 and tls1.2 and need server auth at least. (3) Having a MUST for TLS1.0 and a SHOULD for TLS1.2 is going to generate a DISCUSS or two. I think you really need to justify that in the Document and PROTO write up if you want to stick with the current choices. I personally would prefer if those were swapped myself, that is have a MUST for the latest version of TLS (TLS1.2) and a MAY/SHOULD for TLS 1.0. In addition to taking care of process concerns, there is also the recent TLS chosen plaintext attack where TLS1.2 has no problem but TLS1.0 does. (This differs from (2) above, since the former is almost editorial, but I guess this one needs to go to the WG.) (4) The response_type description in 3.1.1 is unclear. You say it MUST be "one of" various values, but then that it can be a space separated list of values. When >1 value is provided, it doesn't say what that means, it only says that the order is irrelevant. (It could mean "any of these" or "all of these" for example, both are order independent, but are not the same.) I suggest adding a sentence to say that "code token" means "please send both" if that's what it means. (5) How does a client implement the MUST in the last sentence of 3.1.2.3? I don't see anything here or in 10.15 that says how to do this. I guess ideally, you'd just need a reference to somewhere else in the doc or to something else, but I do think you need something since you've made this a "MUST ensure" rule for clients. Adding a sentence/short paragraph here or in 10.15 with some typical/good ways to ensure this would be fine. (6) In 7.1 what does it mean to say that the client MUST NOT use the access token if it doesn't trust the token type? I don't know what code I'd write there in a client. Maybe s/trust/support/ or s/trust/understand/ would fix this. (7) Doesn't 7.1 need to say which token types are MTI so that we get interop? I think I'd like to see mac being a MUST and bearer being a MAY but regardless of my preference, I don't think you can be silent on this. One way to do this would be to mandate that the client MUST support mac and MAY support bearer but to allow the server to choose which token types they support. And as a consequence one or both of the mac/bearer drafts need to end up as normative I think. (8) 10.3 says access tokens MUST be kept confidential in transit. Does that not mean that non-anonymous TLS is a MUST? If so, then saying that clearly here would be good. If not, then saying what's really meant clearly is needed. (Same point for refresh tokens in 10.4.) (9) 10.5 says "effort should be made" to keep codes confidential, but I expect that'll generate AD comments - that's just a bit too vague - what do you really mean there? (10) 10.10 has an impossible requirement - you cannot stop/prevent attackers guessing, but you can ensure that such guessing is futile. Can you not be a bit more specific about a "reasonable" level of entropy here? I'd say 10 bits is not enough, 128 is, and there may be a good level to at least RECOMMEND (e.g. maybe >N bits if rate limiting can effectively prevent 2^(N/2) guesses going undetected? I'm not recommending an "N" myself here, but rather that the WG consider picking one or else justify not picking one. (The same comment applies to the term "non-guessable" as used in 10.12 and maybe elsewhere as well.) (11) Section 11 says a couple of times that the apps ADs are the appeal chain - shouldn't that be the SEC ADs now? List 2 - Questions: (not sure whether change needed or not) ---------- (1) p12 - 2.1 says that the client developer declares the type of the client, but then says that the definition is up to the authorization server really, so, in principle one client might have different types for different authorization servers. I think you could make this clearer by adding a sentence saying that one client could have >1 type according to different authorization servers or that the client type could change over time, e.g as some vulnerability in an OS gets discovered or if a new OS feature is added. I'm not sure if there's any action that could/should be taken if the client type changes, perhaps re-registration with a SHOULD for using a new redirection URL and invalidating all outstanding tokens or something? Maybe this sounds like something that some other document (and RFC or not) can tackle later. (2) 3.1 - I think you may need more about how passwords are handled, esp. when they contain odd characters. Is there really no problem with x-www-form-urlencoded when such a password is in a form? Have you checked with someone who really cares about such things? (3) Generally, the specificaion is a bit unclear about what each component MUST do, its hard to figure that out with the style of describing the 2119 rules on a per endpoint basis. Not sure what to suggest that'd not be loads of work, but I wondered if there's evidence that someone not involved in the WG or Oauth1 would find it easy or very hard to implement and get interop? I know that there've been some people who've implemented from the drafts and I'm told some of those were not involved in the WG at all. If you know of any such implementers, it'd be good to note that in the PROTO write-up since it'd answer this question, which I'd expect to be posed by some AD. (4) Is it clear that a client always knows when a redirection request will result in sensitve data being sent, thus triggering the SHOULD for use of TLS in 3.1.2.1? It may well be the case but it wasn't clear to me from reading whether I could write code that'd know when it's ok to not use TLS. (5) 3.1.2.2 says ASes MAY support >1 redirection URI. Why not make that a MUST? If a client needed it, then it couldn't interop with an AS that only supports 1, and it ought be easy for an AS to support >1 I guess. (6) What's the case where the AS is ok to redirect to an unregistered or untrusted URI that corresponds to the 2nd para of 3.1.2.4? (7) Can a client really ensure that other scripts don't go before its own as required in 3.1.2.5? (8) What is the impact of the "MUST be taken into consideration" at the end of 3.2.1? I can't see how to implement that nor is it clear what checks an AS could do at registration time. (9) Does the recent list discussion about scope encoding require any changes to 3.3? (10) Is there anything that needs saying if the AS ignores the client's requested scope but doesn't indicate that in the response? Put another way - why is that SHOULD not a MUST? (Just asking that you explain, not change.) (11) State and scope variables probably should not contain anything sensitive for the client or user, since the AS and UA both get to see them, is that stated somewhere? (Maybe with a hint that if you need something sensitive in the state, then just generate a symmetric key and encrypt it for yourself.) An example of what not to include would be a banking client including a user's bank a/c number as the state variable. (12) 4.4.3 says a refresh token SHOULD NOT be included. Seems like the AS actions for good requests are fairly variable, which is likely to cause developers to do the wrong thing - is there no way to make the AS actions more consistent? (Could be that they are consistent though, but I'm just getting lost in the text;-) (13) 10.9 says that the client MUST verify the server's cert which is fine. However, does that need a reference to e.g. rfc 6125? Also, do you want to be explicit here about the TLS server cert and thereby possibly rule out using DANE with the non PKI options that that WG (may) produce? Suggested non-trivial clarifications: ------------------------------------- (1) 1.3.4 - "previously arranged" might trigger discusses on the document since it implies that this spec might not be suited for broad use. I think that making it clear that the normal mode for client developers is to work against an existing service (AS and resource server) would help to clarify that such arrangements are ok here. (2) p11, in step (F) is there a way to distinguish between an access token that is invalid due to expiry vs. e.g. data corruption? Section 6 refers to 5.2 for the error codes but its not clear to me which one is returned for this case. I think clarifying that in section 6 or 5.2 is needed. (3) p13, 2.2 and 2.3 - these things happens at registration time right? I think making that clear is needed since we don't specify a registration protocol here. (4) 2.3.1 uses the term "token endpoint" without definition (its defined in section 3) and in particular without making it clear if both access and refresh token issuance is covered (I guess it is). (5) The same text about x-www-form-urlencoded is repeated various times, it'd be better to do that once and refer to it where necessary. (6) 3.1.2.2 states the rules for when ASes are to require registration of redirection URIs. I think you need to clarify that some. First, you use the term "redirection_uri" for both a "complete" URI and for a scheme/authority/path triple that can be added to via a query component which is confusing. Second, its overall a very complex rule with a MUST, two MAYs and 3 SHOULDs. I do think it could be made clearer by putting the MUST up front and separating issues related to complete URI and triples separately from the when something MUST be registered. (7) 4.2.1 and elsewhere - refers back to 3.1.2 for the way in which the redirection-uri is OPTIONAL - I'm not sure that's sufficiently clear, 3.1.2 is quite long and discusses a bunch of things - couldn't it be made clearer when the messages are defined? More generally, is there no way to avoid the extensive cross-referencing in the message field definitions? E.g. 4.2.2 has references to 7.1 and 3.3, and others are similar. Organising the text for the benefit of the reader is a good thing so would it be worthwhile to do an editing pass for just this purpose - to reduce the number of forward references and minimise the number of pointers in general? On a similar note, 4.1 & 4.2 call out specific errors, but 4.3 defers to 5.2, why? Be good if that could be made more cosistent at the TOC level. (8) How can the AS protect against brute force attacks in 4.3.2? I think you could give a bit more guidance, e.g. say to rate-limit or generate alerts or whatever, but not as normative text, just good hints. (9) In 10.12 you say how a client can protect against CSRF via the state field but you don't say how the AS can do the same in order to satisfy the MUST in the last para of 10.12. Can you not add a hint or reference here? Some nits: (Stuff that seemed more serious at first:-) --------- (1) In 2.3.1 I think you're ruling out putting the client_id and client_secret in the request URL - is that right? If so, that's good, but I think it needs calling out since people do that all the time and it'd be good to say why its bad to do that. (2) The redirection endpoint is introduced in 3.2.1 but is not listed at the start of section 3 which only lists two endpoints. (3) In 4.1.2 what does it mean to "attempt" to revoke tokens? Why can't the AS just revoke them? (Where revoke == not accept them when they are next presented, right?) (4) I think this is just language but wanna check. 4.1.2.1 and 4.2.2.1 errors have a state field. Text says: "If a valid "state" parameter was present..." which would imply that state variables are either valid or invalid according to the AS. I don't think that's the case, and nor should it be. (If it were, I'd have a security concern that I could use otherwise crap requests to probe for good state values.) s/valid// I think? (5) I don't get the benefit of saying the client SHOULD ignore unknown fields in the response in 4.2.2 - what's effectively the difference between that and "MUST ignore" - I don't get it, and hence don't see why you don't say MUST ignore. (6) Why say "MUST NOT issue a refresh token" in 4.2.2? Are you making an assumption that access & refresh tokens are distinguishable to anyone other than the issuer? If not, then is this just saying "don't make a mistake"? (7) 5.1 says that the client SHOULD ignore "unrecognized response parameters" - does that mean unrecognized parameters in the JSON entity body? Is it clear enough as-is that those are "response parameters"? (8) How does the use of TLS on endpoints used for end-user interaction "reduce the risk of phishing attacks"? I don't get that. Maybe you mean that TLS+users actually checking server identity reduces the risk of successful phishes? I think that's a bit different. (I do like the MUST though.) Original list of nits: ---------------------- - Intro: Maybe add some ascii-art showing the roles of the user, browser, client etc. The term client as used here is still commonly confusing and especially worth going the extra mile to clarify, before it is first used. What I think needs to be said early is that what is here called a client is normally (running on) a web server. - p4: Maybe s/a string/an octet string/ in the token descriptions, unless the access token encoding is constrined to what'd be understood as a string. - p8: Seems odd to speak of "issuing" an implicit grant - wouldn't that make something explicit? Maybe say "With an implicit grant..." instead in the 2nd para of 1.3.2? - p8: I don't get what "its device operating system" means. Do you mean the client is an already-trusted part of the resource owner's device OS? - p9 - "Issuing a refresh token is optional" - might be better to say that its the authorization server's choice and there's no way for the client to signal a request for one. - p10 - In figure 2, why is the Refresh token shown as optional in step (H) but not in step (B)? I think it'd be clearer for the figure to reflect the protocol options and say that the refresh token is optional in (B) as well. - p12 - the confidential/public terminology isn't great, did the WG consider "authenticating" vs "non-authenticating"? Trying again to find better terms would maybe be worthwhile. Also, it may be worth explicitly saying that it doens't matter how hard to guess a secret the client has nor how good a MAC algorithm you use with that secret - if anyone can get the secret then the client is still public. It nearly says that, but not quite and given that many developers just don't apparently still think that a hardcoded secret embedded into a binary is good enough, I'd argue its worth adding a bit more here. - p12/13 in the application profile descriptions - is "resource owner's device" correct? That seems to rule out kiosks, which may be correct, but which isn't obvious. Maybe you mean "device being used by the resource owner" with no implication as to ownership of the device? - p13 - client application: typos: s/such access tokens/such as access tokens/ s/which...interact with/with which the application may interact/ - p13, "establishing trust with the client or its developer" is badly phrased, I think you mean the AS SHOULD NOT just accept the client type unless it has some external reason to do so. Trusting the developer might be one such. Being paid is another, and maybe more likely;-) - p13, 2.3 has 2119 language both about the things established at registration time and things done in the protocol defined here - would it be better to identify those separately in different sections or maybe move the registration time stuff into 2.2 with a possible renaming of 2.2. - 3.1.2.1 has a SHOULD for TLS which I think generated a lot of mail on the list. I think saying why this is not a MUST would be useful, since its the kind of thing that might get revised later on if the situation changes. - Figure 3, (p21) has multiple lines labelled A,B & C - saying why might reduce some confusion. Or maybe, say that the labels reflect rough event timing or something. It'd also be good if subsequent sections said which parts of figure 3 they're addressing, e.g. 4.1.1 maps to (A), right? Its hard to tell. -p25, 4.1.3, what does "assigned" "authentication requirements" mean? Suggest deleting the parenthetical clause since "confifential client" should be sufficiently well-defined to cover that. -p28, the description of step (D) isn't clear to me - who does what with the URI fragment? -p30, why refer to "HTTP client implementations" when you're previously said UA? If there's a substantive difference it'd be good to be clear about that. Same para: s/such client/such clients/ -p33 - Just checking - 4.3.1 says the client MUST discard the credentials once an access token has been obtained - why not before though, e.g. once an access token has been requested? Is there a re-tx thing that the client might do? -p38, is "token_type":"example" a valid value? If not, better to use one that is. -p40, s/client it was issued/client to which it was issued/? -p40, s/require/REQUIRE/ in the 2nd last bullet on the page -p43, s/native clients may require/native clients require/ I'd say the "may" is worth deleting both to avoid 2119 language and because we do know that these require special consideration. -p46, s/such malicious clients/such potentially malicious clients/? Not all unauthenticated clients are bad, though all of them could be bad. -p47, s/Access token (as well as.../Access tokens (as well as.../ -p50, 10.8 seems to just repeat stuff from earlier in 10.3 & 10.4, is that deliberate?
_______________________________________________ OAuth mailing list OAuth@ietf.org https://www.ietf.org/mailman/listinfo/oauth
