Subject: [Ace] Review for draft-ietf-ace-key-groupcomm-05

From: Jim Schaad <[email-masked]>
Date: Fri, 13 Mar 2020 20:28:00 -0700

Here is a new review - the sooner you ask about anything that is unclear the
more likely I will remember what I was referring to.

Jim

*  In figure 4:  The CDDL is not correct.  "2*role" should be "2*role:tstr"
or role should be defined as a separate item

* Section 3.2 - The third to last paragraph is incorrect, the token should
have the expires AT not the expires IN parameter.  There is a solid
assumption that in this case there is a common idea of a wall clock between
all of the servers and there for stating a hard expiration time is more
appropriate than an expires in moment.  Additionally, tokens will generally
not include the rs_cnf even if it is in the response message and will
include a cnf for an asymmetric key.  In short, this paragraph is just plain
wrong.

* In section 3.2 - Why is the AS not allowed to return the same
authorization token to the same request assuming that things such as the
amount of time left before it expires would seem reasonable.  I am not sure
why this is a requirement let along one in this draft.   Additionally, it
says nothing about re-use of the same secret for a symmetric algorithm which
also might be something that needs to be said.

* In Section 3.3 - As an input to your "make it an array" decisions,
remember that the order of scope elements that the client gave to the AS are
not necessarily the same order as are in the token.  I think you might just
need to collapse all three arrays into a single value in the map.

* In section 3.3 - Move the data structure of sign_info to section 3.3.1

* In section 3.3 - Move the data structure description of pub_key_enc to
section 3.3.2

* In section 3.3.3 - I think you might want to rename the 'rsnonce' to be
something that is KDC specific.

* In section 4.1 - I am not sure that I like the idea of reserving
/ace-group against all comers, esp. as this is not the resource name that is
being used by the OSCORE version of the document.  You can talk to the core
people, but a better way of saying that this is a specific interface is to
defined that as a property ("if=....") and register that interface name.  If
you really do mean to reserve this name against all comers then you need to
make a registration in the well-known registry as part of the IANA sections.
The last paragraph may or may not reverse the section on /ace-group.

* In section 4.1.2.1 - You cannot really reference cnonce to the framework
document as you are going to be using a different content-format and
therefore a new identifier is needed.

* In section 4.1.2.1 - See issue #60 on github about when credential
verification is needed.

* In section 4.1.2.1 - Please insert a forward pointer on 'control_path' to
a section which describes exactly what this is supposed to be doing because
it is not clear from this text.  I think it will need more than a single
paragraph to do so. 

* In section 4.1.2.1 - What happens if a 'control_path' is provided but not
supported - does that mean a bad request?

* In section 4.1.2.1 - /assigns a name NAME to the node/   is this assigning
a name to the node or to the join result?  I don't know what the node would
be but I may have missed a definition.

* In section 4.1.2.1 - Unless you have a reason for allowing floating point
for 'exp', make it integer only.  

* In section 4.1.2.1 - For control_path,  I assume that fragment is not
supported - are paramenters?  The problem is that you say "URI path" which
has a meaning.

* In section 4.1.2.1 - Mismatch between NAME and NODENAME in the text - need
to harmonize

* In Section 4.1.2.1 - I believe that the minimal content for a join request
is an empty map.  I believe that the minimal content for a join response is
an empty map.  The first makes sense as authorization comes from else where
and so can the public key if it is needed.  I am not so sure that an empty
response makes a great deal of sense for a response but it may be what you
intended.

* In Section 4.1.2.2 - I am not sure that it makes sense to return key
material to a client for which the scope permits it to join, but which has
not actually done the join process.   

* In section 4.1.3.1 - I would like to have the ability to do a fetch based
on the roles of the client associated with the public key.  That is I would
like to retrieve only those public keys associated with a requestor.

* In section 4.1.4.1 - Is there any information in the policies which is
sensitive?   Is there a reason not to return this info to everybody?

* In section 4.1.5.1 - Should this be restricted to group members?

* In section 4.1.6.1 - This is be restricted to just the correct client -
That is subscriber #2 should not be able to access subscriber #1's resource
here.

* In section 4.1.6.3 - The third paragraph is redundant as this is already
covered by the first paragraph

* In section 4.1.6.3 - The second and fourth paragraph are not making any
sense.  There is no body for a DELETE handler.

* In section 4.1.7.1 - If a public key is replaced, does this constitute a
reason that a key roll over is going to be required?  Is this something that
needs to be distributed proactively if it is allowed by the server since
otherwise things may be really messed up?

* In section 4.3 - I don't know if this is worth while thinking about or
not.  There may want to be two different expiration values that are defined
in the system.  When you stop sending using the key material and when you
stop receiving using the keying material.  These values would need to be
contingent on a "key revocation" event as well.  The delta could be setup as
a new group parameter with a default value set by the document.

* In section 4.5 - Fetch should return 2.05 (content) not 2.01 (Created).
Nothing was modified on the server.

* In Section 5 - For bullet 1 of bullet 3 - I think that the reasoning
behind not authorized needs to be expanded.  For example it might say, the
node is no longer authorized for that group. 

* In section 5 - For bullet 2 of bullet 3 - This sentence seems to be an
oxymoron - how can it still be valid and expired at the same time?   Note -
I am not sure that introspection actually separates these two different
cases, but I have not gone back to read the framework document on that
point.

* In section 7 - para #1 - This need to be explained a bit more.  I know
what this is talking about but my first thought was how can it be a replay
if this is the first time I have seen it?   This should probably also note
the possible mitigation that it is limited by the time since the last time
the key was rolled over.

* Section 7.1 - In the third paragraph, there is a discussion of CoAP
retransmissions, this is causing a bit of a frisson in my mind as, at least
for multicast, this would not be something that happens.  The retransmission
would need to occur at the application level instead.

* Section 8. - In term of the name, what about doing the encoding in JSON?

s/minum/minimum/