[Openid-specs-ab] Native SSO spec feedback
gffletch at aol.com
Wed Jul 3 18:44:05 UTC 2019
Sigh... I'm just getting to updating the spec. I'll share updated XML
and HTML later today.
On 2/28/19 11:44 AM, Filip Skokan wrote:
> You're welcome George, please find my feedback inline.
> 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
> 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
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
>> *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
> 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.
>> > 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
>> 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
>> *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!*
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the Openid-specs-ab