Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AD re-review of -10 #30

Merged
merged 1 commit into from May 11, 2020
Merged

AD re-review of -10 #30

merged 1 commit into from May 11, 2020

Conversation

kaduk
Copy link
Contributor

@kaduk kaduk commented Apr 28, 2020

Not much here, though a couple points I'll leave some further comments on, as the proper disposition is not entirely clear.

@@ -255,7 +255,7 @@
</figure>

<t>
If the client wants to update its access rights without changing an existing OSCORE Security Context, it MUST include in its POST request to the token endpoint a req_cnf object. The req_cnf MUST include a kid field carrying a bstr-wrapped CBOR array object containing the client's identifier (assigned as discussed in <xref target="as-c"/>) and optionally the context identifier (if assigned as discussed in <xref target="as-c"/>). The CBOR array is defined in <xref target="kid"/>, and follows the notation of <xref target="RFC8610"/>. These identifiers, together with other information such as audience, can be used by the AS to determine the shared secret bound to the proof-of-possession token and therefore MUST identify a symmetric key that was previously generated by the AS as a shared secret for the communication between the client and the RS. The AS MUST verify that the received value identifies a proof-of-possession key that has previously been issued to the requesting client. If that is not the case, the Client-to-AS request MUST be declined with the error code 'invalid_request' as defined in Section 5.6.3 of <xref target="I-D.ietf-ace-oauth-authz"/>.
If the client wants to update its access rights without changing an existing OSCORE Security Context, it MUST include in its POST request to the token endpoint a req_cnf object. The req_cnf MUST include a kid field carrying a bstr-wrapped CBOR array object containing the client's identifier (assigned as discussed in <xref target="as-c"/>) and the context identifier (if assigned as discussed in <xref target="as-c"/>). The CBOR array is defined in <xref target="kid"/>, and follows the notation of <xref target="RFC8610"/>. These identifiers, together with other information such as audience, can be used by the AS to determine the shared secret bound to the proof-of-possession token and therefore MUST identify a symmetric key that was previously generated by the AS as a shared secret for the communication between the client and the RS. The AS MUST verify that the received value identifies a proof-of-possession key that has previously been issued to the requesting client. If that is not the case, the Client-to-AS request MUST be declined with the error code 'invalid_request' as defined in Section 5.6.3 of <xref target="I-D.ietf-ace-oauth-authz"/>.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(We had made a similar change later in the document already, so this just makes them consistent again.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@@ -455,7 +455,7 @@ A5 # map(5)


<t>
If the client has requested an update to its access rights using the same OSCORE Security Context, which is valid and authorized, the AS MUST omit the 'cnf' parameter in the response, and MUST carry the client identifier and the context identifier (if it was set and included in the initial access token response by the AS) in the 'kid' field in the 'cnf' parameter of the token, with the same structure defined in <xref target="kid"/>. These identifiers need to be included in the response, in order for the RS to identify the previously generated Security Context.
If the client has requested an update to its access rights using the same OSCORE Security Context, which is valid and authorized, the AS MUST omit the 'cnf' parameter in the response, and MUST carry the client identifier and the context identifier (if it was set and included in the initial access token response by the AS) in the 'kid' field in the 'cnf' parameter of the token, with the same structure defined in <xref target="kid"/>. These identifiers need to be included in the token in order for the RS to identify the previously generated Security Context.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check this, but I'm pretty sure this has to be in the token, since the token is all the RS has available when it's determining whether this is an update or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right. "in the response" was not incorrect (it needs to be in the response), but your change makes the sentence more precise. 👍

@@ -652,7 +652,7 @@ OSCORE_Security_Context = {
The client MUST generate a nonce value very unlikely to have been previously used with the same input keying material. This profile RECOMMENDS to use a 64-bit long random number as nonce's value. The client MUST store the nonce N1 as long as the response from the RS is not received and the access token related to it is still valid. The client MUST use CoAP and the Authorization Information resource as described in section 5.8.1 of <xref target="I-D.ietf-ace-oauth-authz"/> to transport the token and N1 to the RS.
</t>
<t>
Note that the use of the payload and the Content-Format is different from what described in section 5.8.1 of <xref target="I-D.ietf-ace-oauth-authz"/>, which only transports the token without any CBOR wrapping. In this profile, the client MUST wrap the token and N1 in a CBOR map. The client MUST use the Content-Format "application/ace+cbor" defined in section 8.14 of <xref target="I-D.ietf-ace-oauth-authz"/>. The client MUST include the access token using the correct CBOR label (e.g., "cwt" for CWT, "jwt" for JWT) and N1 using the 'nonce1' parameter defined in <xref target="nonce1"/>.
Note that the use of the payload and the Content-Format is different from what described in section 5.8.1 of <xref target="I-D.ietf-ace-oauth-authz"/>, which only transports the token without any CBOR wrapping. In this profile, the client MUST wrap the token and N1 in a CBOR map. The client MUST use the Content-Format "application/ace+cbor" defined in section 8.14 of <xref target="I-D.ietf-ace-oauth-authz"/>. The client MUST include the access token using the "access_token" parameter and N1 using the 'nonce1' parameter defined in <xref target="nonce1"/>.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one I'm not sure about. The main thing I care about is consistency between this and the example, which does not use anything that I can see as being a "cwt" or "jwt" label. I'm not sure what these labels would be or where the would be registered, either; combined with the fact that the RS probably has a good idea what kind of token its going to get anyway, this change seems more likely to be correct than any alternatives I can come up with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! I remember making the change to the example but did not change here as well... Yes it should be access_token, as defined in the framework.

@@ -773,7 +773,7 @@ token.
</t>

<t>
If the exchange was an update of access rights, i.e., a new Security Context was derived from a client that already had a Security Context in place, the RS is RECOMMENDED to delete the old Security Context after OSCORE verification and verification of access rights succeed. The RS MUST delete the Security Context if it deletes the access token associated to it.
If the exchange was an update of access rights, i.e., a new Security Context was derived from a client that already had a Security Context in place, the RS is RECOMMENDED to delete the old Security Context after OSCORE verification succeeds. The RS MUST delete the Security Context if it deletes the access token associated to it.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do see that the previous paragraph talks about "verification of access rights" which in some sense is parallel to the structure I'm removing here, but IIUC that paragraph is considering the generic case of "verify requests and send responses to C using OSCORE"; this paragraph seems to be specific to the "get a new token and update access rights" operation, which does not have the same type of ongoing access rights verification to perform.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this paragraph is specific to update of access right, but I think this text should not be removed. What I want to say is that the old security context is recommended to be deleted from the RS only after successful processing of a request with that new security context, and that includes both OSCORE verification and access right verification.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like a reasonable behavior to recommend, but IIUC it seems like it would involve a period where two tokens are stored -- one "active" and one "provisional" since the opportunity to process a request with the new security context would not occur in the same transaction as the authz-info endpoint upload. Do you want to add some more text about this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, about 2 tokens being stored at the same time, that is covered by the ace framework and reported in this document (in section 4.2):

As specified in section 5.8.1 of [I-D.ietf-ace-oauth-authz], when
receiving an updated access token with updated authorization
information from the client (see Section 3.1), it is recommended that
the RS overwrites the previous token, that is only the latest
authorization information in the token received by the RS is valid.
This simplifies for the RS to keep track of authorization information
for a given client.

So no, only one token is RECOMMENDED to be kept, and that happens before the new security context is derived and "approved" (so before 2 Sec ctx exist at the same time in the RS).
Do we need some more text about this? I am not really sure what is missing... Also looking back at the open issues, I am noticing that #21 by @jimsch might be somewhat related to this (this paragraph was added to answer that issue).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to consider the series of events for this "updated authorization information" flow.
Presuming we start in a situation where the client has a valid token and security context with the RS, and has just obtained an updated token (and new key) from the AS. Then:

  • the client posts the new token to the authz-info endpoint
  • the RS validates the token (signature by AS, etc.) and notes that this token uses a different key than the current token/security context
  • ???
  • the RS receives an OSCORE request with the new key and is able to validate the proof of possession. At this stage it is appropriate to delete the old token and security context (and also to validate the access rights for the OSCORE request providing the proof of possession).

I'm not sure what happens in the "???", where the RS knows that there's a new token+key but doesn't have a proof of possession for it yet. I don't see a way for the RS to escape storing two tokens during that time without being trivially vulnerable to a DoS attack.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't know in ??? how the RS determines that it is the same client.

Copy link
Contributor

@fpalombini fpalombini May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaduk, you are correct, RS would have had to keep 2 tokens (one valid, one not yet valid) for the amount of time it takes to validate the OSCORE request.
I have a proposal that I posted to the list as it is a big enough change that I wanted to get feedback there: https://mailarchive.ietf.org/arch/msg/ace/dLkW-MYHXfZqmtY7AP7ZBDJpxOw/

@@ -505,7 +505,7 @@ A5 # map(5)
<section anchor="oscore-sec-ctx" title="OSCORE_Security_Context Object">

<t>
An OSCORE_Security_Context is an object that represents part or all of an OSCORE Security Context, i.e., the local set of information elements necessary to carry out the cryptographic operations in OSCORE (Section 3.1 of <xref target="RFC8613"/>). In particular, the OSCORE_Security_Context object is defined to be serialized and transported between nodes, as specified by this document, but can also be used in this way by other specifications if needed. The OSCORE_Security_Context object can either be encoded as a JSON object or as a CBOR map. The set of common parameters that can appear in an OSCORE_Security_Context object can be found in the IANA "OSCORE Security Context Parameters" registry (<xref target="sec-ctx-params-reg"/>), defined for extensibility, and is specified below.
An OSCORE_Security_Context is an object that represents part or all of an OSCORE Security Context, i.e., the local set of information elements necessary to carry out the cryptographic operations in OSCORE (Section 3.1 of <xref target="RFC8613"/>). In particular, the OSCORE_Security_Context object is defined to be serialized and transported between nodes, as specified by this document, but can also be used as a representation of Security Context state by other specifications if needed. The OSCORE_Security_Context object can either be encoded as a JSON object or as a CBOR map. The set of common parameters that can appear in an OSCORE_Security_Context object can be found in the IANA "OSCORE Security Context Parameters" registry (<xref target="sec-ctx-params-reg"/>), defined for extensibility, and is specified below.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the editorial improvement, but I have a problem with calling it "a representation of Security Context state", as the OSCORE_Security_Context does not contain all the informations of a full Security Context, for example no informations about Replay Window or sequence numbers, or derived keys... what about:

OLD:
but can also be used in this way by other specifications if needed.

NEW:
but can also be used by other specifications if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should work, thanks. The "in this way" is what was bothering me, as you surmised :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want me to update the PR or can you handle things from your end?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do it, thanks! :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6df6563.

fpalombini added a commit that referenced this pull request May 4, 2020
@fpalombini
Copy link
Contributor

If you agree @kaduk I would go ahead and merge this PR (excluding the "update of access rights" change at l.776).

@kaduk
Copy link
Contributor Author

kaduk commented May 9, 2020

If you agree @kaduk I would go ahead and merge this PR (excluding the "update of access rights" change at l.776).

Sorry, I missed this when it came in. Please go ahead.

@fpalombini fpalombini merged commit e9bad52 into ace-wg:master May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants