[Openid-specs-ab] Native SSO spec feedback

[George Fletcher]

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.
>
>     > 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.
>
> *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.
>
> *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.
>
> 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.

> 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.
>
> 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
>
> *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?
>
> *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/20190228/217ba346/attachment.html>