This patch adds cades support for openssl #7611

opensignature · 2018-11-10T18:21:05Z

Checklist

documentation is added or updated
tests are added or updated

apps/cms.c

mspncp

Thank you for contributing to OpenSSL, @opensignature. Without having looked too much into the details, it is my strong believe that in this part of your pull request you are implementing a feature which should be implemented in a reusable fashion in libcrypto and made available by introducing an API call. The purpose of the command line application is to make the functionality in libcrypto and libssl available on the command line, not to implement new features by itself.

Also, it would be good if you could find out whether your editor is able to visualize whitespace errors, for example trailing spaces or tabs instead of spaces. (Note: the git diff command will highlight trailing spaces).

apps/cms.c

mspncp · 2018-11-17T13:54:32Z

Also, it is not clear to me how much of the 141 pages of RFC5126 your pull request in fact implements. Please give the necessary references, because our policy mandates that we only add cryptographic algorithms which are covered by some official standard.

Also, adding new features - in particular api changes - require adding the corresponding documentation (updating the manual pages, etc.).

mspncp · 2018-11-17T13:59:07Z

Finally, the legal part: Every nontrivial contribution requires that the contributor fills out and signs a contributor license agreement. For details, please see https://www.openssl.org/policies/cla.html.

opensignature · 2018-11-17T16:17:54Z

Finally, the legal part: Every nontrivial contribution requires that the contributor fills out and signs a contributor license agreement. For details, please see https://www.openssl.org/policies/cla.html.

Thanks, I will provide soon

opensignature · 2018-11-17T16:32:30Z

Also, it is not clear to me how much of the 141 pages of RFC5126 your pull request in fact implements. Please give the necessary references, because our policy mandates that we only add cryptographic algorithms which are covered by some official standard.

Also, adding new features - in particular api changes - require adding the corresponding documentation (updating the manual pages, etc.).

A CAdES Basic Electronic Signature (CAdES-BES) contains, among other specifications, a collection of mandatory signed attributes, between these ESS signing-certificate or ESS signing-certificate-v2.
Section 5.7.3 of RFC (https://tools.ietf.org/html/rfc5126#section-5.7.3) provides the details of these attributes.
Patch add ESS signing-certificate-v2 attribute to signedData.
I am aware that the few lines of the patch can not add all the features provided by the standard, however this patch is sufficient, in many cases, to make the file signed with openssl, legal (for example it complies with Italian law)

apps/cms.c

FdaSilvaYY · 2018-11-18T11:19:10Z

I see you have opened PR #206 ;)
You can take inspiration from PR #771, and reuse its code :

extract and publish its API

static ESS_SIGNING_CERT_V2 *ess_signing_cert_v2_new_init(const EVP_MD *hash_alg,
                                                         X509 *signcert,
                                                         STACK_OF(X509)  *certs);
static ESS_CERT_ID_V2 *ess_cert_id_v2_new_init(const EVP_MD *hash_alg,
                                               X509 *cert, int issuer_needed);
static int ess_add_signing_cert_v2(PKCS7_SIGNER_INFO *si,
                                   ESS_SIGNING_CERT_V2 *sc);

These methods should be renamed to respect current conventions:
ess_signing_cert_v2_new_init => ESS_SIGNING_CERT_V2_new_init
ess_cert_id_v2_new_init => ESS_CERT_ID_V2_new_init
...
Edit: I made a confusion between CMS_SignerInfo, and PKCS7_SIGNER_INFO ;)
=> a CMS_add1_signing_cert_v2 method should be added, similar to ess_add_signing_cert_v2.

Document them
rewrite your CAdES signing code to reuse them
make it work even when building with no-ts
This may be need some renaming and code moving as these methods are no more TS specific code.
... moved to to cms_ess.c , i bet.

it is a large project ;)

opensignature · 2018-11-18T15:32:49Z

ok thanks, I will follow your advice :)

richsalz

Some feedback/nits.

crypto/cms/cms_ess.c

util/libcrypto.num

mattcaswell · 2018-11-19T16:06:13Z

Close/reopen to kick CLA bot.

mattcaswell · 2018-11-19T16:09:49Z

Please update all commits so that the author email matches that on the CLA that you registered. Without that the CLA bot will continue to complain.

mspncp · 2018-12-10T08:47:51Z

Will digging the matter, i do the core changes myself to uncover all the traps.
I may share my branch too, as i don't mind having my name in final commits.

Let's wait for @opensignature's reaction. If we teach him how to do it himself, we might manage to attract a regular contributor for the future... :-)

PS: @mspncp : you write at least 2 or 3 crystal-clear guides about Git manipulations ( amend, rebase .. ) , in previous PRs, who deserve to be saved in a Wiki page , as reference . ;)

Thanks for the compliments. Unfortunately I don't have the time to excavate and edit those guides. So feel encouraged to 'steel' them and place them into the Wiki. (No need to mention my authorship, it's all from Scott Chacon's book anyway.)

opensignature · 2018-12-10T16:10:26Z

ok, in fact there is a bit of disorder.
What steps do you suggest?
Close this PR?
Create branch add-cades on my github fork (already done)
Delete master?
Concentrate only on changes to the crypto and leave those to cms cli at a later time?
Can i create a PR from git cli?
Thanks

mspncp · 2018-12-10T16:16:01Z

What steps do you suggest?

I‘ll write it up for you, but I can’t promise it for today, and tomorrow I‘m out of office. Please have a little patience.

opensignature · 2018-12-10T16:20:25Z

Don't worry. I'm not in a hurry :)

YuryStrozhevsky · 2018-12-12T05:41:46Z

I do suggest to have SigningCertificateV2 attribute to be a default attribute for all CMS signatures made by OpenSSL. By doing this you will have a chance to people in future extene any OpenSSL signature to any CAdES signature level (higher than CAdES-BES). Similar already exists in Russian cryptoprovider product from Crypto-Pro: there by default all signatures already have all CAdES-BES attributes + there is a specific API flag preventing from making CAdES-BES by default. It is really helpful when there are needs having CAdES higher level signatures.

mspncp · 2018-12-12T07:01:11Z

@YuryStrozhevsky thank you very much for your comment. I looks like you have experience with CAdES? If yes, it would be highly appreciated if you could take a deeper look at @opensignature's pull request and provide more technical feedback.

mspncp · 2018-12-12T07:07:31Z

@opensignature I will help you with the first step and untangle the merge commits and rebase your branch. This is a little bit more tricky than usual due to the version number change, so it is not a good starting point to learn. The second step will be to reorganize the commits, which you will do with my help. I will need a little bit more time for preparing everything.

Until then, you can start by reading my initial draft for a TUTORIAL which is intended for novice Git[Hub] users. (Thanks to @FdaSilvaYY for animating me to do a general writeup.)

(Edit: the tutorial was moved from master to a feature branch)

YuryStrozhevsky · 2018-12-12T07:47:07Z

@mspncp Yes, I have an expertise in CAdES. And I already done some work on the pull request. But anyway I will continue with the inspection. From the first time all looks more or less good.

opensignature · 2018-12-12T08:29:31Z

I agree with Yury and also suggest to have a SigningCertificateV2 attribute to be a default attribute.
Currently in OpenSSL CMS the Signed Data structure includes several CMS signedAttributes including the signing time, the CMS content type and the supported list of ciphers in an SMIMECapabilities attribute (if CMS_NOATTR is set then no signedAttributes will be used).
I suggest adding SigningCertificateV2 to these signedAttributes :)

mspncp · 2018-12-12T22:57:26Z

The pull requests mspncp#4 mspncp#5 and mspncp#6 show the different stages of the transformation of your original pull request.

Step 1 - Add CAdES Support - merged mspncp/openssl#4 your pull request + a final merge commit from master
Step 2 - Add CAdES Support - rebased mspncp/openssl#5 after rebasing
crypto/cms: Add support for CAdES Basic Electronic Signatures (CAdES-BES) mspncp/openssl#6 after squashing

Note that the branches add-cades-support-merged, add-cades-support-rebased and add-cades-support-squashed have identical trees (i.e, the end-result is provable the same). You can see this by comparing the hash values ot the three tree objects:

msp@msppc:~/src/openssl$ for b in merged rebased squashed; do branch=add-cades-support-$b ; echo $branch ; git cat-file commit $branch ; done
add-cades-support-merged
tree 3cb8dec414acca63c1a3ffa618b1fcca266a62da
parent 5418ae0e147253a07781415101ee0996642c4426
parent 00eb879f74971e3c048286ef44f6f544676f90d7
author Dr. Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> 1544642546 +0100
committer Dr. Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> 1544642652 +0100

Merge branch 'master' into rework-cades-ori
add-cades-support-rebased
tree 3cb8dec414acca63c1a3ffa618b1fcca266a62da
parent 047c55b20f7b7cec2a22103b23a597765672cbff
author Dr. Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> 1544649792 +0100
committer Dr. Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> 1544649792 +0100

make update
add-cades-support-squashed
tree 3cb8dec414acca63c1a3ffa618b1fcca266a62da
parent 00eb879f74971e3c048286ef44f6f544676f90d7
author Dr. Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> 1544652529 +0100
committer Dr. Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> 1544652529 +0100

crypto/cms: Add support for CAdES Basic Electronic Signatures (CAdES-BES)

A CAdES Basic Electronic Signature (CAdES-BES) contains, among other
specifications, a collection of  Signing Certificate reference attributes,
stored in the signedData ether as ESS signing-certificate or as
ESS signing-certificate-v2. These are described in detail in Section 5.7.2
of RFC 5126 - CMS Advanced Electronic Signatures (CAdES).

This patch adds support for adding  ESS signing-certificate[-v2] attributes
to CMS signedData. Although it implements only a small part of the RFC, it
is sufficient many cases to enable the `openssl cms` app to create signatures
which comply with legal requirements of some European States (e.g Italy).

Alternatively, you can verify it if from the following output, which shows commit-id -> tree-id:

msp@msppc:~/src/openssl$ for b in merged rebased squashed; do branch=add-cades-support-$b ; echo "$branch:" ; echo "$(git rev-parse $branch) -> $(git rev-parse $branch^{tree})"; done
add-cades-support-merged:
92b47b6f7ce3c962d734d7f19ec72678d9186840 -> 3cb8dec414acca63c1a3ffa618b1fcca266a62da
add-cades-support-rebased:
d04cbacc76b3579c329938f03daab33c1883b014 -> 3cb8dec414acca63c1a3ffa618b1fcca266a62da
add-cades-support-squashed:
10c3ece252dd2f68d7378a8e59105f4accefb4ba -> 3cb8dec414acca63c1a3ffa618b1fcca266a62da

TO BE CONTINUED...

mspncp · 2018-12-12T23:12:06Z

Summary

Since you are adding a completely new feature, it makes sense to add it as a single large commit with a nice explaining commit message. Because when the new feature is ready and merged to master, nobody actually want's to see a complete history anymore of how the new feature was conceived and implemented over the time, with all the errors and corrections (mspncp#5). It is much nicer to get the final result with a nice explanation (mspncp#6). That's what "rewriting the history" in git is all about.

TO BE CONTINUED...

(Note: My commit message in mspncp#6 is just an example which I sketched and which should only give you an idea how the pull request can look like. (It might even be technically wrong or incomplete.)

mspncp · 2018-12-12T23:24:26Z

Since you are adding a completely new feature, it makes sense to add it as a single large commit

Of course it is equally justified to split it into a small number of separate commits wich split the work into logical parts, with explaining commit messages. But no history, no error fixups in the final commit.

mspncp · 2018-12-12T23:24:42Z

There is one caveat: rewriting the history (with interactive rebasing and squashing) is a valid method as long as your pull request is still in the work-in-progress state. (Contributors add "WIP: " to the pr title to indicate that it's work-in-progress). When the final review process has started, you should avoid actions that change existing commits (amending, rebasing, squashing) and only push additional fixups, unless the reviewer tells you otherwise.

mspncp · 2018-12-12T23:28:05Z

Homework Excercise: try to get the three branches add-cades-support-merged, add-cades-support-rebased and add-cades-support-squashed from my GitHub clone into your local repository. :-)

mspncp · 2018-12-12T23:36:23Z

Homework 2: commit 10c3ece lists me as author, which is obviously fake news. You can change the author of a commit retrospectively using git commit --amend --author=<author>. Here you need to specify exactly the e-mail address which you specified in the CLA and which you configured for your local repository.

mspncp · 2018-12-12T23:48:02Z

Ok @opensignature, enough for today. I hope I did not bury you with information. I'd be happy to hear some feedback from you.

My favorite choice would be if you would fetch the add-cades-support-squashed branch from my clone as a starting point, optionally rework and/or split the commit, and then create a fresh pull request against master, which replaces this one here. But it's your pull request, so it's up to you what you want to do.

opensignature · 2018-12-13T15:44:34Z

Homework Excercise: try to get the three branches add-cades-support-merged, add-cades-support-rebased and add-cades-support-squashed from my GitHub clone into your local repository. :-)

Homework Excercise done :-)

mspncp · 2018-12-13T15:54:31Z

Note: In case you wonder why git log --oneline add-cades-support-merged does not show your commits but the ones on master: as a merge commit it has two parents and git log is showing only one ancestry line. To get the history of the other parent, use git log --oneline add-cades-support-merged^, or to see both branches, use git log --oneline --graph add-cades-support-merged

Conflicts: util/libcrypto.num

opensignature · 2018-12-13T17:02:28Z

Now add-cades is synchronized with upstream. How can I fetch your add-cades-support-squashed into add-cades?

mspncp · 2018-12-13T17:16:08Z

The intended proceeding is not that you repeat the steps I did to rebase, but to abandon this pull request and open a new pull request using add-cades-support-squashed as a starting point, which contains all your changes up to now.

fetch add-cades-support-squashed into your local copy and check it out
change the author to your e-mail address and edit the commit message as you like (using git commit --amend --author=<author>)
(optionally) split the commit in logical parts
create a new pull request against master

mspncp · 2018-12-13T17:18:17Z

You don't need to keep my branch name, you can just

git checkout -b your-favorite-branch-name  add-cades-support-squashed

mspncp · 2018-12-13T17:22:22Z

I don't recommend to keep this pull request, because it uses your master branch and not a dedicated feature branch.

opensignature · 2018-12-13T18:59:52Z

ok done PR 7893

mspncp · 2018-12-13T20:58:02Z

Closing in favor of #7893.

mspncp · 2019-10-25T16:51:07Z

PS: @mspncp : you write at least 2 or 3 crystal-clear guides about Git manipulations ( amend, rebase .. ) , in previous PRs, who deserve to be saved in a Wiki page , as reference . ;)

@FdaSilvaYY did you save any of these links you mentioned? If yes, could you send me the links via e-mail? I'd be happy to have them, because I plan to do some documentation.

FdaSilvaYY · 2019-10-29T20:30:37Z

@FdaSilvaYY did you save any of these links you mentioned? If yes, could you send me the links via e-mail? I'd be happy to have them, because I plan to do some documentation.
Hi @mspncp, I'm currently digging into my mailbox.
Will send you the results ;)

opensignature added 3 commits November 10, 2018 19:12

Add files via upload

6509be5

Delete cms.c

3610180

Add files via upload

732536a

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Nov 10, 2018

FdaSilvaYY reviewed Nov 12, 2018

View reviewed changes

apps/cms.c Outdated Show resolved Hide resolved

FdaSilvaYY reviewed Nov 12, 2018

View reviewed changes

apps/cms.c Outdated Show resolved Hide resolved

FdaSilvaYY reviewed Nov 12, 2018

View reviewed changes

apps/cms.c Outdated Show resolved Hide resolved

opensignature added 2 commits November 17, 2018 12:43

Add files via upload

acd1d2d

Add files via upload

142b848

mspncp requested changes Nov 17, 2018

View reviewed changes

apps/cms.c Outdated Show resolved Hide resolved

apps/cms.c Outdated Show resolved Hide resolved

apps/cms.c Outdated Show resolved Hide resolved

apps/cms.c Outdated Show resolved Hide resolved

Add files via upload

78d88e7

FdaSilvaYY reviewed Nov 18, 2018

View reviewed changes

apps/cms.c Outdated Show resolved Hide resolved

opensignature added 5 commits November 18, 2018 19:50

Add files via upload

fc2318e

Add files via upload

4fe6a48

Add files via upload

856c968

Add files via upload

97b65e2

Add files via upload

41ea6cf

richsalz reviewed Nov 18, 2018

View reviewed changes

FdaSilvaYY reviewed Nov 18, 2018

View reviewed changes

util/libcrypto.num Outdated Show resolved Hide resolved

mattcaswell closed this Nov 19, 2018

mattcaswell reopened this Nov 19, 2018

Add files via upload

edb085b

mspncp mentioned this pull request Dec 12, 2018

Step 1 - Add CAdES Support - merged mspncp/openssl#4

Closed

Merge remote-tracking branch 'upstream/master'

66537c2

Conflicts: util/libcrypto.num

mspncp closed this Dec 13, 2018

opensignature mentioned this pull request Dec 14, 2018

Add support for CAdES Basic Electronic Signatures (CAdES-BES) #7893

Closed

2 tasks

This patch adds cades support for openssl #7611

This patch adds cades support for openssl #7611

Conversation

opensignature commented Nov 10, 2018 • edited

Checklist

mspncp left a comment • edited

Choose a reason for hiding this comment

mspncp commented Nov 17, 2018 • edited

mspncp commented Nov 17, 2018

opensignature commented Nov 17, 2018

opensignature commented Nov 17, 2018

FdaSilvaYY commented Nov 18, 2018 • edited

opensignature commented Nov 18, 2018

richsalz left a comment

Choose a reason for hiding this comment

mattcaswell commented Nov 19, 2018

mattcaswell commented Nov 19, 2018

mspncp commented Dec 10, 2018

opensignature commented Dec 10, 2018

mspncp commented Dec 10, 2018

opensignature commented Dec 10, 2018

YuryStrozhevsky commented Dec 12, 2018

mspncp commented Dec 12, 2018

mspncp commented Dec 12, 2018 • edited

YuryStrozhevsky commented Dec 12, 2018

opensignature commented Dec 12, 2018

mspncp commented Dec 12, 2018

mspncp commented Dec 12, 2018

Summary

mspncp commented Dec 12, 2018

mspncp commented Dec 12, 2018 • edited

mspncp commented Dec 12, 2018

mspncp commented Dec 12, 2018

mspncp commented Dec 12, 2018

opensignature commented Dec 13, 2018

mspncp commented Dec 13, 2018 • edited

opensignature commented Dec 13, 2018

mspncp commented Dec 13, 2018 • edited

mspncp commented Dec 13, 2018

mspncp commented Dec 13, 2018

opensignature commented Dec 13, 2018

mspncp commented Dec 13, 2018

mspncp commented Oct 25, 2019

FdaSilvaYY commented Oct 29, 2019

opensignature commented Nov 10, 2018 •

edited

mspncp left a comment •

edited

mspncp commented Nov 17, 2018 •

edited

FdaSilvaYY commented Nov 18, 2018 •

edited

mspncp commented Dec 12, 2018 •

edited

mspncp commented Dec 12, 2018 •

edited

mspncp commented Dec 13, 2018 •

edited

mspncp commented Dec 13, 2018 •

edited