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

define where in mdocs to include transaction_data_hashes #330

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Sakurann
Copy link
Collaborator

@Sakurann Sakurann commented Nov 19, 2024

blocked by #421

resolves #259

| --------- | ---------- | ----------- | --------------- |
| `net.openid.openid4vc` | `transaction_data_hashes` | as defined in Section 7.4 of this specification | [ tstr* ] |

If KeyAuthorization for the requested transaction authorization is not present, the Wallet MUST return an error.
Copy link
Collaborator Author

@Sakurann Sakurann Nov 19, 2024

Choose a reason for hiding this comment

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

need to define proper data element identifier for KeyAuthorization? cc @martijnharing

Copy link
Contributor

Choose a reason for hiding this comment

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

When using the mdoc to sign data, there will often be security considerations is to whether the particular mdoc can / should / may sign a particular data element. Whether there are any limitations strongly dependent on the specific document type and use case. But typically the Issuer indicates with a particular namespace and data element identifier which exact things the mdoc can / should / may sign.
So from a security perspective we should define the namespace and data element dependent on the use case and not as a generic identifier.

Choose a reason for hiding this comment

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

as I understand it, transaction_data contains a 'type' parameter. If we want issuers to have more granular control over what type of transaction_data is allowed to be returned by the wallet we could define a set of DataElements:

namespace: net.openid.openid4vc.transactiondata
identifier: must be the type of the transaction_data

The wallet MUST return split the transaction_data_hashes so each is returned as part of the value array in a DataElement with a namespace net.openid.openid4vc.transactiondata and identifier of transaction_data.type. A wallet MUST return an error if a request contains transaction_data with a type that is not present in the KeyAuthorizations.

psuedocode example:
paymentsTransactionData = { type = "payments", ... }
fooTransactionData1 = { type = "foo" , ...}
fooTransactionData2 = { type = "foo", ... }

request {
...
transaction_data [ paymentsTransactionData, fooTransactionData1, fooTransactionData2 ]
..
}

then the response would have in DeviceSigned:

...
net.openid.openid4vc.transactiondata:
[
{ payments -> [ base64(hash(paymentsTransactionData))) }
{ foo -> [ base64(hash(fooTransactionData1)), base64(hash(fooTransactionData2)) ] }
]
}
..

additionally the KeyAuthorizations would either need to authorize the entire net.openid.openid4vc.transactiondata namespace, or the identifiers payments and foo within the net.openid.openid4vc.transactiondata namespace.

@martijnharing this would provide the granularity you are looking for?
@Sakurann does this roughly make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the type was included, it would mean the issuer controls what types of transactions the wallet can sign with the credential. Is this really an issuer decision?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, since the mdoc can use device signature or device mac, i would argue it should be mostly device signature since for transaction signing non-repudiation is probably a requirement, isn't it?

Copy link

Choose a reason for hiding this comment

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

If the type was included, it would mean the issuer controls what types of transactions the wallet can sign with the credential. Is this really an issuer decision?

@awoie in my opinion, yes. See also #259 (comment). Of course, an issuer may decide to include a device-signed attribute with arbitrary CBOR content, confirming “any” transaction. But I do not expect for example EUDIW PID issuers to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

The approach of @GarethCOliver on where to put the data mostly makes sense to me. I think we should define the openid namespace as an option that profiles can use, but not mandate it as the only option.

@Sakurann Sakurann added the ISO? label Nov 28, 2024
@Sakurann Sakurann added ISO_VirtualMeeting relevant for ISO OID4VP mdoc profile over DC API and removed ISO_VirtualMeeting relevant for ISO OID4VP mdoc profile over DC API labels Jan 13, 2025
@andprian
Copy link

I believe the hash algorithm should also be protected by the signature of the mdoc. In the case of transaction data, signing implies commitment from the user on some data. Or the commitment is not complete if it is made on the data representation (hash) without committing to the algorithm as well.

@Sakurann Sakurann added this to the Final 1.0 milestone Mar 4, 2025
@GarethCOliver
Copy link

I believe the hash algorithm should also be protected by the signature of the mdoc. In the case of transaction data, signing implies commitment from the user on some data. Or the commitment is not complete if it is made on the data representation (hash) without committing to the algorithm as well.

Yeah, you are right. Based on the outcome of #443 it should either be a single, additional data element, or in a wrapper cbor structure (if alg remains per-transaction-data).
{
hash = hash(...)
alg = ALG
}

@Sakurann Sakurann removed the ISO? label Mar 11, 2025
@sander
Copy link

sander commented Mar 11, 2025

See also: #259 (comment)

@@ -2234,6 +2234,14 @@ See ISO/IEC TS 18013-7 Annex B [@ISO.18013-7] and ISO/IEC 23220-4 Annex C [@ISO.

The VP Token contains the base64url-encoded `DeviceResponse` CBOR structure as defined in ISO/IEC 18013-5 [@ISO.18013-5] or ISO/IEC 23220-4 [@ISO.23220-4]. Essentially, the `DeviceResponse` CBOR structure contains a signature or MAC over the `SessionTranscript` CBOR structure including the OpenID4VP-specific `Handover` CBOR structure.

The `transaction_data_hashes` response parameter MUST be returned in DeviceSigned using the `transaction_data_hashes` DataElement defined below:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The `transaction_data_hashes` response parameter MUST be returned in DeviceSigned using the `transaction_data_hashes` DataElement defined below:
The `transaction_data_hashes` response parameter MUST be returned in `DeviceSigned` using the `transaction_data_hashes` data element defined below:

@sander
Copy link

sander commented Mar 13, 2025

Note #421 contains an alternative proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

define where in mdoc presentation to include transaction data
6 participants