Review comments from Yaron

Master issue to track comments from @yaronf

[x] 1.5: While it is possible to deploy an AS in a stateless environment, such deployments will need a way to manage the current state of the grant request in a secure and deterministic fashion. - I'm not sure such a deployment is possible, e.g. if the state is managed by the client, it would be able to roll back to a previous state and cancel any pending revocations.
[x] 1.6.1: the generic sequence diagram seems to have a bug, especially when compared to the one in 1.6.4: after message (3), there is interaction with the RO, marked with (B). What then initiates the client's next step, the one marked (4)? Is there polling going on?
[x] 2.1.1: access token flags - the "bearer" flag is mentioned for requests and responses, but the "durable" flag only for responses. I suggest to add "allowed use" as a new column to the relevant IANA registration registry (Sec. 11.2).
[x] 2.1.1: "since no flag is provided in this example, is bound to the client instance's key", better be more precise about the flag: since the "bearer" flag is not provided in this example, is bound to the client instance's key.
[x] 2.2: SAML - please add a reference. Also, is "SAML 2 assertion" well defined? Are there any constraints? If we don't want to give a detailed definition, maybe leave it out.
[x] 2.3: the example includes both a "jwk" and a "cert" for the public key. I think this contradicts the text in Sec. 7.1: "A key [...] MUST be presented in one and only one supported format".
[x] 13.12: when we say "natural extension point" I suggest we add a reference to the relevant IANA registry (Sec. 11.5 IMO). In general it's worth looking at where the word "extension" is mentioned throughout the document. For example, in Sec. 4, "or through some other method defined by an extension of this specification", I don't think we have defined such an extension point, so I would remove that text or add the extension point.
[x] 2.3: the "display" URI is limited to web addresses, and can simply be a URL.
[x] 2.3: "SHOULD use a different key for each AS" - I would change it to MUST. We know of one attack, there may be more. If we leave it a “should”, deployers will not have enough information to make an informed decision here.
[x] 2.3.1: "If the client instance is identified in this manner, the registered key for the client instance MAY be a symmetric key known to the AS." I think this gets it wrong: for symmetric keys, it is the "key" value that's given by reference. The "client" value is unrelated.
[x] 2.4: "Assertions SHOULD be validated by the AS." In addition to ensuring the JWS is signed correctly, are there other checks the AS must do? E.g., ensuring the assertion refers to the same identity as the sub_ids?
[x] 2.4.1: what is the lifetime of the user reference? Can the client assume it will remain valid forever (as long as the user itself is valid)?
[x] 2.5: "array of strings/objects" - this is somewhat ambiguous, is it allowed to mix strings and objects in the same array?
[x] 2.5.1: "by a JSON object with one required field" -> "by a JSON object with the mode required field"
[x] 2.5.2: I think the nonce MUST be valid ASCII, to avoid complexity when computing the hash. The same goes for the Finish nonce in Sec. 3.3.5.
[x] 3.2.1: the text in 2.1 implies that for a single access token, if a table is included in the request, it should be included in the response. So I suggest: "REQUIRED for multiple access tokens or if a table was included in the request, OPTIONAL otherwise."
[x] 3.2.1: when the AS returns a key by value, there's a lot of complexity and possible security issues as the Client tries to correlate it to known keys. We should RECOMMEND to use the key reference (Sec. 7.1.1) mechanism instead.
[x] 3.2.2: this sentence got mangled: "other issued access tokens are included in the response the requested names appropriate names".
[x] 3.4: ISO8610 -> ISO 8601 (but why not RFC 3339?)
[x] 3.6: "provide a error as a string" -> "provide the error key as a string"
[x] 3.6: what is the error that corresponds to "not yet granted" - async authorization as per Sec. 1.6.4 (step 2)?
[x] 4.1.2: "the URI is usually be opened" -> "the URI is usually opened"
[x] 4.2: "follow the appropriate method at upon completion" -> "follow the appropriate method upon completion"
[x] 4.2.2: "the AS MUST protect itself" - I think this should refer to the client.
[x] 4.2.3: there's something missing here: the section discusses hash functions applied to strings, but does not mention the string's encoding to bytes. Maybe we should say, instead of "The party then hashes this string", "The party then hashes the ASCII bytes that encode to this string".
[x] 5: "Access tokens other than the continuation access tokens MUST NOT be usable for continuation requests." I would also add: "Conversely, continuation access tokens MUST NOT be usable to authorize requests into Resource Servers, even if collocated with the AS."
[x] 5: "The AS MUST be able to tell from the client instance's request which specific ongoing request is being accessed, using a combination of the continuation URI, the provided continuation access token, and the client instance identified by the key signature." The first two (and preferably only one) must be sufficient to identify the request. I suggest to change to: "The AS MUST be able to tell from the client instance's request which specific ongoing request is being accessed, using a combination of the continuation URI and the provided continuation access token. The AS MUST verify the signature and MUST ensure that it is bound to the client that originated the request."
[x] 5.1: "If the AS detects a client instance submitting the same interaction reference multiple times" - can we rephrase this rule so that it depends on the grant's state, in order for the AS not to have to cache references?
[x] 5.2: the text says the POST does not include a message body, but the example still includes a Content-Type header.
[x] "If the grant request was previously in the approved state, the AS could decide to remember the larger scale of access rights associated with the grant request..." If the client first asks for A+B permissions than asks for only A, I would say "remembering" A+B and granting it again is a mistake. The client may have good reasons to ask for restricted permissions, e.g. if the client's security posture has changed and it is at higher risk now. Granting it A+B again would be a mistake. If this text is not about the access rights being granted but rather about some internal optimization at the AS, please clarify it in the text.
[x] 5.3: "Modification requests MUST NOT alter previously-issued access tokens. Instead, any access tokens issued from a continuation are considered new, separate access tokens. The AS MAY revoke previously-issued access tokens after a modification has occurred." IMO the AS SHOULD revoke in this case, which would be more in line with the intuitive semantics of a PATCH. If the client wanted more access tokens for the permissions it is already holding, it could have made a whole new grant request.
[x] "In the future" -> "Some time later".
[x] "The AS would likely revoke previously-issued access tokens that had the greater access rights associated with them, unless they had been issued with the durable flag." I think a more consistent interpretation of "durable" is that it is used ONLY to make access token rotation easier, by allowing overlap between the old and the new token. If a new token with reduced right is expressly requested, it would make more sense to always revoke old tokens, regardless of the "durable" flag.
[x] In general the logic of Sec. 5.3 implies that the AS needs to keep track of all tokens (a potentially unlimited number of them) associated with the grant request, along with each one's associated rights, and compare each new token's rights to all previous ones. A simpler way to manage the grant request would be to just keep the last one, and always revoke it whenever the grant request is modified.
[x] #502

ietf-wg-gnap / gnap-core-protocol

Review comments from Yaron #493