Hi Frederick,
thanks very much for your review! Here is our feedback:
> As written, the spec does not normatively require replay protection
for PoPs to actually be implemented by the verifier. Is this intentional?
That is intentional and mirrors DPoP's design by giving the server the
freedom to choose to cache previously seen values (e.g., jti) or not. We
are recommending Replay detection in this draft, but do not want to
mandate it to give deployments the freedom.
> Even though trust management is explicitly out of scope, I think
there should be a normative statement that an AS which trusts multiple
attesters must somehow scope the accepted client_ids to each attester to
prevent multiple attesters from asserting the same client_id.
That sounds like a requirement with a rather specific deployment in
mind. Given that a client attestation requires special integration with
the client and the communication is out of scope of this document, we do
believe it makes sense to keep the client attester relationship out of
scope as well and as something that gets defined for specific use-cases.
> I think there might be a kind of mild "downgrade" attack allowing a
mode switch from "normal" to "combined" by removing the PoP header. The
severity of this is probably pretty limited, but it feels fragile to
signal combined mode only by the absence of the
OAuth-Client-Attestation-PoP header.
It doesn't really change the security posture of the situation since it
is only valid if DPoP is bound to the same public key.
> I think iat should be REQUIRED. Otherwise, the AS cannot actually
determine how fresh an attestation is, and has to exclusively rely on
the exp from the attester which the AS does not control.
While this is a breaking change, we believe that this change makes sense
and propose to switch the `iat` value of Client Attestation JWT from
OPTIONAL to REQUIRED to always allow the AS/RS to make their own policy
decision. This would also match better with the`use_fresh_attestation`
error.
> I didn't implement combined mode, but I think that there should be a
line explicitly saying that a server implementing combined mode SHOULD
also provide DPoP nonces (which are optional in RFC 9449).
We currently do not have a recommendation that challenges should be
used, leaving the server the decision to either use challenges or rely
purely on `jti`. Given that we are not recommending challenges, we
should not recommend nonces.
> The text is not currently clear on the fact that a client instance
needs to remember which client instance key each instance-bound protocol
artifact (i.e. refresh tokens) is actually bound to. If it doesn't do
this, changing client instance keys will result in bad user experiences
(potentially unbounded retries). Usually the client instance key
rotation is not the choice of the client instance, but is something the
platform does for your e.g. when biometrics are reset/updated, so this
is something that will happen to most implementations. Guidance would be
helpful.
We will add a note / implementation consideration that clients should
manage what artefacts are bound to what keys.
> It would be helpful for interoperability to define the status codes
that should be used for use_fresh_attestation and
invalid_client_attestation errors.
Error responses in OAuth default (mostly) to status code 400. It might
be beneficial to make this explicit and define specific codes like 400
for use_fresh_attestation and 401 for invalid_client_attestation? We
think it may be good to be more explicit on error codes for the AS, but
unsure whether we should say something about RS. WG feedback is welcome.
TLDR:
- we propose a breaking change to make `iat` of Client Attestation JWT
required
- we add a note to ensure clients keep their keys that artefacts are
bound to
- we welcome wg feedback whether to define HTTP status codes for the
errors defined in this spec
Best regards,
Paul + Christian + Tobias
_______________________________________________
OAuth mailing list -- [email protected]
To unsubscribe send an email to [email protected]
