Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add resource parameter to PAR #301

Merged

Conversation

spheroid
Copy link
Contributor

@spheroid spheroid commented Aug 20, 2024

As recommended by the OpenID4VCI draft 14, use RFC8707 resource parameter when requesting issuance using scope parameter.

The exact phrasing in the draft reads:

If the Credential Issuer metadata contains an authorization_servers property, it is RECOMMENDED to use a resource parameter.

The information about existence of authorization_servers property is unfortunately lost in the TO → domain conversion, so it's not taken into account.

This fix has already been applied to the iOS library in eu-digital-identity-wallet/eudi-lib-ios-openid4vci-swift#56.

Closes #302

As recommended by the OpenID4VCI draft 14, use RFC8707 resource parameter when requesting issuance using scope parameter.

The exact phrasing in the draft reads "If the Credential Issuer metadata contains an authorization_servers property, it is RECOMMENDED to use a resource parameter". However, this information is lost in the TO->domain conversion, so it's not taken into account.
@spheroid spheroid requested a review from a team as a code owner August 20, 2024 16:04
Copy link
Contributor

@babisRoutis babisRoutis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear @spheroid

Thanks for this.

In general, we can understand whether the credential issuer is a resource server that uses another authorization server or whether is both resource & authorization server by comparing the credentialIssuerId and the authorizationIssuer attributes.

So, please check my comments

  • Introduce a new function isCredentialIssuerAuthorizationServer()
  • Use it, to identify
    • If location attribute has to be populated in the authorization_details (your last change)
    • if resource attribute needs to be populated in the PAR (your first change)
    • if resource attribute needs to be populated in the simple authorization request (not PAR) at line 190

@babisRoutis babisRoutis added the feature New feature or request label Aug 21, 2024
@babisRoutis babisRoutis added this to the openid4vci-kt v0.4.0 milestone Aug 21, 2024
@spheroid
Copy link
Contributor Author

Thanks for quick reply, a few thoughts:

In general, we can understand whether the credential issuer is a resource server that uses another authorization server or whether is both resource & authorization server by comparing the credentialIssuerId and the authorizationIssuer attributes.

Technically this is against the specification, as it states (emphasis mine):

If the Credential Issuer metadata contains an authorization_servers property, it is RECOMMENDED to use a resource parameter [RFC8707] whose value is the Credential Issuer's identifier value to allow the Authorization Server to differentiate Credential Issuers.

I understand this is hard to determine this way as the information is lost in the domain conversion and to me it feels like you're suggesting to capture the "spirit of the specification" instead of literally implementing it. I think there's two more alternatives to consider:

  • Using the resource parameter always when scope is used, as it's currently implemented in this PR. Does it hurt that we're redundantly including the parameter? I wasn't able to find any harmful consequences from the RFC nor the OpenID4VCI spec. It does add one extra parameter that increases the HTTP request payload, though.
  • Determining the "issuer as authorization server" value literally as stated in the draft (i.e. resolving it during JSON parsing and exposing it in the domain object).

I know I'm not in the position to demand how it is to be implemented, but wanted to point out my opinion and the alternatives.

  • If location attribute has to be populated in the authorization_details (your last change)
  • if resource attribute needs to be populated in the PAR (your first change)
  • if resource attribute needs to be populated in the simple authorization request (not PAR) at line 190

Good catch! This issue came up during interop and obviously I was focused on the code paths related to our use case. 😄

Instead of separately determining whether the credential issuer works as the authorization issuer in every use case, define a single property that does it.
@babisRoutis
Copy link
Contributor

babisRoutis commented Aug 21, 2024

Dear @spheroid ,

I don't agree with your assessment 😄

AuthorizationEndpointClient is being instantiating by providing - among other attributes :

  • The credentialIssuerId : representing the credential issuer to interact with
  • The authorizationIssuer : representing the authorization server to target

This means, that it is not the responsibility of the this class to choose (based on the issuer metadata) which authorization server will be used.

Whose responsibility is this?
Unfortunately it is not so simple to determine with which authorization server to interact.

The caller's of course and in our case, Issuer instantiates this client

As you can see, for the initialization a CredentialOffer is being used. This class is not a simple Transfer Object. Rather represents the validated issuer & authorization server metadata.

Here we choose among the authorization server possibly found in the grants of an offer or (in the absence of it) fallback to the first authorization server advertised by metadata (or the credential issuer itself)

val authorizationServer = grants?.authServer() ?: credentialIssuerMetadata.authorizationServers[0]

In summary, in my subjective opinion, we are fully aligned with the specification and no information is being lost.

PS: Why choose the first authorization server advertised ?
On the Issuer side, credential offer can make it explicit.
On the Wallet side, we wanted to keep it simple. In the future we could allow wallet to define a predicate for choosing an authorization server.

Copy link
Contributor

@babisRoutis babisRoutis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spheroid

LGTM

Let me thank you for this and especially for your valuable comments!

@babisRoutis babisRoutis merged commit 9d6af99 into eu-digital-identity-wallet:main Aug 21, 2024
4 checks passed
@spheroid
Copy link
Contributor Author

I don't agree with your assessment 😄

This means, that it is not the responsibility of the this class to choose (based on the issuer metadata) which authorization server will be used.

I know I'm splitting hairs here but that's not my point. My point is that the current draft says that it's both recommended to use the resource parameter and mandatory to use the locations parameter when the authorization_servers property is set, despite what authorization server you're using.

Don't get me wrong, I do agree that your approach makes sense. Perhaps this issue should be raised in the OpenID4VCI repository instead to clarify their point: Is it tied to the existence of the authorization_servers property or whether the credential issuer is acting as the authorization server.

@spheroid spheroid deleted the fix/par-add-resource branch August 21, 2024 14:03
@babisRoutis
Copy link
Contributor

I don't agree with your assessment 😄
This means, that it is not the responsibility of the this class to choose (based on the issuer metadata) which authorization server will be used.

I know I'm splitting hairs here but that's not my point. My point is that the current draft says that it's both recommended to use the resource parameter and mandatory to use the locations parameter when the authorization_servers property is set, despite what authorization server you're using.

Don't get me wrong, I do agree that your approach makes sense. Perhaps this issue should be raised in the OpenID4VCI repository instead to clarify their point: Is it tied to the existence of the authorization_servers property or whether the credential issuer is acting as the authorization server.

I really value your comments and of course I don't get you wrong.

Unless I missing something, locations and resource make sense when the credential issuer is not acting as an authorization server. Only in this case, there is a need to populate authorizations_servers.

Mind though that nothing stops an issuer (acting as authorization server) to populate authorization_servers with its issuer id.

Raising an issue to spec sounds like a good idea. I think there are two options:

  • Either authorization_servers should not contain credential issuer id (if present), in this case requirements for resource and locations can be left as they are, or
  • Clearly state that resource & locations is needed when credential issuer is not an authorization server.

@spheroid
Copy link
Contributor Author

Yes, exactly. 👍

Anyway – thank you for the discussion and accepting this PR in such a quick manner!

@babisRoutis
Copy link
Contributor

@spheroid FYI openid/OpenID4VCI#378

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request openid4vci alignment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use resource parameter according to RFC8707 in authorization request
2 participants