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

Reworked w3c credentials and presentations to use DataIntegrityProof instead of custom AnonCreds context #291

Merged
merged 24 commits into from
Jan 11, 2024

Conversation

Artemkaaas
Copy link
Contributor

@Artemkaaas Artemkaaas commented Dec 20, 2023

Update W3C Credential to match the design: hyperledger/anoncreds-spec#192

I also included changes from the following PRs as their changes significantly intersected with the current one:

Regarding vc format 1.1/2.0: I added an option parameter version to anoncreds_process_w3c_credential, anoncreds_credential_to_w3c, and anoncreds_create_w3c_presentation functions defining what vc format to return.

Artemkaaas and others added 13 commits December 8, 2023 08:46
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: Abdulbois <abdulbois123@gmail.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: Abdulbois <abdulbois123@gmail.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
@Artemkaaas Artemkaaas force-pushed the anoncreds-w3c-format-changes branch 2 times, most recently from 05e725d to 16ec1e9 Compare December 20, 2023 09:52
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Copy link
Member

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

I tried to go over it, but there's a lot of changed code, so it's a bit hard to look really in-depth whether are changes are correct. But overall looks good to me!

docs/design/w3c/w3c-representation.md Outdated Show resolved Hide resolved
@@ -280,20 +200,21 @@ pub extern "C" fn anoncreds_process_w3c_credential(
cred_p: *mut ObjectHandle,
) -> ErrorCode {}

/// Get value of requested credential attribute as string
/// Get credential signature information required for proof building and verification
/// This information is aggregated from `anoncredsvc-2023` and `anoncredspresvc-2023` proofs.
Copy link
Member

Choose a reason for hiding this comment

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

@swcurran my suggestion would still be to add some more dashes, and make pres -> proof. Proof is what also seems to have been used in bbs suite.

So anoncreds-vc-2023 and anoncreds-vc-proof-2023. Or maybe something like ac-vc-23 and ac-vc-proof-23 (or cl-vc-23)

design.md Outdated Show resolved Hide resolved
docs/design/w3c/w3c-representation.md Outdated Show resolved Hide resolved
src/data_types/w3c/credential.rs Outdated Show resolved Hide resolved
src/data_types/w3c/credential.rs Outdated Show resolved Hide resolved
src/data_types/w3c/credential.rs Outdated Show resolved Hide resolved
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com>
@TimoGlastra
Copy link
Member

I think one thing that is missing to make it compliant with the DI spec is that the proofValue MUST be multibase encoded and have a header:

A string value that contains the base-encoded binary data necessary to verify the digital proof using the verificationMethod specified. The contents of the value MUST be expressed with a header and encoding as described in Section 2.4 Multibase. The contents of this value are determined by a specific cryptosuite and set to the proof value generated by the Add Proof Algorithm for that cryptosuite. Alternative properties with different encodings specified by the cryptosuite MAY be used, instead of this property, to encode the data necessary to verify the digital proof.

I don't know if there's a multibase format we can use that won't add too much overhead to the final proofValue? It must decode to the binary data needed to verify the proof. I don't know if the output of messagepack is binary? If so, we could do a base64 pass on it and use that as the proofValue?

Here's a list of all multibase types: https://github.com/multiformats/multibase/blob/master/multibase.csv

@Artemkaaas
Copy link
Contributor Author

@TimoGlastra I already applied multibase encoding for the proof value with u prefix (base64 no padding) here:

https://github.com/hyperledger/anoncreds-rs/pull/291/files#diff-0423f5bfbbb105eb8c19ba64df62479a284b630ed112415e211756ac7b2d38d1

const BASE_HEADER: char = 'u';

pub trait EncodedObject {
    fn encode(&self) -> String
    where
        Self: Serialize,
    {
        let json = json!(self).to_string();
        let serialized = base64::encode(json);
        format!("{}{}", BASE_HEADER, serialized)
    }

    fn decode(string: &str) -> Result<Self>
    where
        Self: DeserializeOwned,
    {
        match string.chars().next() {
            Some(BASE_HEADER) => {
                // ok
            }
            value => return Err(err_msg!("Unexpected multibase base header {:?}", value)),
        }
        let decoded = base64::decode(&string[1..])?;
        let obj: Self = serde_json::from_slice(&decoded)?;
        Ok(obj)
    }
}

@TimoGlastra
Copy link
Member

Oh woops, sorry completely missed that.

Nice 😬

@swcurran
Copy link
Member

@Artemkaaas — there is a conflict in the PR now. Could you please resolve and we’ll merge this. Sorry for the delay over the break.

…changes

# Conflicts:
#	.github/workflows/build.yml
@swcurran swcurran merged commit ae4f793 into hyperledger:main Jan 11, 2024
26 checks passed
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.

4 participants