Skip to content

Fix inconsistent doc of CMS_verify and PKCS7_verify etc.#18915

Closed
DDvO wants to merge 4 commits intoopenssl:masterfrom
siemens:fix_inconsistent_doc_SMIME_CMS_PKCS_verify
Closed

Fix inconsistent doc of CMS_verify and PKCS7_verify etc.#18915
DDvO wants to merge 4 commits intoopenssl:masterfrom
siemens:fix_inconsistent_doc_SMIME_CMS_PKCS_verify

Conversation

@DDvO
Copy link
Contributor

@DDvO DDvO commented Jul 30, 2022 •
edited
Loading

  • Fix many inconsistencies in documentation of CMS_verify() and PKC7_verify()
  • Add some missing detail in related doc: CMS_add0_cert.pod, PKCS7_sign.pod, and PKCS7_sign_add_signer.pod
  • Add missing doc for PKCS7_add_certificate() and PKCS7_add_crl()
  • Change B<p to I< in CMS_verify(), PKC7_verify() , PKCS7_sign.pod, and PKCS7_sign_add_signer.pod
  • Minor related code cleanup in crypto/cms and crypto/pkcs7 and minor related cleanup in pkcs7.h.in

BTW, I wonder why the tremendous overlap between CMS and PKCS7 was introduced.
It should be clear that they are basically the same.
Major consolidation should be done here, as now requested in #18919.

@DDvO DDvO added branch: master Applies to master branch approval: review pending This pull request needs review by a committer branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) approval: otc review pending triaged: bug The issue/pr is/fixes a bug triaged: documentation The issue/pr deals with documentation (errors) triaged: cleanup The issue/pr deals with cleanup of comments/docs not altering code significantly branch: 3.0 Applies to openssl-3.0 branch labels Jul 30, 2022
@DDvO DDvO force-pushed the fix_inconsistent_doc_SMIME_CMS_PKCS_verify branch from b63720a to 844ff45 Compare July 31, 2022 05:01
@DDvO DDvO mentioned this pull request Jul 31, 2022
Open
@DDvO DDvO changed the title Fix inconsistent doc of SMIME purposes and CMS_verify vs. PKCS7_verify Fix inconsistent doc of CMS_verify and PKCS7_verify etc. Jul 31, 2022
@DDvO DDvO removed the triaged: bug The issue/pr is/fixes a bug label Jul 31, 2022
@openssl-machine
Copy link
Collaborator

openssl-machine commented Aug 31, 2022

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

@t8m
Copy link
Member

t8m commented Aug 31, 2022

This cannot be merged to stable branches because code cleanups are not allowed there. The missingcrypto111.txt should not be touched. If you want the doc fixes on stable branches, please split the PR.

@DDvO
Copy link
Contributor Author

DDvO commented Aug 31, 2022 •
edited
Loading

So I will remove the changes to missingcrypto111.txt from this PR and move the code/header refactoring to a separate commit within this PR.
Likely I'll set up a PR that fixes the doc for 1.1.1 - there missingcrypto111.txt should be updated, right?
I'm not sure it's worth having a 3rd PR for fixing the doc in 3.0 - what do you think?

@DDvO DDvO force-pushed the fix_inconsistent_doc_SMIME_CMS_PKCS_verify branch from 844ff45 to 9e9bc39 Compare August 31, 2022 08:33
@DDvO DDvO removed branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) branch: 3.0 Applies to openssl-3.0 branch labels Aug 31, 2022
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Aug 31, 2022
@DDvO
Copy link
Contributor Author

DDvO commented Aug 31, 2022 •
edited
Loading

So I will remove the changes to missingcrypto111.txt from this PR and move the code/header refactoring to a separate commit within this PR.

Done, and rebased the PR.
So the only content change here that I just did is not to touch missingcrypto111.txt anymore.
And marked this PR to pertain to master only.

@t8m
Copy link
Member

t8m commented Aug 31, 2022

Likely I'll set up a PR that fixes the doc for 1.1.1 - there missingcrypto111.txt should be updated, right?
I'm not sure it's worth having a 3rd PR for fixing the doc in 3.0 - what do you think?

There is no missingcrypto*.txt in 1.1.1. However you can do just one PR for 3.0 and handle the inevitable conflict on the missing missingcrypto when cherry-picking it to 1.1.1. As that is a trivial conflict.

t8m
t8m requested changes Aug 31, 2022
View reviewed changes
t8m
t8m approved these changes Aug 31, 2022
View reviewed changes
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.

@paulidale still OK?

@DDvO DDvO mentioned this pull request Sep 1, 2022
Closed
@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 Sep 1, 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 Sep 2, 2022
@openssl-machine
Copy link
Collaborator

openssl-machine commented Sep 2, 2022

This pull request is ready to merge

@DDvO
Copy link
Contributor Author

DDvO commented Sep 2, 2022

Merged to master - thanks @paulidale and @t8m

openssl-machine pushed a commit that referenced this pull request Sep 2, 2022
@DDvO
Fix many inconsistencies in doc of CMS_verify() and PKC7_verify() etc.
cae1d08 
Also change B< to I< in {CMS,PKCS7}_verify.pod, PKCS7_sign{,_add_signer}.pod

Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: David von Oheimb <[email protected]>
(Merged from #18915)
openssl-machine pushed a commit that referenced this pull request Sep 2, 2022
@DDvO
PKCS7: add notes to pkcs7.h.in and minor code cleanup in crypto/{pkcs…
f69ec4b 
…7,cms}/

Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: David von Oheimb <[email protected]>
(Merged from #18915)
@DDvO DDvO closed this Sep 2, 2022
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
@DDvO @sftcd
Fix many inconsistencies in doc of CMS_verify() and PKC7_verify() etc.
5b8533d 
Also change B< to I< in {CMS,PKCS7}_verify.pod, PKCS7_sign{,_add_signer}.pod

Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: David von Oheimb <[email protected]>
(Merged from openssl#18915)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
@DDvO @sftcd
PKCS7: add notes to pkcs7.h.in and minor code cleanup in crypto/{pkcs…
f479a91 
…7,cms}/

Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: David von Oheimb <[email protected]>
(Merged from openssl#18915)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@DDvO @beldmit
Fix many inconsistencies in doc of CMS_verify() and PKC7_verify() etc.
52daf1e 
Also change B< to I< in {CMS,PKCS7}_verify.pod, PKCS7_sign{,_add_signer}.pod

Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: David von Oheimb <[email protected]>
(Merged from openssl#18915)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@DDvO @beldmit
PKCS7: add notes to pkcs7.h.in and minor code cleanup in crypto/{pkcs…
f071b81 
…7,cms}/

Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: David von Oheimb <[email protected]>
(Merged from openssl#18915)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t8m t8m t8m approved these changes

@paulidale paulidale paulidale approved these changes

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch severity: fips change The pull request changes FIPS provider sources triaged: cleanup The issue/pr deals with cleanup of comments/docs not altering code significantly triaged: documentation The issue/pr deals with documentation (errors)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@DDvO @openssl-machine @t8m @paulidale