[Openid-specs-heart] Feedback on the HEART OAuth and OpenID Connect profiles from Mike Jones

Justin Richer jricher at mit.edu
Wed May 18 21:39:55 UTC 2016 

Software statements are defined in RFC7591:

https://tools.ietf.org/html/rfc7591#section-2.3

Essentially they lock down parts of a dynamic registration process by carrying client information with a trusted signature.

 — Justin


> On May 18, 2016, at 4:01 PM, Adrian Gropper <agropper at healthurl.com> wrote:
> 
> Thank you Mike and Justin!!! What is a software statement?
> 
> -Adrian
> 
> On Wednesday, May 18, 2016, Justin Richer <jricher at mit.edu <mailto:jricher at mit.edu>> wrote:
> Hi Mike,
> 
> Thanks again for the thorough review of the specifications. Comments and replies to each point are inline here. 
> 
>> On May 8, 2016, at 12:27 AM, Mike Jones <Michael.Jones at microsoft.com <javascript:_e(%7B%7D,'cvml','Michael.Jones at microsoft.com');>> wrote:
>> 
>> The 27 review feedback items below are based upon a complete read of the HEART OAuth and OpenID Connect profiles during the Implementer’s Draft review follows.  I believe that all the comments equally apply to the current working group drafts.  I’ve used links to the Implementer’s Drafts below since these are stable references to immutable versions.  They are intended to make the profiles clearer, more easily understood, more easily implemented, and more secure.
>>  
>> OAuth Profile - http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html>
>>  
>> http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.2.1.1 <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.2.1.1>
>> 2.1.1. <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.2.1.1> Full Client with User Delegation <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#FullClient>
>>  
>> (1)  Why is this called a “full client”?  Wouldn’t a more descriptive name be something more aligned with OAuth naming, like “Client using Authorization Code Flow” or with OpenID Connect naming, like “Basic Client”?
> 
> The name here comes from the original SecureRESTFUL project at the VA that originally developed the specifications, and we haven’t gotten any other feedback to change the names. Instead of naming it after the flow, we want it to reflect the nature of the client itself. Perhaps “User Interactive Client”?
> 
>>  
>> (2)  Especially in light of the OAuth mix-up attacks, wouldn’t it be better for these clients to use, or at least be allowed to use, “response_type=id_token code”, rather than be required to use “response_type=code”?
> 
> This is still a controversial approach in the OAuth WG, so I don’t think we should jump in and advocate support for it here. Since the delivery method for “id_token code” and “code” are completely different, I believe that it hurts interoperability to allow both for this profile. I would suggest a new profile to support the other methods. If “id_token code” were delivered in the same manner as “code”, that would be a different story, as the addition of the id_token would require very little change to the client application. 
> 
>>  
>> (3)  What is the “unique key” that is RECOMMENDED used for, how is it used, and what restrictions are there on the key type?  For instance, is it assumed to be an asymmetric key?  Are there required or recommended algorithms to use, such as “RS256”?  The spec should say more about this in the interests of both interoperability and clarity.
> 
> Thanks, this could be clearer. We intended this to be an asymmetric keypair generated on the device, with RS256 as the baseline for support. It also needs to be registered in JWK format, which is specified elsewhere but can be restated here.
> 
>>  
>> (4)  What are the implications of not having the “unique key” (not following the RECOMMENDED)?  Why is this RECOMMENDED, rather than REQUIRED?  For one thing, not having this key would seem to conflict with the requirement in 2.2 to use private_key_jwt client authentication.
> 
> The alternative is to have the key generated off-device and provisioned to it, not to have “no key” instead. This can be clarified, thanks!
> 
>>  
>> http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.2.1.2 <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.2.1.2>
>> 2.1.2. <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.2.1.2> Browser-embedded Client with User Delegation <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#BrowserClient>
>>  
>> (5)  Why is this called “Browser-embedded Client” when this flow could also be used by non-browser applications?  Why not follow the OAuth and Connect naming conventions and call this an “Implicit Client”?
> 
> It’s almost universally considered bad practice to use the implicit flow for anything but an in-browser client, and we very explicitly don’t want to encourage bad practices. Thus I think that “Browser-embedded Client” makes the most sense here.
> 
>>  
>> http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.2.1.3 <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.2.1.3>
>> 2.1.3. <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.2.1.3> Direct Access Client <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#DirectClient>
>>  
>> (6)  What is the use case behind the decision for the profile to include use of the Client Credentials flow, especially when this bypasses user involvement and user consent?  Is including this actually necessary?  Many servers don’t support this, by design.
> 
> Yes, we have use cases and drivers for this kind of access. Discussion is on the list archives for this.
> 
>>  
>> http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.2.2 <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.2.2>
>> 2.2. <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.2.2> Requests to the Token Endpoint <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#RequestsToTokenEndpoint>
>>  
>> (7) The text “MAY use other asymmetric signature methods listed in the JSON Web Algorithms (JWA <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#RFC7518> [RFC7518]) specification” should actually refer to the IANA JSON Web Signature and Encryption Algorithms registry athttp://www.iana.org/assignments/jose/jose.xhtml#web-signature-encryption-algorithms <http://www.iana.org/assignments/jose/jose.xhtml#web-signature-encryption-algorithms>– not the JWA spec.
> 
> Good point, thanks. I’m not sure how to include a reference to an IANA registry in xml2rfc, but we’ll fix that.
> 
>>  
>> (8)  The paragraph including “Authorization servers MAY require some clients to additionally authenticate using mutual Transport Layer Security (TLS) authentication” is problematic in several ways.  First, how does the server communicate to the client that mutual TLS is required?  How is the client TLS key registered with the server?  Is support for mutual TLS required of servers?  Of clients?  If it’s not required, how is interop assured when it is used?
>>  
> 
> This is something that’s not very well baked. In the original environment for the SecureRESTFUL project, there was a customer requirement to allow for mutual TLS authentication, even if we weren’t specifying how it will happen. I agree that it doesn’t add to interoperability, so I suggest to the WG that we strike this allowance and remain silent on it.
> 
>> http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.3 <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.3>
>> 3. <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.3> Client Registration <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#ClientRegistration>
>>  
>> (9)  The profile requires that “each client instance MUST receive a unique client identifier from the authorization server”.  This isn’t how most OAuth deployments work today – particular for native applications.  Is this significant departure from existing practice truly necessary or is this just a preference?  If it’s necessary, the specification should clearly say why.  If it’s not truly necessary, this requirement should be downgraded to being a recommendation.
> 
> We don’t really have public clients in HEART, which is what usually causes a client ID to be re-used for native applications. We do require dynamic registration be made available to support this.
> 
>>  
>> http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.3.2 <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.3.2>
>> 3.2. <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.3.2> Dynamic Registration <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#DynamicRegistration>
>>  
>> (10)  The profile requires the use of Dynamic Registration.  This is another significant departure from existing practice.  Like the previous comment, this requirement either needs to be justified as being essential or downgraded to being a recommendation.
> 
> See above. We want a brand new piece of HEART client software to be able to walk up to a HEART AS and register without going through a manual page. We realize it’s a significant departure and believe that it’s essential to HEART’s mission of wide interoperability and smooth integration.
> 
>>  
>> (11)  The profile indicates that servers MAY accept software statements.  How does the server communicate to clients that it supports software statements or not?  How does it communicate whether their use by clients is required or not?  How does it communicate to developers and clients what kinds of software statements will be accepted and rejected?
>>  
> 
> This isn’t fully baked, either in HEART or in the wider community. Software statements are a great idea but under defined, and we need to decide in HEART whether we want to lock that down, leave an allowance for it, or remain silent.
> 
>> (12)  The profile requires that authorization servers “MUST indicate to the end user that such a statement was used in the client's registration”.  This seems beyond silly, as most normal people would have no clue what such a message means or how they should respond to it (or the lack of it).  Requiring another “Click OK” barrier in the UI that people won’t understand and will just click through won’t help either usability or security.
>>  
> 
> We need better guidance on the UX process. Really, we want to tell authorization servers that they should flag whether a client was:
>  - dynamically registered
>  - dynamically registered with a trusted software statement
>  - statically registered through a self-service
>  - statically registered by an administrator
> 
> These are all different trust classes for clients, and each is valuable in the HEART space. We welcome better text for this section, especially backed by empirical research as opposed to anecdotal experience (which is what we’re working with at the moment).
> 
>> http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.4 <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.4>
>> 4. <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.4> OAuth 2.0 Server Profile <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#ServerProfile>
>>  
>> (13)  The profile requires that “The authorization server MUST limit each registered client (identified by a client ID) to a single grant type only”.  This may or may not be fine but doesn’t match existing practice for many implementations and the requirement isn’t justified in the specification.  At a minimum, clear rationale for this requirement needs to be included.
>>  
> 
> This fits with the client profile definitions, and we can add better rationale for it. It’s largely security based.
> 
>> http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.4.1 <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.4.1>
>> 4.1. <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.4.1> Discovery <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#Discovery>
>>  
>> (14)  The profile requires support for the introspection endpoint.  This unnecessarily increases the implementation footprint of both clients and servers, increases run-time cost of common operations, and is a significant deviation from most deployments.  If, for instance, the UMA profile requires introspection for some reason, add it there, but do not require HEART OAuth or Connect to support introspection.  This must be removed from the OAuth profile.
> 
> Discussed in the other thread, this requirement will not be removed. Better justification can be provided.
> 
>>  
>> (15)  The profile requires support for the revocation endpoint.  This unnecessarily increases the implementation footprint of both clients and servers and is a significant deviation from most deployments.  If, for instance, the UMA profile requires revocation for some reason, add it there, but do not require HEART OAuth or Connect to support revocation.  This must be removed from the OAuth profile.
> 
> For reasons similar to the introspection endpoint, this function is made available to clients that want to behave proactively. Better justification can be supplied as well.
> 
>>  
>> (16)  The profile states “It is RECOMMENDED that servers provide cache information through HTTP headers and make the cache valid for at least one week.”  How does this interact with the need to immediately rotate keys upon key compromise?  Is this saying that a compromised key must remain valid for at least a week?  If this isn’t implied by this recommendation, please explain what is actually implied for clients and servers.
>>  
> 
> We’re open to better cache guidance. Cache consistency is hard, since we don’t want clients and resources to fetch the AS key every single request (I’ve seen naive clients do just that), cache is a good idea. But if someone’s compromised the key, you’ll probably want some type of cache-busting signal, like what RISC is talking about for account compromise. 
> 
>> http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.4.2 <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.4.2>
>> 4.2. <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.4.2> JWT Bearer Tokens <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#JWTBearerTokens>
>>  
>> (17)  The spec is unclear what kinds of bearer tokens are being referred to here.  Is it saying that access tokens must be JWTs?  Is it saying that refresh tokens must be JWTs?  This is certainly a reasonable implementation choice but the specification does not clearly state why this is a requirement.  Please say why bearer tokens must be JWTs.
> 
> The draft specs do better at this but I’m sure this can be clearer. The JWT format was used to allow resources to have a first-order validity check against tokens without having to call introspection (which is still needed for token active status as well as privacy-sensitive data like user ID, scopes, etc). This also allows an RS to accept tokens issued by multiple RS’s, which has been raised as a desirable deployment mode for HEART systems.
> 
>>  
>> (18)  The profile says that an audience may be included.  Why is the audience not required?  What are the implications of sending and accepting bearer tokens that aren’t audience?
>>  
> 
> The problem is that it’s hard to specify audience in OAuth right now, so there’s a lot of friction in requiring its inclusion. We’re going to be specifying some audience components in the FHIR/OAuth spec, which would fit here. This relaxed restriction can be made clearer.
> 
>> (19)  The profile requires including “azp” in JWT bearer tokens.  It’s now widely held by the OpenID Connect working group that “azp” is both underspecified and its inclusion in OpenID Connect was a mistake.  (Seehttps://bitbucket.org/openid/connect/issues/973/core-2-3137-azp-claim-underspecified-and <https://bitbucket.org/openid/connect/issues/973/core-2-3137-azp-claim-underspecified-and> - Core 2 / 3.1.3.7 - azp claim underspecified and overreaching.)  Please remove all uses of “azp” from the HEART specifications.  If the client ID needs to be conveyed, an appropriate new well-specified claim should be defined and used for this purpose.
> 
> Except that we’re defining the “azp” to *be* the client ID, so I don’t see a problem in using that field. We could easily just make it “client_id” to convey the information from the introspection endpoint.
> 
>>  
>> http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.5 <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.5>
>> 5. <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#rfc.section.5> Requests to the Protected Resource <http://openid.net/specs/openid-heart-oauth2-1_0-ID1.html#RequestsToProtectedResource>
>>  
>> (20)  The profile says that resources may support the query-parameter parameter method from RFC 6750.  This is a terrible security mistake.  Change the text to say that “Resources MUST NOT support the query-parameter parameter method from RFC 6750”.
>>  
> 
> The security parameter has its use cases with appropriate caveats. I don’t see a reason to remove it.
> 
>> OpenID Connect Profile - http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html <http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html>
>>  
>> http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html#rfc.section.2 <http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html#rfc.section.2>
>> 2. <http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html#rfc.section.2> ID Tokens <http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html#IDTokens>
>>  
>> (21)  The profile says that “The ID Token MUST expire and SHOULD have an active lifetime no longer than five minutes”.  While this expiration time may be a reasonable implementation choice, the specification is silent on why this reasonably short expiration time is a requirement.  Please either justify this requirement or downgrade this text to a non-normative discussion on criteria for choosing appropriate ID Token lifetimes.
>>  
> 
> All the tokens have recommended lifetimes, just like this one. We can justify this easily — it’s an authentication event assertion, you use it once and chuck it. Five minutes is generous.
> 
>> http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html#rfc.section.3 <http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html#rfc.section.3>
>> 3. <http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html#rfc.section.3> UserInfo Endpoint <http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html#UserInfoEndpoint>
>>  
>> (22)  The profile requires supporting signed UserInfo responses.  This may be a reasonable thing for the profile to require but the specification fails to motivate this requirement.  Please do so.
> 
> It’s to allow for a higher-security mode, optional for the client. We can provide better justification.
> 
>>  
>> (23)  The profile talks about encrypted UserInfo responses but doesn’t say whether support for these is required for clients or servers, or why.  Please do so.
> 
> Same as above. Plus, all HEART OIDC clients have keys registered, so encryption is generally possible here where it’s less tenable in the general OIDC case.
> 
>>  
>> http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html#rfc.section.4 <http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html#rfc.section.4>
>> 4. <http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html#rfc.section.4> Request Objects <http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html#RequestObjects>
>>  
>> (24)  The profile requires that servers support request objects.  This may be a reasonable thing for the profile to require but the specification fails to motivate this requirement.  Please do so.
>>  
> 
> Justification can be added: To support higher security modes optionally from the clients
> 
>> http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html#rfc.section.5 <http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html#rfc.section.5>
>> 5. <http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html#rfc.section.5> Authentication Context <http://openid.net/specs/openid-heart-openid-connect-1_0-ID1.html#AuthenticationContext>
>>  
>> (25)  The profile uses URIs for FICAM LOA numbers as “acr” values.  This seems short-signed and limiting, as these values are specific to the United States and not international in nature.  These must be changed to appropriate international equivalents – especially in coordination with other working groups, such as MODRNA.
> 
> This part is known to be woefully incomplete. We want to provide implementations with a list here, but such a list doesn’t exist that I’m aware of.
> 
>>  
>> (26)  Remove the requirement to support the introspection endpoint from this profile, for the same reasons as in (14).
>>  
> 
> This is now no longer a requirement as per the new “conformance” section.
> 
>> (27)  Remove the requirement to support the revocation endpoint from this profile, for the same reasons as in (15).
> 
> Since this is how the client interacts with the AS, the requirement currently carries through, with the same justification.
> 
>  — Justin
> 
>>  
>>                                                           Thanks all,
>>                                                           -- Mike]
>>  
>> _______________________________________________
>> Openid-specs-heart mailing list
>> Openid-specs-heart at lists.openid.net <javascript:_e(%7B%7D,'cvml','Openid-specs-heart at lists.openid.net');>
>> http://lists.openid.net/mailman/listinfo/openid-specs-heart <http://lists.openid.net/mailman/listinfo/openid-specs-heart>
> 
> 
> -- 
> 
> Adrian Gropper MD
> 
> PROTECT YOUR FUTURE - RESTORE Health Privacy!
> HELP us fight for the right to control personal health data.
> DONATE: http://patientprivacyrights.org/donate-2/ <http://patientprivacyrights.org/donate-2/>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openid.net/pipermail/openid-specs-heart/attachments/20160518/c9045f24/attachment.html>

More information about the Openid-specs-heart mailing list