Last Call Review of draft-ietf-gnap-core-protocol-16
review-ietf-gnap-core-protocol-16-secdir-lc-housley-2023-11-04-00

I reviewed this document as part of the Security Directorate's ongoing
effort to review all IETF documents being processed by the IESG.  These
comments were written primarily for the benefit of the Security Area
Directors.  Document authors, document editors, and WG chairs should
treat these comments just like any other IETF Last Call comments.

Document: draft-ietf-gnap-core-protocol-16
Reviewer: Russ Housley
Review Date: 2023-11-04
IETF LC End Date: 2023-11-21
IESG Telechat date: Unknown

Summary: Has Issues


I did not review the Appendices.


Major Concerns:

Section 1.2: I am having trouble getting my head around the idea that
the AS role grants subject information to client instances.  This feels
like a purposeful confusion between authentication and authorization.

Section 1.3: Elsewhere in the document, there is a careful distinction
between software that acts on behalf of a subject and the natural person
that uses the software.  This distinction is missing in the definition
of Subject.  It is made worse by including organizations.  Please say
how people and organizations "decide whether and under which conditions
its attributes can be disclosed to other parties."  I believe this is
done by interacting with the software or initial configuration of the
software.

Section 1.5: Is the AS the only stateful role?  If not, Section 1.2
should say which ones are stateful and which ones are not.  If so,
please clarify the 2nd para of Section 1.5.

Section 2: I worry about the phrase "unless otherwise specified by the
signature mechanism" inside a MUST statement.  Which part of the MUST
statement can the signnature mechanism change?  Can it change the use of
a JSON object?  Can it change the use of HTTP POST?  Can it change the
use of application/json as the content type?

Section 2.1.1: This subsection is about a request for a single access
token, yet the description continues to talk about "one or more access
tokens".  Please be consistent.

Section 2.1.2: I expected a parallel structure to Section 2.1.1.  I
think it would really help the reader if the sections had the same
structure.

Section 2.3.2: I am not worried about logo_uri if only data: URIs are
allowed, but that does not seem to be the case.  Since the logo might
be fetched, there need to be an integrity protection mechanism to 
ensure that the web server does not provide a different image than
was intended.  RFC 3709 has this same concern and it uses a hash of
the image to ensure that the intended image was provided.

Section 2.5.1: I do not understand what an implementer is supposed to
do based on this MUST statement:

   "All interaction start method definitions MUST provide enough
   information to uniquely identify the grant request during the
   interaction."

Section 2.5.2.1: How does the use of an application-specific URI scheme
provide for the same security as HTTPS?  It seems like an way to avoid
them.  This text appears several places in the document.

Section 3.1: It says:

      " ... and omission of the value MUST NOT be
      interpreted as zero (i.e., no delay between requests)."

It would be much better to provide a default value.  That is,
omission of the value MUST be treated a delay request of 5 seconds.

Section 7: Please consider a reference to RFC 4107.  I'm not sure where
in this section is the best place to add a cite.

Section 9.1: I do not understand what an implementer is supposed to
do based on this MUST statement:

   "client instance MUST check its value to protect itself"

Section 13.1: This section ought explain what is menant by "mutual TLS"
as used in the body of the document.  Please consider moving Section 13.17
and Section 13.18 to follow Section 13.1.

Section 13.2: Please consider the work of the SUIT WG.

Section 13.24: This section does not seem like a Security Consideration.
If the calculation of the interaction hash is not done the same, then
there will be interoperability issues.


Minor Concerns:

Section 1.4, 1st para: Where does the quoted text appear?  Please add a
cite.

Section 1.4, "end user/client" bullet: This seems like the wrong place
to be an attacker's client software.  Rather, a forward pointer to
authentication considerations seems more appropriate here.

Section 1.4, "client/AS" bullet: Again, this seems like the wrong place
to discuss the difference between honest AS and otherwise.  As a result,
the actual trust relationship between the client and the AS is unclear.

Section 1.4, last para: Why is this here?  If it is relevant, this feels
like the wrong place for this statement.

Section 3.3: This seems like a throw away statement:

   "The AS MUST NOT respond with any interaction mode that the AS does
   not support."

It would me much more helpful to say what ought to happen is the client
asks for an unsupported interaction mode.

Section 3.3.3: It is unclear whether you want the code to avoid
characters that are easily confused, like 1 and l.  Of course, the
more characters that are to supported reduces the effort to guess the
code.  This text appears several places in the document.

Section 7.3: Please provide a reference for the "RS256" algorithm.


Nits:

Global: Please create a reference for the hash-alg IANA registry and use
it instead of the URL to the registry: 
https://www.iana.org/assignments/named-information/named-information.xhtml#hash-alg

Section 1.2: Suggested wording improvement:

   OLD:
   ... For
   example, a client instance could have components that are installed
   on the end user's device as well as a back-end system that it
   communicates with. 

   NEW:
   ... For
   example, a client instance could have components that are installed
   on the end user's device as well as on a communicating back-end
   system.

Section 1.5: Why are the state bullest offered with underlines? Please
use the same format as Section 1.3 uses for the elements.

Section 1.6.3: Stretch the "End User" box in the figure or shift things
to avoid "Completed+".

Section 2.2: s/subject that information is being requested for./
              /subject for which information is being requested./

Section 2.2: s/All identifiers in the sub_ids array MUST/
              /If present, all identifiers in the sub_ids array MUST/

Section 2.5.1.1: s/techniques such as this/techniques/

Section 3.3.1: s/is a string containing the URI to direct the end user to./
                /is a string containing the URI for the end user to visit./

Section 3.4: s/that the subject information is received from/
              /AS from which the subject information was received/

Section 4.2.3: s/single newline (\n)/single newline (0x0a)/

Section 5: s/requests to RS's/requests to an RS/

Section 6.2: s/ AS should/ AS SHOULD/