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

RFC018: Issuing credentials using OIDC4VCI #257

Closed
wants to merge 10 commits into from
Closed

Conversation

reinkrul
Copy link
Member

@reinkrul reinkrul commented Apr 12, 2023

TODO:

Fixes #258

@reinkrul reinkrul changed the title RFC018: Initial version RFC018: Issuing credentials using OIDC4VCI Apr 13, 2023
Copy link
Member

@woutslakhorst woutslakhorst left a comment

Choose a reason for hiding this comment

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

I think it also misses the fact that an implementation MUST implement both the wallet and issuer roles.

rfc/rfc018-oidc4vci-credential-issuance.md Outdated Show resolved Hide resolved
rfc/rfc018-oidc4vci-credential-issuance.md Outdated Show resolved Hide resolved
rfc/rfc018-oidc4vci-credential-issuance.md Outdated Show resolved Hide resolved

### Status of document

This document describes a Nuts standards protocol.
Copy link
Member

Choose a reason for hiding this comment

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

status is draft

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure

rfc/rfc018-oidc4vci-credential-issuance.md Outdated Show resolved Hide resolved
rfc/rfc018-oidc4vci-credential-issuance.md Outdated Show resolved Hide resolved
rfc/rfc018-oidc4vci-credential-issuance.md Outdated Show resolved Hide resolved
@reinkrul
Copy link
Member Author

I think it also misses the fact that an implementation MUST implement both the wallet and issuer roles.

why?

@woutslakhorst
Copy link
Member

I think it also misses the fact that an implementation MUST implement both the wallet and issuer roles.

why?

Otherwise it's not a Nuts node, just a half one.

While OpenID4VCI specifies a PIN to mitigate credential stealing,
implementations MUST NOT require a PIN in server-to-server flows (see Appendix A.1).

The following from the OpenID4VCI specification does not need to be supported:
Copy link
Member

Choose a reason for hiding this comment

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

"does not need" might be too vague.
Are all the items in the list restricted by what's advertised in the (wallet/issue) metadata? If not then the does not becomes a must not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then you can't have a generic OpenID4VCI wallet that also support Nuts, since a compelte implementation vs this RFC are mutually exclusive.

Maybe we should inverse it, specifying what MUST be supported? Although I wrote it up this way since that sounded easier.

Copy link
Member

Choose a reason for hiding this comment

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

Usually I scan to see what I MUST do and skip everything else ;)


### 3.2.1 Credential Offer Success Response

In case the wallet accepted the offer and will try to retrieve the credential, it MUST respond with HTTP status `200 OK`.
Copy link
Member

Choose a reason for hiding this comment

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

You mean retrieve within the context of the offer call? Not async

Copy link
Member Author

Choose a reason for hiding this comment

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

No, just whether the wallet accepted the credential offer. I'll clarify the sentence.

or coerced into giving the QR code.
To prevent eavesdropping, HTTPS MUST be used to protect communication (see Appendix B.1).

### A.2 Trust issuer based on TLS certificates
Copy link
Member

Choose a reason for hiding this comment

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

Do we do this currently?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I tried to convey that we only trust issuers from trusted certificates, which is defined by the truststore. And since identifier URLs are HTTPS URLs, they're trusted if the connection can be made.

@reinkrul reinkrul requested a review from woutslakhorst July 11, 2023 12:27
Copy link
Member

@woutslakhorst woutslakhorst left a comment

Choose a reason for hiding this comment

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

I think the does not needs to be inversed to MUSTs

@reinkrul reinkrul requested a review from woutslakhorst July 12, 2023 11:48
- Pre-authorized code flow
- Credential offer:
- with `credential_offer` parameter
- with `string` values, referring to a credential's `id` in `credentials_supported`
Copy link
Member

Choose a reason for hiding this comment

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

We currently do not support this since our credential definitions do not contain an id? @woutslakhorst

Copy link
Member

Choose a reason for hiding this comment

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

For the credentials array I think we only support the JSON object containing the ldp_vc format specific credential_definition as defined for in https://openid.bitbucket.io/connect/editors-draft/openid-4-verifiable-credential-issuance-1_0.html#name-vc-secured-using-data-integ

Copy link
Member

Choose a reason for hiding this comment

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

Recent versions of the spec seem to have replaced id by scope. The scope field is also not present in our definitions. Not sure if the node or this RFC should be changed?

rfc/rfc018-openid-credential-issuance.md Outdated Show resolved Hide resolved
rfc/rfc018-openid-credential-issuance.md Outdated Show resolved Hide resolved
rfc/rfc018-openid-credential-issuance.md Outdated Show resolved Hide resolved
rfc/rfc018-openid-credential-issuance.md Show resolved Hide resolved
rfc/rfc018-openid-credential-issuance.md Show resolved Hide resolved
rfc/rfc018-openid-credential-issuance.md Show resolved Hide resolved
rfc/rfc018-openid-credential-issuance.md Outdated Show resolved Hide resolved

### 3.2.1 Credential Offer Success Response

In case the wallet considers the offer valid, it MUST respond with HTTP status `200 OK`.
Copy link
Member

Choose a reason for hiding this comment

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

Just to check, we still need to implement this right?

Copy link
Member

Choose a reason for hiding this comment

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

This conflicts with the synchronous flow. An invalid credential in the synchronous flow for a valid offer would result in a 400 while this line says that it MUST be a 200. A better separation between sync/async flow should fix this

Copy link
Member Author

Choose a reason for hiding this comment

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

It says when it is considered valid? So if valid, 200, if not 400.

The sync part will probably change when we support retying.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it depends on what you consider to be "the offer". My interpretation was that this is just the credential_offer message, but seems that you refer to the entire flow which includes retrieval of the credential for the sync flow.

@reinkrul reinkrul requested a review from gerardsn July 28, 2023 04:58
@reinkrul
Copy link
Member Author

reinkrul commented Dec 17, 2024

We're not going to support this as it currently is. Keeping branch for future reference.

@reinkrul reinkrul closed this Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OIDC4VCI: Write RFC
4 participants