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

Difference between MdocCborIssuer private key and wallet devicekeyinfo #7

Closed
oligatorr opened this issue Nov 5, 2024 · 10 comments
Closed
Labels
help wanted Extra attention is needed
Milestone

Comments

@oligatorr
Copy link
Contributor

Hello

In the example code of the Readme (Usage/Issue an MDOC CBOR) one can see:

mdoci = MdocCborIssuer(
    private_key=PKEY
)

mdoc = mdoci.new(
    doctype="eu.europa.ec.eudiw.pid.1",
    data=PID_DATA,
    devicekeyinfo=PKEY  # TODO
)

According to this example, the private key utilised to sign the attestation in CBOR is the same as the devicekeyinfo. However devicekeyinfo in my understanding is a secret concealed in the wallet that is used to ensure that the attestations stored in the wallet are cryptographically linked to the wallet. (Furthermore I think this is the private key linked to the Wallet Instance Attestation).

Is this example meaningful ? Shouldn't MdocCborIssuer private_key attribute and devicekeyinfo be two different private keys

  • one owned by the issuer and
  • the other concealed in the wallet in which attestations will be issued ?

Thank you in advance for your answers/comments

Olivier

@peppelinux
Copy link
Member

you are right, the example naively uses the same key while the device signed part must use a private cryptographic material under the sole control of the holder and therefore different from the one used by the credential issuer

if you might want, you can offer a contribution in the form of a PR adding another key as owned only by holder

@peppelinux peppelinux added the help wanted Extra attention is needed label Jan 28, 2025
@oligatorr
Copy link
Contributor Author

Hello Giuseppe,

I have forked this project in an internal repository and brought some necessary changes (IMHO) to make it work with python script attestation generator of our own. I will consider to make a public fork on github so that a proper PR could be submitted as my preference is not to depend of a fork of your library.

"Necessary changes" include a fix in the check signature functions in the library and other stuff like unit tests enhancing.

I'll let you know when it is ready.

Olivier

@peppelinux
Copy link
Member

@oligatorr if you are ready with the PR, please do

@oligatorr
Copy link
Contributor Author

oligatorr commented Mar 14, 2025

Hello @peppelinux

I recently figured out that I actually (hopefully) enhanced a fork (https://github.com/eu-digital-identity-wallet/pyMDOC-CBOR) of this lib. So if I suggest a PR, it will be based on the latest version of the here before repo which includes this commit eu-digital-identity-wallet@d46a5f7

I was not sure that merging back the fork from https://github.com/eu-digital-identity-wallet/pyMDOC-CBOR, is what is needed or wished for this library so I have put the question aside. I am ready to discuss these questions in a private channel if you feel it useful. Unfortunately I already tried to figure out how to create a private channel from github without success.

Regards

Olivier

@peppelinux
Copy link
Member

we hare handling a merge of the eudiw fork here: #9
please join in the review and provide the changes within the review or using the same branch opening a sub fork

ottherwise you can wait for the PR 9 to be merged before proceed

@oligatorr
Copy link
Contributor Author

oligatorr commented Mar 17, 2025

I prefer to wait for your eudiw fork merge to avoid to mixing different subject and goals together. In the mean time you could have a look to my last commit: oligatorr@9703fdf in https://github.com/oligatorr/pyMDOC-CBOR

@peppelinux
Copy link
Member

@oligatorr I just have release version 0.7.0 with all the alignments taken from the eu reference implementation

we now are ready to take your PR. I have read your commit, it is pretty good

looking forward for your contributions

@peppelinux peppelinux added this to the 0.8.0 milestone Mar 18, 2025
@oligatorr
Copy link
Contributor Author

Hello @peppelinux

As you have seen I have proposed my PR (see #12). Two remarks:

  1. Looks like Luis P. and I add the same argument to pass status list (revocation) to mdoc and mso. I kept my naming because it is more specific ("Status list" revocation vs "Identifier list" revocation refs) (and easier for me :o)
  2. In the code that use this library I have an implementation of status list reference integration into mso, would that make sense to move status list implementation into this lib instead of having to build status list dictionary before passing it to the mdoc/mso objects ? It will imply more parameter though (cert, index,url...)

@peppelinux
Copy link
Member

peppelinux commented Mar 20, 2025

@oligatorr what about moving the status list python impl into a dedicated python project?

it would be usable also for other token data formts, like sd-jwt-vc.

we may also think in the future to migrate it into identitypython and invite you as an active contributor of this community

I just have completed my last review on your PR, you will find it there

@peppelinux
Copy link
Member

Resolved by #12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants