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

Encoding of Authority Key Identifier #173

Closed
xipki opened this issue Apr 16, 2024 · 11 comments
Closed

Encoding of Authority Key Identifier #173

xipki opened this issue Apr 16, 2024 · 11 comments
Assignees

Comments

@xipki
Copy link
Contributor

xipki commented Apr 16, 2024

As in Page 14 (version -09)

Authority Key Identifier (authorityKeyIdentifier).  If the
      authority key identifier contains all of keyIdentifier,
      certIssuer, and certSerialNumberm or if only keyIdentifier is
      present the extension value can be CBOR encoded.  If all three are
      present a CBOR array is used, if only keyIdentifier is present,
      the array is omitted:

      KeyIdentifierArray = [
        keyIdentifier: KeyIdentifier / null,
        authorityCertIssuer: GeneralNames,
        authorityCertSerialNumber: CertificateSerialNumber
      ]
      AuthorityKeyIdentifier = KeyIdentifierArray / KeyIdentifier

To encode it in CBOR, either only keyIdentifieris present, or all three fields must be present.

What is the reason to have this limitation? Changing KeyIdentifierArray as follows can also cover other situations:

      KeyIdentifierArray = [
        keyIdentifier: KeyIdentifier / null,
        authorityCertIssuer: GeneralNames *** / null ***,
        authorityCertSerialNumber: CertificateSerialNumber *** / null ***
      ]
      AuthorityKeyIdentifier = KeyIdentifierArray / KeyIdentifier
@emanjon
Copy link
Collaborator

emanjon commented Apr 18, 2024

KeyIdentifier: KeyIdentifier / null,

The null in the current draft seems weird. The current draft is supposed to be

 KeyIdentifierArray = [
        keyIdentifier: KeyIdentifier,
        authorityCertIssuer: GeneralNames,
        authorityCertSerialNumber: CertificateSerialNumber
      ]
      AuthorityKeyIdentifier = KeyIdentifierArray / KeyIdentifier

I fixed that in a commit to main.

@highlunder
Copy link
Collaborator

The motivation to always include keyIdentifier is found in 5280:

"The keyIdentifier field of the authorityKeyIdentifier extension MUST be included in all certificates generated by conforming CAs to facilitate certification path construction." -- https://datatracker.ietf.org/doc/html/rfc5280

With your proposal also cases where GeneralNames XOR CertificateSerialNumber are included could be handled. We considered this in the design, but concluded that it would add unwanted overhead both in encoding and in processing, especially given that we hadn't found any such certificates "out in the wild". Do you have use cases where one but not the other field is used, that you want to share?

@emanjon
Copy link
Collaborator

emanjon commented Apr 19, 2024

RFC 5280 also says

There is one exception;
   where a CA distributes its public key in the form of a "self-signed"
   certificate, the authority key identifier MAY be omitted.

@emanjon
Copy link
Collaborator

emanjon commented Apr 19, 2024

Would be good to know what is used in practice. My memory is that allowing all fields to independently be null increases code-complexity for compression

@xipki
Copy link
Contributor Author

xipki commented Apr 19, 2024

RFC 5280 also says

There is one exception;
   where a CA distributes its public key in the form of a "self-signed"
   certificate, the authority key identifier MAY be omitted.

I think this clause means the whole extension authorityKeyIdentifier could be absent, but not the keyIdentifierfield in the extension.

@xipki
Copy link
Contributor Author

xipki commented Apr 19, 2024

Would be good to know what is used in practice. My memory is that allowing all fields to independently be null increases code-complexity for compression

I have also never seen a certificate with NULL keyidentiifer field in the authorityKeyIdentiifer (alias AKI). But as described under https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.1 ,

The identification MAY be based on either the
   key identifier (the subject key identifier in the issuer's
   certificate) or the issuer name and serial number.

, an AKi may have the non-empty fields authorityCertIssuerand authorityCertSerialNumber, and the empty field keyIdentifier.

@xipki
Copy link
Contributor Author

xipki commented Apr 19, 2024

Would be good to know what is used in practice. My memory is that allowing all fields to independently be null increases code-complexity for compression

@emanjon BTW, if we consider code-complexity in this issue, then cases mentioned #172 shall be considered much more seriously.

@highlunder
Copy link
Collaborator

Would be good to know what is used in practice. My memory is that allowing all fields to independently be null increases code-complexity for compression

@emanjon BTW, if we consider code-complexity in this issue, then cases mentioned #172 shall be considered much more seriously.

FYI The cases mentioned in #172 have been presented in their current form with their trade-offs, discussed and accepted by IETF meeting attendees at more than one IETF COSE meeting. Which, of course, is not to say that we can't discuss it again.

@xipki
Copy link
Contributor Author

xipki commented May 4, 2024

I do not have strong option to change the syntax. But the CDDL need to be changed to be consistent with the text:
From

      KeyIdentifierArray = [
        keyIdentifier: KeyIdentifier / null,
        authorityCertIssuer: GeneralNames,
        authorityCertSerialNumber: CertificateSerialNumber
      ]

to

      KeyIdentifierArray = [
        keyIdentifier: KeyIdentifier, --removed null
        authorityCertIssuer: GeneralNames,
        authorityCertSerialNumber: CertificateSerialNumber
      ]

@highlunder
Copy link
Collaborator

Assigned Lijun to propose a PR

@xipki
Copy link
Contributor Author

xipki commented Jul 1, 2024

The current github version has already the fixed version. Thus I close this issue.

      KeyIdentifierArray = [
        keyIdentifier: KeyIdentifier, --removed null
        authorityCertIssuer: GeneralNames,
        authorityCertSerialNumber: CertificateSerialNumber
      ]

@xipki xipki closed this as completed Jul 1, 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

No branches or pull requests

3 participants