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

Adrian Gropper agropper at healthurl.com
Wed May 18 21:01:53 UTC 2016 

Thank you Mike and Justin!!! What is a software statement?

-Adrian

On Wednesday, May 18, 2016, Justin Richer <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
> 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
> 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
> 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
> 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 at
> 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
> 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
> 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
> 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
> 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
> 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.  (See
> 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
> 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
> 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
> 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
> 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
> 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
>
>
>

-- 

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/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openid.net/pipermail/openid-specs-heart/attachments/20160518/ee22c735/attachment-0001.html>

More information about the Openid-specs-heart mailing list