Skip to content

Add purpose codesign#18567

Closed
ljaenicke wants to merge 6 commits intoopenssl:masterfrom
PHOENIXCONTACT:purpose_codesign
Closed

Add purpose codesign#18567
ljaenicke wants to merge 6 commits intoopenssl:masterfrom
PHOENIXCONTACT:purpose_codesign

Conversation

@ljaenicke
Copy link
Contributor

@ljaenicke ljaenicke commented Jun 14, 2022

This PR adds the purpose "Code Signing" to allow verification of certificate purposes in the context of code signing. Code signing certificates have the critical keyUsage "digitalSignature" and the non-critical extendedKeyUsage "id-kp-codeSigning".

Checklist
  • documentation is added or updated
  • tests are added or updated

@ljaenicke
Copy link
Contributor Author

This pull request is part of issue #18491

Copy link
Contributor

@FdaSilvaYY FdaSilvaYY left a comment

Choose a reason for hiding this comment

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

A few typos on 'signing' word.
Good work over all

@t8m t8m added branch: master Applies to master branch triaged: feature The issue/pr requests/adds a feature labels Jun 15, 2022
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jun 15, 2022
@t8m
Copy link
Member

t8m commented Jun 15, 2022

We will need tests to be able to merge this according to our policy.

@ljaenicke
Copy link
Contributor Author

I spent some time trying to reverse engineer the content of test/certs and the other subcomponents of the test/ directory. Is there any README I have missed?

@t8m
Copy link
Member

t8m commented Jun 15, 2022

I spent some time trying to reverse engineer the content of test/certs and the other subcomponents of the test/ directory. Is there any README I have missed?

I suppose you did not miss anything. There is not much documentation, unfortunately. I'd put some new test certs to the smime-certs subdir - look at and extend the mksmime-certs.sh script on how to generate additional cert with the code signing purpose. The test recipe to update would be the 80-test_cms.t.

@t8m t8m added approval: review pending This pull request needs review by a committer approval: otc review pending labels Jun 16, 2022
@ljaenicke
Copy link
Contributor Author

Updated pull request to resolve merge conflicts

@kaduk
Copy link
Contributor

kaduk commented Jun 27, 2022

CMS is something of a general-purpose content format these days, getting used not just for S/MIME email protection and code signing, but also used as parts of bootstrapping autonomic network equipment (RFC 8995), Kerberos public-key initial authentication (RFC 4556), RPKI (many RFCs), etc. It seems like we're setting ourselves up for a maintenance headache if we add individual flags for "use this purpose value", "use that purpose value", etc. I think it would be better to just have a -purpose <foo> argument, leaving the default to the current smime_sign but making this a flexible extension point rather than an ad hoc one.

@ljaenicke
Copy link
Contributor Author

Note: adding this to the flags looked like the least intrusive change.

From my perspective your proposal could be achieved in two ways. The simple way would be to extend CMS_verify() with an additional argument providing the purpose as a string, just as I did with the internal function cms_signerinfo_verify_cert(), passing the purpose string down from the command line argument.
This solution would howver be incomplete in the sense that the other use cases mentioned like BRSKI (RFC 8995) would still not be covered unless the purpose and its checking is covered inside OpenSSL. Allowing respective hooks to be passed down from an application however seems to be a significant rewrite.
Additional note: BRSKI uses certificates according to IEEE 802.1AR and the standard here gives a lot of freedom on how to use the KU and EKU extensions.

@github-actions github-actions bot removed the severity: fips change The pull request changes FIPS provider sources label Jun 29, 2022
@ljaenicke
Copy link
Contributor Author

I have reworked the pull request after digging deeper. It is not necessary to set the purpose within the CMS_verify() call tree affecting the X509_STORE_CTX. It is sufficient to set the purpose when setting up the X509_STORE which can be done with the command line application(s) -purpose option.
This is now reflected by respective test cases.
(Did not see this when reverse-engineering the logic based on the 1.1.1 documentation, even if the option was listed but not as detailed in the 3.x manual pages...)

@ljaenicke
Copy link
Contributor Author

Fix typo spottet by @t-j-h

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Aug 15, 2022
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Aug 16, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

Copy link
Contributor

@DDvO DDvO left a comment

Choose a reason for hiding this comment

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

Thanks for very swiftly handling my first comment.
Meanwhile I've done a full review - please see the individual new comments.
Your test cases are commendable.

Comment on lines +936 to +954
if ((x->ex_xkusage & (XKU_ANYEKU | XKU_SSL_SERVER)) != 0)
return 0;
Copy link
Contributor

@DDvO DDvO Aug 16, 2022

Choose a reason for hiding this comment

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

And how about the requirements demanded by CA/BF on CA certs,
and the further ones on EE certs regarding policies, CDPs, and AIA?

@DDvO
Copy link
Contributor

DDvO commented Aug 16, 2022

BTW, when making further adaptations to already (at least partly) reviewed PRs,
please use 'fixup' commits (which will be auto-squashed on merging) rather than amending and force-pushing.
such that it is easier to keep track which stages/portions a reviewer has already seen.
For instance, git commit --fixup @~1 crypto/ to adapt the but-last commit with changes in crypto/

@ljaenicke
Copy link
Contributor Author

Agreed. There was no "--fixup" that many years ago : -).

Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Still OK

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Aug 17, 2022
@ljaenicke ljaenicke requested a review from DDvO August 17, 2022 07:40
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Aug 18, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented Aug 18, 2022

Merged to master branch. Thank you for your contribution!

@t8m t8m closed this Aug 18, 2022
openssl-machine pushed a commit that referenced this pull request Aug 18, 2022
Put "}," on separate lines as suggested in PR #18567

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #18567)
openssl-machine pushed a commit that referenced this pull request Aug 18, 2022
Code signing certificates have other properties as for example described in
CA Browser Forum documents. This leads to "unsupported certificate purpose" errors when
verifying signed objects.
This patch adds the purpose "codesign" to the table in X.509 certificate verification and
the verification parameter "code_sign" to X509_VERIFY_PARAM.

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #18567)
openssl-machine pushed a commit that referenced this pull request Aug 18, 2022
Correct configuration according to CA Browser forum:
  KU: critical,digitalSignature
  XKU: codeSiging

Note: I did not find any other document formally defining the requirements
for code signing certificates.

Some combinations are explicitly forbidden, some flags can be ignored

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #18567)
openssl-machine pushed a commit that referenced this pull request Aug 18, 2022
The tests only cover the correct handling of the codesigning purpose in the certificates
in the context of the cms command line tool.
The interpretation of the certificate purpose is tested in the context of the "verify"
app. The correct handling of the cms objects is tested by other tests in 80-test_cms.t.

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #18567)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Put "}," on separate lines as suggested in PR openssl#18567

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#18567)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Code signing certificates have other properties as for example described in
CA Browser Forum documents. This leads to "unsupported certificate purpose" errors when
verifying signed objects.
This patch adds the purpose "codesign" to the table in X.509 certificate verification and
the verification parameter "code_sign" to X509_VERIFY_PARAM.

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#18567)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Correct configuration according to CA Browser forum:
  KU: critical,digitalSignature
  XKU: codeSiging

Note: I did not find any other document formally defining the requirements
for code signing certificates.

Some combinations are explicitly forbidden, some flags can be ignored

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#18567)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
The tests only cover the correct handling of the codesigning purpose in the certificates
in the context of the cms command line tool.
The interpretation of the certificate purpose is tested in the context of the "verify"
app. The correct handling of the cms objects is tested by other tests in 80-test_cms.t.

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#18567)
pr000000f added a commit to pr000000f/tongsuo-dev that referenced this pull request Dec 8, 2025
…RAM default_table

From OpenSSL commit 1a68a3e42142a2c188f4b69c7337438c89502143
openssl/openssl#18567

Put "}," on separate lines as suggested in PR #18567
pr000000f added a commit to pr000000f/tongsuo-dev that referenced this pull request Dec 15, 2025
…rtificates

From OpenSSL commit 178696d6020878361a088086243d56203e0beaa9
openssl/openssl#18567

Code signing certificates have other properties as for example described in
CA Browser Forum documents. This leads to "unsupported certificate purpose" errors when
verifying signed objects.
This patch adds the purpose "codesign" to the table in X.509 certificate verification and
the verification parameter "code_sign" to X509_VERIFY_PARAM.
pr000000f added a commit to pr000000f/tongsuo-dev that referenced this pull request Dec 15, 2025
…ication

From OpenSSL commit 61a97676914df358dd014a9b6fe2ba01b0ebe508
openssl/openssl#18567

Correct configuration according to CA Browser forum:
  KU: critical,digitalSignature
  XKU: codeSiging

Note: I did not find any other document formally defining the requirements
for code signing certificates.

Some combinations are explicitly forbidden, some flags can be ignored
pr000000f added a commit to pr000000f/tongsuo-dev that referenced this pull request Dec 15, 2025
…plication

From OpenSSL commit 19914fec9bac08ca7c7917eddc1b7d1dba67e4a7
openssl/openssl#18567

The tests only cover the correct handling of the codesigning purpose in the certificates
in the context of the cms command line tool.
The interpretation of the certificate purpose is tested in the context of the "verify"
app. The correct handling of the cms objects is tested by other tests in 80-test_cms.t.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch triaged: feature The issue/pr requests/adds a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants