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

MACed ID_CRED_x is always {4: "k"} #272

Closed
chrysn opened this issue May 16, 2024 · 2 comments
Closed

MACed ID_CRED_x is always {4: "k"} #272

chrysn opened this issue May 16, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@chrysn
Copy link
Collaborator

chrysn commented May 16, 2024

Currently, MAC calculations always generates a {4: "k"} where k is the 1-long string encoded in the numeric kid:

    let mac_2 = compute_mac_2(
        crypto,
        &prk_3e2m,
        c_r,
        &cred_r.get_id_cred(),
        cred_r.value.as_slice(),
        &th_2,
        ead_2,
    );

    pub fn get_id_cred(&self) -> BytesIdCred {
        [0xa1, 0x04, 0x41, self.kid] // cbor map = {4: kid}
    }

However, as I understand the RFC, the value should be

  • what we currently do iff the serialized ID_CRED_x was a number
  • something similar to what we do currently on longer KID (which we don't support)
  • the literal ID_CRED_x otherwise

This is only becoming apparent through the latest PRs #267, #270 and #271 -- without those, pass-by-kid was the only option anyway.

I think we'll have to overhaul a bit how we transport ID_CRED_x, CRED_x and how we link them to each other.

I anticipate that any plug test we do with a different implementation will fail as soon as we try anything but ID_CRED_x-is-KID with another implementation.

@chrysn
Copy link
Collaborator Author

chrysn commented May 16, 2024

I propose we differentiate CredentialRpk into two types (this is clearly an API change):

  • Credential similar to CredentialRpk, but without the kid, and it always has a value and a key
  • IdCredential, which is either an enum (u8 | bytes (not initially) | CBOR item in a buffer), or just a CBOR item in a buffer (I think the preferred serialization there is the compact one, but whatever we choose, building the corresponding other form is either along the lines of "prepend a10441 to it if it doesn't start with a0/3", or of "if it starts with a10441, strip those 3 bytes").

By being different types, the using application is forced to do a conversion. Currently we do that conversion implicitly by one of two ways:

  1. If it is by-value and we can parse it, we parse it
  2. If it is by-reference and matches what we configured earlier, we take that

I don't like that we do 2 anyway (it is asking the application to set up something that it expects way earlier than it has the information), but if we want to stick with 2, we can still allow it in the API by having a combined parse-or-recognize method; I'd personally rather get rid of it and just have a fallible parse method (that recognizes an IdCred to be by value and then turns it into a Cred) and a match or lookup method, and run those independently of the context.

@geonnave
Copy link
Collaborator

geonnave commented Aug 2, 2024

Solved in #284, e.g. see this line.

@geonnave geonnave closed this as completed Aug 2, 2024
@geonnave geonnave added bug Something isn't working and removed enhancement New feature or request labels Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants