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

From: Jim Schaad <[email-masked]>
Date: Thu, 7 Nov 2019 17:33:12 -0800

This is a review based just on reading through the document.  Comments based
on trying to implement after some time.

After reading this, you have started down the path of being RESTful, but you
are definitely not there yet.  You need to examine statements like "GET
method is safe, meaning that applying it to a resource does not result in a
state change of the resource".  

1.  In section 4 - The new format name is backwards - the "+cbor" goes on
the end not the front.  If you are defining a CBOR number/JSON name mapping
that is unique for this then a new content type would be indicated.

2.  In section 4.1.2.2:  It I not clear to me why an unauthenticated version
of this would be to return the group parameters, assuming that such are
public.  The assumption of public would come from the fact that it is
returned from the POST of the token where it is returned.

3.  In section 4.1 - I understand that "gid" in the paths is going to be a
group specific identifier.  It is not clear to me if "node" in the path is
also going to be a per registered node or a single node.  This may be left
over from what I remember in Montreal.

4.  In Section 4.1.2.1 - 'get_pub_keys' - This text appears to imply that
the client knows a priori if the KDC is going to store the keys.  It implies
that if the KDC does not store them, then it might be considered an error to
include this key.

5.  In Section 4.1.2.1 - 'client_cred' - should this text be altered to
reflect the concept of a monitoring node that would never respond?

6. In Section 4.1.2.1 - 'client_cred_verify' - where does N_S come from in
the event that the token is used directly to validate in TLS rather than
being posted.

7. In Section 4.1.2.1 - 'pub_keys_repos' - It is not clear to me if the URI
includes some type of query parameter to map to the certificate rather than
just being the URI of the repository itself.

8. In Section 4.1.2.1 - 'num' - Is this field ever allowed to be reset to
zero by the KDC?  

9. In Section 4.1.2.1 - 'kty' - I would suggest renaming this to 'gkty' to
avoid confusion with the same field used for COSE and JOSE keys.

10. In Section 4.1.2.1 - 'mgt_key_material' - I think a better set of
requirements is MUST say if it is used, if used then MUST be specified.

11. In section 4.1.3.1 - This seems to be better reflected as a FETCH rather
than a POST operation.

12. In Section 4.1.3.1 - I don't understand the requirement for the
'get_pub_keys' value has to be non-empty.  Is there any reason not to say
that empty equals all?  Alternatively it could be a return nothing answer
but not an error.

13. In Section 4.1.6.1 - It is not clear how I am identifying the "client"
to be removed.  Can I do a third party removal?

14.  In Section 4.1.6.2 - Is this supposed to return new keying material or
the current keying material.

15. In Section 4.3 - I am not sure why I would be observing /ace-group/gid
rather than /ace-group/gid/node in order to get updated keying material.  I
don't know that I would want to get all of the public keys when it rolls
over.  Alternatively, I would observer /ace-group/gid/ctx-num for the same
purpose.

16. Section 4.4 -  It seems odd to use GET for the purpose of renewing key
material.