[Openid-specs-ab] Native SSO spec feedback

[George Fletcher]

Sigh... I'm just getting to updating the spec. I'll share updated XML 
and HTML later today.

Comments inline...

On 2/28/19 11:44 AM, Filip Skokan wrote:
> You're welcome George, please find my feedback inline.
>
> Best,
> *Filip*
>
>
> On Thu, 28 Feb 2019 at 16:56, George Fletcher <gffletch at aol.com 
> <mailto:gffletch at aol.com>> wrote:
>
>     First, thank you very much for your feedback and I sincerely
>     apologize for not getting back to you sooner!
>
>     Comments inline.
>
>     On 1/18/19 5:46 AM, Filip Skokan wrote:
>>     Hello George,
>>
>>     I've re-read the document and updated my past feedback draft.
>>     Here goes
>>
>>     *Section 3.2 *
>>
>>         > and as such it is RECOMMENDED that the device secret be
>>         implemented as a JWE encrypted for the
>>         > Authorization Server.
>>
>>
>>     This recommendation suggests that the AS does not store the
>>     device secret state, why is JWE?? recommended over any opaque
>>     value representing the device secret state stored on AS side?
>     We can relax this to allow for a client only vs server state
>     solution. As to JWE, I often recommend it because it makes it easy
>     to do key rotation and good security hygiene says keys should be
>     rotated. That say, I agree that we don't have to be prescriptive
>     around how in that the value is opaque to the client.
>
>
> FS: ????
>
>>
>>         > The client MAY provide the "device_secret" to the /token
>>         endpoint during refresh token requests.
>>
>>
>>     Why? What's the AS supposed to do, refresh the device secret too?
>>     Validate it as well? Should the AS rotate the secret, is the
>>     client expected to update the shared storage with every new
>>     device secret it receives? Given the previous recommendation to
>>     use a JWE, how to handle rotation or revocation, if it's just an
>>     opaque value leading to a record on the side, should the used
>>     ones be marked as consumed and when re-used revoke the grant as a
>>     whole?
>     So we have the requirement that the server should be able to
>     update the device secret if needed. This provides a way for the
>     device secret to be "rotated". As to issuing of access_tokens in
>     the refresh_token grant flow, then I'd suggest that the
>     device_secret represent the device to which the refresh_token was
>     issued. This creates a tighter binding between the refresh_token
>     and the device.
>
>     Yes, if an "old" device_secret is used, I would reject the entire
>     /token grant_type flow.
>
>     Note that support for other grant_types other than refresh_token
>     may require device_secret validation.
>
>     Also, my expectation is that device_secrets contain a unique id
>     (like a jti) that can be used on the server side for management,
>     blacklisting, etc.
>
>
> FS: If the intention is for the server to rotate it using its policies 
> it should be required to be present. e.g. if a refresh token is 
> received at the token endpoint which has the "device_sso" scope the 
> device_secret is a required parameter? Too strict? Dunno, but this way 
> it looks like it's up to the client to control the rotation by 
> including it or not. That way, as an implementer i'm forced to load 
> the device_secret by either reference or other means (decode/decrypt) 
> and i can easily confirm the refresh token and device secret belong to 
> one another and that the device secret is not expired / revoked or 
> similar.
I'm updating the spec to say SHOULD instead of MAY. If there is general 
agreement, I don't mind making it a MUST in the case of a refresh_token 
that "contains" a device_sso scope.
>
>>
>>     *Section 3.3*
>>
>>     Why would an app already in possession of a device_secret go
>>     through browser based code flow when it can do token exchange? If
>>     that one was rejected, then a browser based code flow? And the
>>     point of providing the previously rejected device_secret is?
>     It's required so that the id_token can be issued with the
>     device_secret hash (ds_hash). This allows the id_token,
>     access_token and refresh_token to be bound to the device_secret.
>     This constrains these tokens to that device.
>
>
> FS: I'm not sure I follow. I know ID Token being returned from refresh 
> token grant or token exchange in this case is optional, but still, as 
> a client if I get a device secret from the shared storage my instinct 
> reaction is to do the token exchange, not to start a regular flow. It 
> just looks to me like this is out there without an explanation on WHY 
> a client would ever do a regular auth flow when it's already in 
> possession of a device secret.
GFF: So if a device_secret and id_token exist when the native app loads, 
it doesn't need to do the initial browser based flow. The only time the 
browser based flow is required is if the device_secret and associated 
id_token are NOT present on the device or the user want to login with a 
different identity.
>
>>
>>     *Section 3.4*
>>
>>         > ds_hash
>>         > The exact binding between the ds_hash and device_secret is
>>         not specified by this profile. As this
>>         > binding is managed solely by the Authorization Server, the
>>         AS can choose how to protect the
>>         > relationship between the id_token and device_secret.
>>
>>
>>     Why not use the same boilerplate used for other *_hash ID Token
>>     properties?
>     We can. Partly because if the device_secret contains a randomly
>     generated unique id (e.g. jti) then it might not really be
>     required to hash the whole device secret. I should probably update
>     the spec with this and am also interested in your thoughts.
>
>
> FS: The device_secret being either opaque or JWT it's always unique, 
> else it doesn't serve it's purpose right. Ergo, i'd say the ds_hash is 
> calculated the same way other hash properties are.
I think one problem here is that if the device_secret is updated, it 
will have a different hash value and now the ds_hash in the id_token 
won't validate against the device_secret. I want to keep the binding but 
not require issuing new id_tokens when the device_secret changes. In 
this context, maybe hash is the wrong working and we should change the 
claim name to 'ds_id' or something like that.
>
>>
>>     e.g.
>>
>>         > OPTIONAL. Device Secret hash value. Its value is the
>>         base64url encoding of the left-most half of
>>         > the hash of the octets of the ASCII representation of the
>>         device_secret value, where the hash
>>         > algorithm used is the hash algorithm used in the alg Header
>>         Parameter of the ID Token's JOSE
>>         > Header. For instance, if the alg is RS256, hash the
>>         device_secret value with SHA-256, then take
>>         > the left-most 128 bits and base64url encode them. The
>>         ds_hash value is a case sensitive string.
>>
>>
>>     *Section 4.1*
>>
>>
>>         > The client MUST use the HTTP Basic Authentication method
>>         from RFC
>>         > 6749 to authenticate the request to the token endpoint.
>>
>>     1) client_secret_basic without client_secret makes little sense
>>     and in general an AS will issue a client secret when client's
>>     token_endpoint_auth_method is client secret based.
>     So what is the preferred way for a PKCE client to present it's
>     client_id? Given the current token exchange spec, I believe the
>     only way is in the HTTP Authorization header. Happy for
>     suggestions here.
>
>
> FS: OIDC Core Section 9 gives a name to such auth method, it's "none" 
> and the client_id is provided in the body of the request and the Token 
> Exchange spec also mentions "Client authentication to the 
> authorization server is done using the normal mechanisms provided by 
> OAuth 2.0.". Again this goes to the next point, i'd just stick to the 
> boilerplate and not force a client to use a specific authentication 
> scheme.
>
>
>>     2) I wouldn't specify the required authentication method at the
>>     token endpoint, i'd say something along the lines of "the client
>>     authenticates using its registered token endpoint authentication
>>     method". Albeit this is most likely going to be "none" but we
>>     shouldn't rule out dynamically registered clients with secrets or
>>     private_key_jwt auth.
>     This is a good point as I'd like to look at how this works with
>     dynamic client registrations using private_key_jwt. I'll see about
>     updating the text accordingly.
>
>
> FS: ????
>
>>
>>     Furthermore any AS implementation probably has the "client
>>     authentication" abstracted before any specific grant type
>>     processing and it would be a hassle to add extra auth processing
>>     based on the grant type.
>     Fair point.
>>
>>     *dtto in 4.2*, the example should be a non-normative example
>
GFF: I consider all examples non-normative. Should I explicitly state that?
>
>>
>>     *Section 4.3*
>>
>>         > If that session expires, all refresh_tokens associated with
>>         it MUST be invalidated.
>>
>>
>>     Regardless of present offline_access scope and consent on the
>>     initial authorization request?
>     Good point. I need to address the 'offline_access' scope scenario.
>     I believe that even in an offline_access scenario, the server can
>     invalidate the refresh_token so the concept needs to stay.
>     Effectively, there needs to be a way for the id_token to reference
>     something for the long-lived "session" such that if that session
>     is revoked, the token exchange failed.
>
>     session_id could possibly be replaced with refresh_token_id for
>     the offline_access case. Thoughts?
>
>
> FS: I probably skimmed though 4.3 the first time, because point of it 
> 2 says the token endpoint should verify binding between an id token 
> and a device secret. Where did id_token come from? The client doesn't 
> send one, just sends the refresh token and possibly? the device secret.
GFF: the id_token is specified in the 'subject_token' parameter of the 
token exchange profile
>
>>
>>     *Section 4.4*
>>
>>       * expires_in should be RECOMMENDED or OPTIONAL to be in line
>>         with the rest of the specifications
>>       * refresh_token REQUIRED? The point is to obtain an ID and
>>         Access tokens, i'd expect issuing a refresh token is left to
>>         the AS policy discretion
>>
>     Yes, we should leave it up to the AS. What I'd like to describe is
>     that if the new client SHOULD receive a refresh_token for it's own
>     flows, then the AS needs to provide one because the refresh_token
>     is issued to the new client_id. That said, there could be some
>     flows (though unlikely in a mobile environment) where the AS does
>     NOT issue refresh_tokens so it shouldn't be REQUIRED.
>>
>>     Example response has a few nits
>>
>>       * has "Issued_token_type" property name (capital I)
>>       * should be marked as non-normative
>>       * token_type although case-insensitive value should be "Bearer"
>>         to be inline with the rest of the specs
>>
>     Thanks!
>>
>>     *Section 6.1. *
>>
>>     `sid` would already be registered by both front and back-channel
>>     specs, wouldn't it?
>>
>>     *6.2.1. is probably a leftover copy-paste*
>     *Thank you!! This is very helpful!*
>>
>>     Best,
>>     Filip
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openid.net/pipermail/openid-specs-ab/attachments/20190703/3ddb2dfd/attachment.html>