Re: [Ace] Review of draft-ietf-ace-oauth-authz-06

[Ludwig Seitz <[e-mail]>]

On 2017-08-04 23:41, Jim Schaad wrote:
> As promised I finally got finished with this review.

Thank you for your very thorough review Jim. Comments inline (note that 
there are a few questions as well).

/Ludwig


> 
> 1.  Need to decide if /token, /introspect and /authz-info are under
> /.well-defined or not.  If they are then this needs to be noted and there
> needs to be an IANA action if this has not already been done for OAuth.

I've added an issue to our issue tracker on this. However I would think 
that this is something that would need to apply to OAuth as well.


> 
> 2.  Add some of the actor terms to section 2- such as Resource Owner


Ok, will do

> 
> 3.  Still unhappy about the lack of POP between C and AS.  How does the AS
> know that the key it is binding to the token is the right one.  The RS has
> no problems, but assumes that the AS did its job.

I'm not sure what problem that would solve. That C binds the token to a 
key it doesn't own? What benefit would C have from that?

If C wants to relay a token to a "friend" it could always share the 
pop-key with said friend.

> 
> 4.  If I want to use the SCOPES field as defined by Carsten, then I do not
> want the current restriction that is being imposed on the scope parameter in
> Section 3 (?) Under "Scopes and Permissions".

I'm just reiterating the definition of the scope parameter from the 
OAuth spec. I think Carsten's approach can be modified to fit into that 
spec.

> 
> 5. In section 3.2 - s/so- called// - don't be pejorative.

Ok, I'll rephrase that

> 
> 6. In section 4 - 2nd paragraph before figure 1 - I want the ACE profile to
> provide one method to do this.  There may be others that are specific to the
> profile however.

I've already made that change in the "insertDiscovery" branch 
(https://github.com/LudwigSeitz/ace-oauth/tree/insertDiscovery).

> 
> 7.  In section 4 - Under "ACE Profiles" - RECOMMENDS seems to be an odd
> thing to have here - this is not really a protocol recommendation is it?
> Are we allowing for JSON to be used rather than CBOR?  The text here on what
> encodings to use could be made more declarative.  Are we expecting any JSON
> use at this point for any profile define?  If so then a list of tradeoffs
> for profile creators and how to detect would be in order rather than the big
> RECOMMENDS.

You mean section 5 right?

Yes the intention is to allow profiles to specify JSON (or something 
else) as data format instead of CBOR.


> 
> 8.  Section 5.1 - Awkward language "If the client should to act"

Yeah that whole section needed a review. Fixed.

> 
> 9. Section 5.3 - Is this really what you want to say is that one SHOULD
> authenticate C to the AS?  I think this is a MUST.  I question if this is
> really something that profiles should do rather than this document.  I
> thought that the profiles were targeted at the C <-> RS conversation.

My intention for the profiles is that they specify the communication 
(and communication security protocols). This MUST cover C <-> RS and MAY 
include AS <-> *any*.

I agree there should be a default fallback for the MAY parts. Adding issue.


> 10.  Section 5.4 appears to create a new endpoint that has not been
> discussed in other contexts.  Is this intentional?

No that's just an existing OAuth endpoint. Since I've had mostly on 
client credentials in mind, this endpoint wasn't discussed that much.

My impression is that this endpoint would mostly be used by 
non-constrained clients (towards the obviously non-constrained AS), and 
therefore it wouldn't need an ACE profiling, it could just be used as 
specified in OAuth.

> 
> 11. Section 5.5.1 - OK - I am reversing thinking.  However I need to get a
> summary of what a profile is going to need to specify a long ways before I
> get to this point.  Perhaps has part of the overview.  

The summary of what a profile needs to specify is collected in Appendix 
C currently. This appendix reiterates points that are spread out (in 
contextually appropriate places) in the main body of the spec.


> One of the questions
> is going to be can the C-AS section of one profile be used with the C-RS of
> another profile and keep the same security guarantees.  The overview is
> going to need to talk about this at some point.

This seems to be a "it depends" issue. We should have some text on this, 
the security considerations seem the right place to me for this. 
Creating issue.


> 
> 12. Section 5.5.1 - I would like to add some additional parameters at this
> point.  The first is a RS_Data field which is copied from the RS->C message
> verbatim.  It allows for encrypted data to be passed from the RS to the AS
> as part of the request.  Data type is COSE_Encrypt.

Are you referring to the "nonce" that can be part of the AS discovery 
message?  I'm not sure if that should go in this document or in a 
separate draft. What does the group think?

> 
> 13. Section 5.5.1 - I would like to see an AS field documented here so th
> that the 4 corner model can work.

Could you be more specific about the "4 corner model"? I'm not familiar 
with that.

> .
> 14. Section 5.5.1 - What is the default - use this for "aud"?  How is a
> client supposed to know what to put here for a new RS?

"aud" should contain be the identifier of the RS or a group of RS that 
the client wants to access.

Since the client ought to know which RS it wants to access with the 
token, I assumed it would know what to put in this field.

> 
> 15. Section 5.5.1 - on cnf - Use "It is RECOMMENDED that an AS reject a
> request with a symmetric key" as this is positive and where enforcement is
> done.
Ok, fixed.


> 
> 16. Figure 3 - This example no longer looks correct.  the content type
> should be the same as for Figure 2.  That is assuming it is really CoAP and
> not HTTP?
Right, fixed.

> 
> 17. Figure 4 - need to check out if client_secret is a real secret or not -
> it seems odd to say don't use symmetric and then have an example of passing
> one. Could be something else as I don't have RFC 6749 w/ me.

True, this method is specified on section 2.3.1. of RFC 6749, so we 
thought we'd offer an example.

I don't think this is a good feature security-wise.

> 
> 18. The set of operations should start with the C->RS response structure
> from DTLS profile as this is really the first thing that happens.

Sounds reasonable, fixed in the discovery branch. Merge pending approval 
of my co-authors.

> 
> 19. Is there a reason for a client not being able to request a profile
> rather than having it be configured into the AS?

We had that in a previous version and decided it was over-engineered. 
The client needs to be registered at the AS anyways, and could then also 
specify preferred profiles.

> 
> 20.  Section 5.5.2 - You are not being constant about encoding - this is
> saying MUST be CBOR while earlier it was only a recommendation,

Right, I'm adding an issue about this. We're also inconsistent in 
section 5.5.1. although not in RFC 2119 language.


> 21. Section 5.5.2 - Post figure 5 it says examples - but only one example
> exists.

Fixed.

> 
> 22. Section 5.5.4.2 - Please make the abbreviations mandatory not optional.
> I don't really want to support both.

I'll add an issue, I'm not opposed to that change, but I'd like to have 
consensus.

(side note: Congratulations, you just contributed to creating the 100th 
issue on this draft on github)

> 
> 23. Section 5.5.4.4 - Please define profile as being part of a CWT so that
> it can be communicated to the RS in the event that more than one profile is
> supported. Optional if RS only does one.

Wouldn't the RS be able to determine the profile from the message it 
receives from the Client?

Say the AS tells the client to use DTLS, the RS would notice receiving a 
DTLS handshake. Are there any scenarios where two profiles could be 
confused?

> 24. Section 5.5.4.5 - I don't know if it would not be cleaner to have a cnf
> that is only for the client key info and an rs_cnf (defined for
> introspection) that is for dealing with the RS keysWe need to specify that the .  That makes it cleaner
> when looking a the case of an AS->C or an AS->R->C message as the fields
> will be the same.

Indeed I argued the same way, but I was voted down by my co-authors. 
I'll raise the issue again with your newfound support.


> 
> 25. Section 5.5.4.5 - Figure 9 does not really make sense by itself.  It
> needs to have some context because it looks the same as for a RS key.

Your point makes sense, but this whole section is going to be rewritten 
when we transfer cnf to draft-jones-ace-cwt-proof-of-possession. I'll 
take that into account when we do the rewrite.

> 
> 26. Section 5.5.4.5 - Figure 10 is not formatted correctly as the map
> wrapping is missing on unprotected.

Correct, this figure is also bound to disappear, but I'll see that 
draft-jones gets the right example instead.


> Look @ section 3.3. RFC 6749 about scopes

That look more like a note to self to me, right?

> 
> 27. Need to know how to think about the idea of a client doing an
> introspection.  Is the response going to be different than a RS doing the
> query?  I assume that the distinction would be based on the authentication
> and internal knowledge - does not need to be documented except for response
> differences.

You mean if a client would do introspection on an access token it 
received? Depending on the AS its not sure it would be authorized to do 
so (mine has policies for determining who gets to introspect). I believe 
this can be left to the implementers. I might be convinced that it needs 
a security consideration though (if you insist).We need to specify that the

> 
> 28. Figure 14 needs to be updated

Done.

> 
> 29. Section 5.6.2 - Need to add rs_UZWab0RScnf - if more than one key need to tell
> the RS what key to use.

I agree. Issue created

> 
> 30. Section 5.6.4 - I think this is a required item if introspection is
> going to be able to return a random symmetric key.  Not needed otherwise.

It could also be used to inform the client about the RS's public key for 
authentication.

> 
> 31.  Section 5.6.4 - last para - may want to state that this secret is
> established at the same time that the token is established.

Sounds reasonable. Fixed.



> 32. Section 5.6.4 - establish MTI algorithms here?

For encrypting the client token? Is that really necessary? The AS should 
know what algorithms the client supports.

> 
> 33. Section 5.7 - I think that you need to add rs_cnf in the event of an RS
> w/ multiple keys so it knows which to use. usage would be optional.
> 

Agree. I'm extending the existing issue from 31.

> 34. Section 5.7 - last paragraph - should be JOSE and COSE not JSON and
> CBOR.
Right, fixed.

> 
> 35. Section 5.7.1 - Why would the RS respond with the cti - given a lack of
> text it is not clear when the MAY would be triggered.

It's just a suggestion to the implementer, not really necessary but 
useful in some scenarios (e.g. if the client later wants to delete or 
overwrite the token this could be good to have).

> 
> 36. Section 5.7.1 - Not sure how much information is being leaked by having
> different error codes being returned in these situations.  May only want to
> have one code.

This is a difficult one. One the one hand you are right some information 
is leaked, but on the other hand, withholding that information makes it 
very hard for the client to fix erroneous access tokens.

> 
> 37.  Section 5.7.1 - under what circumstances is the introspection request
> not made? 

If the RS is not configured to do so. E.g. if it has intermittent 
connectivity and the token is self contained.

> If the introspection request is done async how is the client
> token communicated back to the client?  Would that be done as part of a
> later access.  Seems to be dicey.

It's not designed to be done async. We should mention that. Issue created.

> 38.  Random thought - Is there a requirement that the same method be used
> for both posting to /authz-info and to the resource?  Could one use OSCOAP
> for one and DTLS for the other?  Question is because we are profiling and
> the assumption is that profiles are whole.

I'm assuming that /authz-info can be unprotected. It could use DTLS 
without client authentication, but it cannot use OSCOAP with a client 
that is not previously known, since the token sent to /authz-info 
contains necessary information to generate the OSCOAP context.

> 
> 39.  Section 5.7.2 - Last bullet - should note that tokens need to be shared
> to the RS - AS may issue lots of tokens but if RS gets none then no
> expirations

I think that bullet is the best we can do in the described usecase. The 
last bullet refers to a RS that has not clock at all and no connectivity 
to the AS. If the RS had connectivity it could do introspection.

If you have a better suggestion I'm open for suggestions.

> 
> 40.  Section 5.7.2 - Another "long running request" might be the case of
> sending back an ACK for a request followed by a 4.01 because the token
> expires before the request has finished processing.  Re-doing introspection
> might also cause this sequence of messages
> 

Do you think it would be useful to give these additional examples?


> 41. Section 6 - personal preference - remove the first sentence it is not
> useful.

I agree, I've heard this more than once now.

> 
> 42. Section 6 - Protection is from signature or MAC - but we are using AEAD
> for most of these.  Should probably have a security consideration that AEAD
> is preferred to MAC in most cases because of confidentiality as well.
> 

Agree, fixed.


> 43. Section 6- the phrase 'long-term key shared with RS' implies to me that
> this is a symmetric key.  May want different language to allow for people
> thinking of asymmetric keys as well.
> 

Fixed.

> 44. Section 6  - Please explain to me how targeting multiple RS with an
> asymmetric key is a problem - change it form shared secret to symmetric key.

Yes that sounds like something that was meant to apply for symmetric 
keys, which is NOT RECOMMENDED anyways some paragraphs above. Fixed.

> 
> 45.  Section 6- I think you want to use a different term than token replay
> in this paragraph.  If a RS is rebooted and loses all token information,
> there is nothing wrong with a C doing a replay of the token to the RS to get
> access to the resources as long as it is still valid.

I reworded the whole paragraph.

> 
> 46. Section 6 - Is the confidentiality protection a requirement for
> asymmetric keys as well?  -- Oh - and it may not be a session key, the
> session key for DTLS is established later, it is just a POP value (or key if
> asymmetric)

That issue relates to your point made in 9. I'll update the issue to 
consider this as well. I also replaced "session key" with 
"proof-of-possession key".

> 
> 47. Section 6 - Sentence structure of this paragraph is difficult.  The AS
> does not use shorter key sizes, it will perhaps create shorter POP keys.
> However, even this may be a problem depending on how short they are.  A
> tradeoff is going to occur here with the ability to record and later decrypt
> keys depending how short the keys are.

I've tried to rephrase that paragraph to clarify it. Please check again.

> 
> 47.  Section 6 - the next to last paragraph is a repeat - but probably
> clearer.

Fixed: I deleted the other paragraph.


> 
> 48.  Section 6 - Please explain the reasoning behind the last paragraph.  It
> does not make a great deal of sense to me.

If you issue an access token bound to a symmetric pop-key that is valid 
for a group of RS, then any of these RS that receives the token can use 
to towards the other RS, impersonating the client.

I'll try to clarify the text in the draft (I also noted that the context 
to using a symmetric pop key got lost in the second sentence of that 
paragraph).

> 
> 49.  Section 7 - I don't understand what you are trying to say with the last
> sentence in the second paragraph.  Given that the AS still needs to do ACL
> control, this does not make sense to me.
> 

In other grants, you have the authorization performed by the resource 
owner and the AS just validates the grant provided by the client. These 
grants can be used to hide the client's identity towards the AS.

I'll reformulate to clarify.

> For now I skipped IANA considerations and the appendixes.
> 
Noted, those are probably bound to change a bit when we extract cnf.

> Let me know if you would prefer that I place these items into the issue list
> on github or if you prefer doing it.
> 
> Jim
> 

I'm creating issues as I go, thanks for the offer.



-- 
Ludwig Seitz, PhD
Security Lab, RISE SICS
Phone [phone]