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

Expose signature string (RFC 5280 4.1.2.3) #36

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

fl034
Copy link

@fl034 fl034 commented Jan 30, 2023

The current signature field exposes the signature value (RFC 5280 4.1.1.3). But at the beginning of the certificate, we have a field called signature which should contain the OID of the signature algorithm.

See https://datatracker.ietf.org/doc/html/rfc5280#section-4.1.2.3

@tmolitor-stud-tu
Copy link

@fl034
Copy link
Author

fl034 commented Feb 20, 2023

@tmolitor-stud-tu you're right, thanks for pointing that out!
sigAlgOID seems to be RFC 5280 4.1.2.3.

But there must be the same value in RFC 5280 4.1.1.2:

This field MUST contain the same algorithm identifier as the
signature field in the sequence tbsCertificate (Section 4.1.2.3).

So this is what is missing. Will update my PR

@fl034
Copy link
Author

fl034 commented Mar 29, 2023

@filom could you check this PR please :)?

@filom
Copy link
Owner

filom commented Mar 30, 2023

The signature algorithm identifier is present in the certificate first level and in the TBSCertificate (what I call with the variable "block1") and they have to be identical by spec, so there is no reason to create additional parameters to return the same information.
Changing the names will also break compatibility.
The only missing part in the current implementation would be the algorithm optional parameter sigAlgParams and the proper implementation should be:

/// Gets the DER-encoded signature algorithm parameters from this certificate's signature algorithm.
public var sigAlgParams: Data? {
    return block1[X509BlockPosition.signatureAlg]?.sub(1)?.rawValue?.derEncodedSequence
}

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.

3 participants