Subject: [Ace] Review of ietf-ace-key-groupcomm-07

From: Jim Schaad <[email-masked]>
Date: Thu, 25 Jun 2020 21:01:45 -0700

* Section 1 para 1 - I have a vague memory of deciding that we were going to
become CBOR only with this document per the argument from Carsten.  I did
not find this in the minutes so this could easily be some other document
that I am thinking of.

* Section 2 - I have a problem with Figure 1 in terms of what is labeled as
an RS.  The text below calls the KDC and RS and not the Dispatcher.  

* Section 2 - update DTLS reference

* Section 2 - I think you might want the last sentence to refer to step 3
rather than section  - The pointer to the section has already been provided
but pointing back to the figure makes a degree of sense.

* Section 2 - KDC - This is the first time that I have heard about the fact
that this is a two part protocol - You should tell me about the phases
before you get here.

* Section 2 - "Leaving or removing" this is a sentence which does not read
well.  (Also why is this current when no other line is.)

* Section 2 - You should probably have a numbered item that corresponds to
the secure group communication line in the picture.

* Section 3 - The content format application/ace+cbor is not defined by the
transport profiles but by the framework.

* Section 3.1 - Given that a role is defined as a tstr, that format is not
usable if one wishes to assign an integer abbreviation per OPT7.

* Section 3.1 - the "Other additional" seems to be a strange thing to have
in this list because it does not match the lead in paragraph.

* Section 3.1 - Is there a reason that a gid is a bstr and not a tstr.  I
assume that this is the same as GROUPNAME which much be a text string to be
in a url-path.

* Section 3.2 - s/non expired/non-expired/

* Section 3.2 - the def of scope seems odd because it is not well defined if
it would be the same as the requested scope. (AS response)

* Section 3.2 - the text w/ scope can be simplified to the scope that the AS
grants access to and skip the last sentence.

* Section 3.2 - ditto w/ above on "other additional"

* Section 3.3 - s/request necessary information/request information/

* Section 3.3 - Am I supposed to know why I care about public keys at this
point?

* Section 3.3 - last sentence of the text implies that sign_info and
pub_key_enc are required.  Last sentence of para 2 needs to be re-written

* Section 3.3 - should identify what sign_info is for.

* Section 3.3 - are you imposing a new requirement that the token be
validated prior to returning the response to a token post?  As I remember
it, this is not required to happen prior to the response being returned just
before access is granted.  (I.e. you can validate the token in parallel)
(After successful verification...)

* Section 3.3 - "payload of response deviates"  I don't believe that this is
a true statement.  However, you should state at tis point how it deviates if
you believe it does.  I assume that you mean that there is a payload as
oppose to no payload.
  
* Section 3.3 - You would not be able to reuse the same kdcchallenge value
with a different key.  It would not have any way of being associated with
that key and would look like somebody else was attempting to use the value.

* Section 3.3 - s/were included in the request/are included in the request/
present tense.

* Section 3.3 - Do you really mean to say "request includes pub_key_enc",
"response does not include pub_key_enc"?  That seems to be wrong in the
general case.  Also seems to say the same thing about 'sign_info'.  Also
seems to contradict the next paragraph.

* Section 3.3 - The forward reference for sign_info is incorrect - it should
be 3.3.1.  (I also don't know if you need this two paragraphs in a row.)

* Same paragraph - group identifier or group name?  You need to do a
detailed pass through the document and look at every instance of this string
and either make it an identifier or a name and perhaps add these to the
terminology section so that one can quickly look back to distinguish them.
Perhaps you should be using context identifier rather than group identifier
just to make it clearer the difference between what goes into the different
locations.  The problem with that is it is OSCORE specific so it may not
work well.

* Section 3.3 - last paragraph - should this be referring to applications or
profiles?  -- really a global question.  Consider - is the OSCORE profile
attached to a specific application or to a set of unknown applications?

* Section 3.3.1 - s/one ore more/one or more/

* Section 3.3.1 - s/is lower or at most equal to/is at most/ - also what
happen if the response would include zero elements?  You asked and nothing
needs to be returned?

* Section 3.3.1 - second bullet - kill the "if included" text.  This is
covered above.

* Section 3.3.1 - s/sign_info/sign_info/

* Section 3.3.1 - This is looking really complicated and I don't know why
you don't just simplify it down to a single request/response pair.  Define a
value for use when the server either does not know or does not want to share
a value and so forth.  The size of data if sign_info were to be omitted is
almost the same size as if it where present so that does not seem to be much
of a win.  Making the answer one byte longer for adding the pub_key_enc_res
field does not seem to be a huge problem for a one-time message.

 * Section 3.3.1 - The first paragraph implies that pub_key_enc would be in
the reply but don't have any information about what that would look like.
See also previous item.

* Section 3.3.3 - s/((/(/

* Section 4 - why are you calling this the first exchange with the KDC - who
was I talking to in section 3?

* Section  4 - are the policies new or just retrieved?  May want to swap
order.

* Section 4 - are we still specifying a group to join or is that implicit in
the URL?  Tell me how it is specified?

* Section 4 - I don't think that the (re-) is helping understanding here.

* Section  4 - what type of individual key is being obtained in bullet 2 -
how is this different than getting the current keying material?

* Section  4 - Who is enforcing the policies?  Just retrieve the policies.

* Section 4 - I don't think the e.g. in getting public keys serves a purpose
here.

* Section 4 - I am not sure what the "at hand" is supposed to be providing.
/for the requested group identifier/

* Section  4.1 - do we want to make the if value be a dot tree?  ace.group

* Section 4.1 - The bullet for /ace-group seems to indicate that the name is
fixed which is the opposite of what the previous paragraph says.  I find
this confusing.  I really find it confusing that this document is specifying
a url-path name is being specified, and the oscore document immediately uses
a different value.

* Section 4.1 - what does it mean for a sub-resource to be "fixed".  Does
that mean it never changes?

* Section 4.1 - a brief description of what is in the resource would be
helpful here.  "pub-key: this resource contains the public keys of all group
members.  The resource supports the GET and FETCH methods."

* Section 4.1 - Yes but what is a NODENAME and why do I care about it?

* Section 4.1.2.1 - It would be useful in the first paragraph to re-iterate
that this is the "join" operation  The current description seems to be
really odd.

* Section 4.1.2.1 - get_pub_keys - One consideration that needs to be
highlighted for a constrained device is that this may result in a large
return value and it may be better to get them only as needed.

* Section 4.1.2.1 - It would be nice to be able to use get_pub_keys with a
filter so that only responders are returned.

* Section 4.1.2.1 - 'client_cred' the description of when this field is used
does not seem to include a server which only responds to an individual. It
could be said in that case that it is not sending messages "to the group"
but only to a member of the group.

* Section 4.1.2.1 - 'conce' why bother pointing to ACE for a description of
this document.  This is not a framework message so I am not sure why this is
interesting.

* Section 4.1.2.1 - Which scope is being used, the one in this message
(assuming it exists) or the one in the token?  If the message does not have
one then what happens?  Deterministic encoding?

* Section 4.1.2.1 - s/token is not being posted/token was not posted/

* Section 4.1.2.1 - formatting - the follow on paragraph to
'client_cred_verify' should be indented a level

* Section 4.1.2.1 - You missed the merge of sign_info and pub_key_enc in the
error handling paragraph.

* Section 4.1.2.1 - What is the reasoning behind returning the sign_info_res
field if the group is not in the scope.  This is not related to the failure
in any way.  Seems more like this would return a pointer to the AS rather
than anything else.

* Section 4.1.2.1 - How much can I just ignore rather than error?  If you
send me a public key and I don't care about it why can I not just ignore
that fact and go forward?  I both agree and disagree with the idea that
erroring on unknown fields is the correct way to proceed.

* Section 4.1.2.1 /succed/succeeded/

* Section 4.1.2.1 - (para before bullet) I don't like the idea that if you
send me a public key, I would use the stored one before looking at the one
that is in the join message.  This seems to be backwards behavior.  Note the
paragraph following the bullets is saying exactly the opposite thing a this
one is.

* Section 4.1.2.1 - I am not clear on the following situation:  Key P1 is
associated with groupX,  I do a second join and send key P1 but have a cred
verify that would fail.  Am I expected to detect this and error or say that
because you have already verified P1 it does not matter?

* Section 4.1.2.1 - both bullets are talking about failed verification -
kill the redundancy.

* Section 4.1.2.1 - 'num' the rule on the initial value of num should be put
into a section about setting up a group not here.

* Section 4.1.2.1 - 'num' vs 'ctx-num' should be harmonized.

* Section 4.1.2.1 - exp_delta should specify a default if it is not in the
message.  I there a reason that this is not a policy for the group?

* Section 4.1.2.2 - Ditto to above on the field being returned in the event
of 4.01

* Section 4.1.2.2 - I don't understand the need for 'gkty' as that I should
know from the join and it should never change in the life of the group.  I
would also expect that 'exp' is something that at least SHOULD be included
as that does change and is useful to have.

* Section 4.1.2.2 - Is the 'key' value expected to be modified for the
requester?

* Section 4.1.3.1 - What is the reason for having a map in this request
type.  Why not just have the array itself?

* Section 4.1.3.1 - Looks like a formatting error 2 paragraphs after figure
7

* Section 4.1.3.1 - The text on the first selection criteria has me really
messed up because I am not sure that 

* Section 4.1.3.1 - If not storing keys, should this return either resource
not found or not supported as an error code instead of an empty response?

* Section 4.1.3.1 - s/resource handlers only verify/resource handler only
verifies/  Unless you move it up one level.

* Section 4.1.4.1 - can we get rid of the map?

* Section 4.1.6.1 - I have no idea from the text why I would use this
method.

* Section 4.1.6.3 - para 2 - the first sentence does not make any sense to
me.  How could the handler accept a request for a node does not match
"NODENAME" as that is the url-path?

* Section 4.2 - "retrieve individual or group keying material"  What
individual material?

* Section 4.3 - /target the URI path possibly provided by/ This text either
needs to be a definitive method or some type of alternative method needs to
be provided.

* Section 4.3 - I am worried about congestion in more places than just for
Observe.  If a server is sending lots of messages out to clients on a
one-to-one basis it could easily flood the network.

* Section 4.4 - What does the first sentence even mean?  It seems to be
almost a circular argument.  

* Section 4.4 - I find the title of 4.4 to be somewhat misleading.  As far
as I can tell this is only talking about one type of renewal, but even that
is not clear to me.

* Section 4.4 - Why would it require an error in the event that a full rekey
is going to be done by the server, can't it just do the full renewal and
return the answer to the PUT?

* Section 4.5 - Is the second paragraph in the wrong place?  I don't really
care what the client does only what the server does.

* Section 4.6 - /After distributing the new group keying material/ This
should be done before doing that - it should also be committed long term
storage if one expects the KDC to restart w/o a forced rekey event.  And
even then it potentially should have a new context number.

* Section 5 - see previous comment

* Section 5 - Does the use of the DELETE method to inform a client that it
is no longer part of the group imply that a different control path is needed
for every group that the client joins?  I don't know that I was expecting
that.

Section 7 - / end of the rekeying process and the next loss of security
state/  This is not what I was trying to get at and it makes no sense to me.
The question is the interval between the end of the last rekey process and
the client joining the group.  If that is small enough then replay will not
work.

Section 7 /the Observations up to that point/ this does not make any sense.
You can lose who I currently doing the observation but you have already
tossed all of the observations themselves even without a reboot.

Section 7 - I don't understand the conditions you have laid out for doing
delayed rekeying  I would expect that it is a minimum number OR a maximum
interval.  But I don't think that is what this says.

Section 7 - This is a tic that I am trying to stop in my own writing.  Look
at all instances of "In fact," and they can probably be removed.  The other
one that I use a lot is "Actually,"

Section 7 - s/initiating rekeys/initiating rekey events/

Section 7.1 - Para 2 - it would not be a second try as the KID context field
would be different in general.

Jim