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

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

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@opensignature
Copy link

opensignature commented Dec 13, 2018

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).

(migrated from #7611)

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

@opensignature opensignature referenced this pull request Dec 13, 2018

Closed

This patch adds cades support for openssl #7611

2 of 2 tasks complete

@mspncp mspncp self-assigned this Dec 13, 2018

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Dec 13, 2018

Ok, so one step was made, but the task is not completed yet. You now have a feature branch but it still contains all the 40+ unsquashed commits, instead of the single squashed commit. Don't worry, we can fix it without creating a new pull request. Your local branch needs to be reset to the squashed commit and then your local branch with the squashed needs to be force-pushed.

You have two options:

  • You ask me to do the fix for you. I have push access to your feature branch, unless you unchecked the 'allow edits from maintainers' check button.
  • You want to do it yourself. Then I can send you the necessary commands to do it.

But it will take a little while because I won't be able to do it this evening.

@opensignature

This comment has been minimized.

Copy link

opensignature commented Dec 13, 2018

'allow edits from maintainers' check button is checked however if you want to send me the commands or indicate me a link where to learn them, I can do it

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Dec 13, 2018

I can do it

Ok, then you will do it. Can you please post the output of the following two commands? You can either post it here or send it to my email address (which you find in the git log).

git remote -v
git branch -vv

@opensignature opensignature force-pushed the opensignature:add-cades branch from 01ded21 to 5000701 Dec 14, 2018

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Dec 14, 2018

Cool! You did it! Congratulations!

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Dec 14, 2018

Next question is: is your pull request ready for final review or still work-in-progress? If the latter holds, then please edit the pull request title (not the commit message title) and add the "WIP:" prefix. There is an [Edit] button right next to it at the top of this page.

WIP: Add support for CAdES Basic Electronic Signatures (CAdES-BES)

@mspncp mspncp removed their assignment Dec 14, 2018

@opensignature

This comment has been minimized.

Copy link

opensignature commented Dec 14, 2018

For me this pull request is ready for final review

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Dec 14, 2018

git branch -vv

Oh, I learned something today...

@levitte
Copy link
Member

levitte left a comment

First read through.

FYI, I usually do more than one read through, so there may be other change requests coming up. This all looks pretty good, though, so I don't expect much changes will be needed.

Show resolved Hide resolved include/openssl/ess.h Outdated
@FdaSilvaYY

This comment has been minimized.

Copy link
Contributor

FdaSilvaYY commented Dec 14, 2018

In line with my previous comment , I specify a bit more my point of view:

A large part of code is already in the project,
See crypto/ts/ts_rsp_sign.c

You just need to extract from this file :

ESS_SIGNING_CERT *ESS_SIGNING_CERT_new_init(X509 *signcert, STACK_OF(X509) *certs);
ESS_SIGNING_CERT_V2 *ESS_SIGNING_CERT_V2_new_init(const EVP_MD *hash_alg, X509 *signcert, STACK_OF(X509) *certs);

Add this two dedicated simple methods :

int CMS_add1_signing_cert(CMS_SignerInfo *si, ESS_SIGNING_CERT *sc);
int CMS_add1_signing_cert_v2(CMS_SignerInfo *si, ESS_SIGNING_CERT_V2 *sc);

and reuse them in your code, and in ts_rsp_sign.c

Show resolved Hide resolved crypto/cms/cms_ess.c Outdated
Show resolved Hide resolved crypto/cms/cms_ess.c Outdated
@maxcuttins

This comment has been minimized.

Copy link

maxcuttins commented Dec 17, 2018

Wow!!!!! :)
This feature seems close to release!!!

THANKS TO EVERYBODY!

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Dec 17, 2018

Wow!!!!! :)
This feature seems close to release!!!

THANKS TO EVERYBODY!

Thanks @maxcuttins. BTW, review and feedback from OpenSSL users are welcome at any time 😃.

@opensignature it might be an option for you to announce the new CAdES feature on the openssl-users mailing list together with a link to this pull request and to ask for additional feedback from other users. Of course that's up to you.

@opensignature

This comment has been minimized.

Copy link

opensignature commented Dec 17, 2018

I agree @FdaSilvaYY, I hope in this last commit to have correctly understood your suggestion

Show resolved Hide resolved include/openssl/ess.h Outdated
Show resolved Hide resolved include/openssl/ess.h Outdated
Show resolved Hide resolved include/openssl/ess.h Outdated
Show resolved Hide resolved include/openssl/ess.h Outdated
@opensignature

This comment has been minimized.

Copy link

opensignature commented Dec 18, 2018

What do you think about adding a new sub-library to OpenSSL, crypto/ess with ess_lib.c, ess_err.c, ecc. ?

@opensignature opensignature changed the title Add support for CAdES Basic Electronic Signatures (CAdES-BES) WIP: Add support for CAdES Basic Electronic Signatures (CAdES-BES) Dec 18, 2018

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Dec 18, 2018

What do you think about adding a new sub-library to OpenSSL, crypto/ess with ess_lib.c, ess_err.c, ecc. ?

Considering the API is a whole bunch of ESS_ functions, it does make sense.

@opensignature

This comment has been minimized.

Copy link

opensignature commented Dec 18, 2018

Now it all seems more logical to me that ESS functions are inside crypto/ess and rest on ts and cms (in this the two CAdES functions have remained).
Thanks to @FdaSilvaYY and @levitte for useful tips and @mspncp for lessons (also in private) about git 👍

@opensignature opensignature changed the title WIP: Add support for CAdES Basic Electronic Signatures (CAdES-BES) Add support for CAdES Basic Electronic Signatures (CAdES-BES) Dec 18, 2018

Show resolved Hide resolved doc/man1/cms.pod Outdated
Show resolved Hide resolved doc/man3/CMS_add1_signing_cert_v2.pod Outdated
Show resolved Hide resolved include/openssl/ess.h
Show resolved Hide resolved crypto/ts/ts_rsp_verify.c Outdated
Show resolved Hide resolved crypto/ts/ts_rsp_sign.c Outdated
Show resolved Hide resolved crypto/ess/ess_lib.c Outdated
Show resolved Hide resolved Configure Outdated
@maxcuttins

This comment has been minimized.

Copy link

maxcuttins commented Dec 20, 2018

Almost! :)
Thanks to everybody.

@opensignature you are a hero!

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Dec 20, 2018

@opensignature you are a hero!

Hey @opensignature, you already acquired a fan club! ;-)

@levitte
Copy link
Member

levitte left a comment

Since you removed "ess" from the disablable features, there's no need to check OPENSSL_NO_ESS

Show resolved Hide resolved crypto/err/err_all.c Outdated
Show resolved Hide resolved include/openssl/ess.h Outdated
Show resolved Hide resolved include/openssl/esserr.h Outdated
Show resolved Hide resolved util/libcrypto.num Outdated
@opensignature

This comment has been minimized.

Copy link

opensignature commented Dec 21, 2018

Test:
(with -cades option)
util/shlib_wrap.sh apps/openssl cms -sign -cades -in test/smcont.txt -outform DER -nodetach -certfile test/smime-certs/smroot.pem -md sha256 -signer test/smime-certs/smrsa1.pem -out testcades.p7m

and
(without -cades)
util/shlib_wrap.sh apps/openssl cms -sign -in test/smcont.txt -outform DER -nodetach -certfile test/smime-certs/smroot.pem -md sha256 -signer test/smime-certs/smrsa1.pem -out testnocades.p7m

try to upload result file here: dss.agid.gov.it/validation

in the first case the result is: Signature format: CAdES-BASELINE-B
in the second: Signature format: CMS-NOT-ETSI

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Jan 9, 2019

When the merge conflict is resolved, you can continue the rebase operation:

git rebase --continue

Sometimes git will complain, that the patch is empty. (This is the case if you reverted all changes.). Then just skip the patch:

git rebase --skip

This procedure will have to be repeated several times.

Alternatively, if you agree that the pr can be squashed into a single commit at the end, you don't have to run Configure ... ; make update at every conflicting commit. Just revert libcrypto.num every time git stops at a merge conflict

git checkout --ours util/libcrypto.num
git add util/libcrypto.num
git rebase --continue

And run Configure ... ; make update once at the very end of the rebase.

Finally, you need to force-push your changes, because you local branch has moved.

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Jan 9, 2019

Side note to @FdaSilvaYY: there is no need to do a git reset --hard HEAD^^ before doing the rebase, because git will automatically detect and drop the merge commits from the rebase-todo-.list.

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).
@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Jan 9, 2019

@opensignature if this all turns out to be too tedious, you can just 'steal' the result from my repository.

I would recommend to take the squashed version, because the history of bugfixes is irrelevant. You might however want to split the commit into logical parts, like the moving of the ESS code to crypto/ess.

If you still have my repository configured as a remote 'ms' like last time, all you have to do is

git fetch --all
git checkout add-cades
git reset --hard ms/pr-7893-rebased-and-squashed

@opensignature opensignature force-pushed the opensignature:add-cades branch from 0734993 to 8a394ad Jan 10, 2019

@maxcuttins

This comment has been minimized.

Copy link

maxcuttins commented Jan 10, 2019

@opensignature you are a saint.

@paride

This comment has been minimized.

Copy link

paride commented Jan 10, 2019

@opensignature This is great, I have no words to thank you!

I have tested the current implementation and it did work smoothly. Now I have a .p7m file signed with my Italian CNS card. Let me know if you are aware of an easy way to check if the signature is recognized as valid by the public administration services.

Edit: I successfully verified the signature!

@mspncp
Copy link
Contributor

mspncp left a comment

In the introduction of your pull request you mentioned RFC 5126 as CAdES standard document.

However, in the CAdES Basic Electronic Signature (CAdES-BES) section of the cms(1) manual page the RFC is not mentioned at all. It's only briefly mentioned in CMS_add1_signing_cert(3).

Furthermore, according to the Abstract of RFC 5126

   The contents of this Informational RFC amount to a transposition of
   the ETSI Technical Specification (TS) 101 733 V.1.7.4 (CMS Advanced
   Electronic Signatures -- CAdES) and is technically equivalent to it.

the RFC is merely a transposition of the ETSI Technical Specification ETSI TS 101 733 V.1.7.4, which meanwhile has been updated and incorporated into an ETSI European Standard ETSI EN 319 122-1 V1.1.1. So it looks like this last reference is the current legally binding standard for CAdES, is that correct? If yes, shouldn't cms(1) reference this ETSI document?

Show resolved Hide resolved doc/man1/cms.pod Outdated
Show resolved Hide resolved doc/man1/cms.pod Outdated
Show resolved Hide resolved doc/man1/cms.pod Outdated
Show resolved Hide resolved doc/man3/CMS_add1_signing_cert.pod Outdated
Show resolved Hide resolved doc/man1/cms.pod
Show resolved Hide resolved doc/man3/CMS_add1_signing_cert.pod Outdated
Show resolved Hide resolved doc/man3/CMS_add1_signing_cert.pod Outdated
Show resolved Hide resolved doc/man3/CMS_add1_signing_cert.pod Outdated
Show resolved Hide resolved doc/man1/cms.pod Outdated
Show resolved Hide resolved doc/man1/cms.pod Outdated
Show resolved Hide resolved doc/man1/cms.pod Outdated
@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Jan 11, 2019

For the records: I created two signatures as described in #7893 (comment). Then I decoded testnocades.p7m and testcades.p7m using openssl asn1parse and stripped the offset prefixes from the lines of the output (which would have created unnecessary diffs). This is the diff I got:

msp@msppc:~/openssl/openssl$ diff testnocades.asn1 testcades.asn1

--- testnocades.asn1    2019-01-11 01:29:05.387973254 +0100
+++ testcades.asn1      2019-01-11 01:29:01.509932340 +0100
@@ -1,7 +1,7 @@
-d=0  hl=4 l=2516 cons: SEQUENCE
+d=0  hl=4 l=2662 cons: SEQUENCE
 d=1  hl=2 l=   9 prim: OBJECT            :pkcs7-signedData
-d=1  hl=4 l=2501 cons: cont [ 0 ]
-d=2  hl=4 l=2497 cons: SEQUENCE
+d=1  hl=4 l=2647 cons: cont [ 0 ]
+d=2  hl=4 l=2643 cons: SEQUENCE
 d=3  hl=2 l=   1 prim: INTEGER           :01
 d=3  hl=2 l=  13 cons: SET
 d=4  hl=2 l=  11 cons: SEQUENCE
@@ -135,8 +135,8 @@
 d=6  hl=2 l=   9 prim: OBJECT            :sha1WithRSAEncryption
 d=6  hl=2 l=   0 prim: NULL
 d=5  hl=4 l= 257 prim: BIT STRING
-d=3  hl=4 l= 609 cons: SET
-d=4  hl=4 l= 605 cons: SEQUENCE
+d=3  hl=4 l= 755 cons: SET
+d=4  hl=4 l= 751 cons: SEQUENCE
 d=5  hl=2 l=   1 prim: INTEGER           :01
 d=5  hl=2 l=  81 cons: SEQUENCE
 d=6  hl=2 l=  68 cons: SEQUENCE
@@ -155,7 +155,7 @@
 d=6  hl=2 l=   9 prim: INTEGER           :D93996EEA64B2040
 d=5  hl=2 l=  11 cons: SEQUENCE
 d=6  hl=2 l=   9 prim: OBJECT            :sha256
-d=5  hl=3 l= 228 cons: cont [ 0 ]
+d=5  hl=4 l= 373 cons: cont [ 0 ]
 d=6  hl=2 l=  24 cons: SEQUENCE
 d=7  hl=2 l=   9 prim: OBJECT            :contentType
 d=7  hl=2 l=  11 cons: SET
@@ -163,7 +163,7 @@
 d=6  hl=2 l=  28 cons: SEQUENCE
 d=7  hl=2 l=   9 prim: OBJECT            :signingTime
 d=7  hl=2 l=  15 cons: SET
-d=8  hl=2 l=  13 prim: UTCTIME           :190111002627Z
+d=8  hl=2 l=  13 prim: UTCTIME           :190111002503Z
 d=6  hl=2 l=  47 cons: SEQUENCE
 d=7  hl=2 l=   9 prim: OBJECT            :messageDigest
 d=7  hl=2 l=  34 cons: SET
@@ -191,7 +191,31 @@
 d=9  hl=2 l=  13 cons: SEQUENCE
 d=10 hl=2 l=   8 prim: OBJECT            :rc2-cbc
 d=10 hl=2 l=   1 prim: INTEGER           :28
+d=6  hl=3 l= 142 cons: SEQUENCE
+d=7  hl=2 l=  11 prim: OBJECT            :id-smime-aa-signingCertificateV2
+d=7  hl=2 l= 127 cons: SET
+d=8  hl=2 l= 125 cons: SEQUENCE
+d=9  hl=2 l= 123 cons: SEQUENCE
+d=10 hl=2 l= 121 cons: SEQUENCE
+d=11 hl=2 l=  32 prim: OCTET STRING      [HEX DUMP]:FD29526975DD9B09AE695D9FD73005D75A30A4DC8D334CB076EFF0B94BCE829C
+d=11 hl=2 l=  85 cons: SEQUENCE
+d=12 hl=2 l=  72 cons: SEQUENCE
+d=13 hl=2 l=  70 cons: cont [ 4 ]
+d=14 hl=2 l=  68 cons: SEQUENCE
+d=15 hl=2 l=  11 cons: SET
+d=16 hl=2 l=   9 cons: SEQUENCE
+d=17 hl=2 l=   3 prim: OBJECT            :countryName
+d=17 hl=2 l=   2 prim: PRINTABLESTRING   :UK
+d=15 hl=2 l=  22 cons: SET
+d=16 hl=2 l=  20 cons: SEQUENCE
+d=17 hl=2 l=   3 prim: OBJECT            :organizationName
+d=17 hl=2 l=  13 prim: UTF8STRING        :OpenSSL Group
+d=15 hl=2 l=  29 cons: SET
+d=16 hl=2 l=  27 cons: SEQUENCE
+d=17 hl=2 l=   3 prim: OBJECT            :commonName
+d=17 hl=2 l=  20 prim: UTF8STRING        :Test S/MIME RSA Root
+d=12 hl=2 l=   9 prim: INTEGER           :D93996EEA64B2040
 d=5  hl=2 l=  13 cons: SEQUENCE
 d=6  hl=2 l=   9 prim: OBJECT            :rsaEncryption
 d=6  hl=2 l=   0 prim: NULL
-d=5  hl=4 l= 256 prim: OCTET STRING      [HEX DUMP]:9FEA291963EAA89785BA5A340CBCDB382FBDA38099C052DD336B0D95625C1CDFAD158CE684A45C7210678F097508D59E9EDB72494FA77CBF4D3DB066E498CEC8F94E3BD95C55DE7A9ADAC971392A4987E6F9200A30D592710ED06AB82A1DD88A6830C2407ED3596A3D1E95A61E6D3ABDD6B3C3D60FAEAD407DA3C4D8E246D86F32D93A5705633180F0B493B56A8B6F41E93E675FB23033C505CEEBB4EEE27B20363C16233B1627DDF053B62A835C0AF36B7DDA201F6B314489F5456FD1DC9CA6E0E11CA1F78C99080D27CA82CD1C2DFC1A9F3AAD965316EE253EAC89BE8803BE9230891C59BEF3FB7A8AE7F1BA99E1CA83FA611AE665C1D7CF532DF242EC6ABB
+d=5  hl=4 l= 256 prim: OCTET STRING      [HEX DUMP]:D3110B2C53F9CF43E64C607EAC8A4F6B006AEFF97AA072BFA181B04455AA88BDEB2C42B90FF2AE81954E4BE5C8740D6641D2302FA001E297001D9A097AA64FFE3C08A571A1366D28844AFA27838B1884522B32046BBE5FA79D83AB7D576725472A9C2435307F2583699CFD427905066DE0073B3CD3AF8246C9A533EE678781C57F1733EACD87723650CE419B394161CB4483C5DDB52B707C03B4CFBE0AF2D67B8AE856C5CAE8C50E020C5ACD26BA6D62F6A49CAC09CA629EC84FE3B852954DA1340E0933097C1636517F542A3A65728CBD798696646C57324C9F89E3CABDEBE9BFA0F88DF86A0C1D70D1F4AADD3886AD060852C8B651221FC736C83329BBCD7A
@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Jan 11, 2019

Here the reports from uploading the signatures to https://dss.agid.gov.it/validation:

The testcades.p7m signature type was successfully recognized as CAdES-BASELINE-B, while the signature type of testnocades.p7m was classified as CMS-NOT-ETSI.

Show resolved Hide resolved include/openssl/tserr.h Outdated
Restore previous values TS_R_ESS_ADD_SIGNING_CERT_ERROR 116
TS_R_ESS_ADD_SIGNING_CERT_V2_ERROR 139
TS_R_ESS_SIGNING_CERTIFICATE_ERROR 101
@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Jan 13, 2019

The ASN1 output looks good to me.

I have just one question regarding the signingCertificateV2 signed attribute, because it took me a little while to understand how it comes to the fourfold nesting (SET > SEQUENCE > SEQUENCE > SEQUENCE).

 2249:d=7  hl=2 l=  11 prim:        OBJECT            :id-smime-aa-signingCertificateV2
 2262:d=7  hl=2 l= 127 cons:        SET
 2264:d=8  hl=2 l= 125 cons:         SEQUENCE
 2266:d=9  hl=2 l= 123 cons:          SEQUENCE
 2268:d=10 hl=2 l= 121 cons:           SEQUENCE
 2270:d=11 hl=2 l=  32 prim:            OCTET STRING      [HEX DUMP]:FD29526975DD9B09AE695D9FD73005D75A30A4DC8D334CB076EFF0B94BCE829C
 2304:d=11 hl=2 l=  85 cons:            SEQUENCE
 2306:d=12 hl=2 l=  72 cons:             SEQUENCE
 2308:d=13 hl=2 l=  70 cons:              cont [ 4 ]
 2310:d=14 hl=2 l=  68 cons:               SEQUENCE
 2312:d=15 hl=2 l=  11 cons:                SET
 2314:d=16 hl=2 l=   9 cons:                 SEQUENCE
 2316:d=17 hl=2 l=   3 prim:                  OBJECT            :countryName
 2321:d=17 hl=2 l=   2 prim:                  PRINTABLESTRING   :UK
 2325:d=15 hl=2 l=  22 cons:                SET
 2327:d=16 hl=2 l=  20 cons:                 SEQUENCE
 2329:d=17 hl=2 l=   3 prim:                  OBJECT            :organizationName
 2334:d=17 hl=2 l=  13 prim:                  UTF8STRING        :OpenSSL Group
 2349:d=15 hl=2 l=  29 cons:                SET
 2351:d=16 hl=2 l=  27 cons:                 SEQUENCE
 2353:d=17 hl=2 l=   3 prim:                  OBJECT            :commonName
 2358:d=17 hl=2 l=  20 prim:                  UTF8STRING        :Test S/MIME RSA Root
 2380:d=12 hl=2 l=   9 prim:             INTEGER           :D93996EEA64B2040

Is my understanding correct, that the SET comes form the rule for the attrValues in RFC 5652

# RFC 5652 - 5.3.  SignerInfo Type

	Attribute ::= SEQUENCE {
	   attrType OBJECT IDENTIFIER,
	   attrValues SET OF AttributeValue
	}

and the three SEQUENCEs are from the following rules (marked [1] - [3])?

# RFC 5035 - 5.4.1 Signing Certificate Attribute Definition Version 2

5.4.1 Signing Certificate Attribute Definition Version 2	

	SigningCertificateV2 ::=  SEQUENCE {                           [1]
	   certs        SEQUENCE OF ESSCertIDv2,                       [2]
	   policies     SEQUENCE OF PolicyInformation OPTIONAL
	}


	ESSCertIDv2 ::=  SEQUENCE {                                    [3]
		hashAlgorithm           AlgorithmIdentifier
			   DEFAULT {algorithm id-sha256},
		certHash                 Hash,
		issuerSerial             IssuerSerial OPTIONAL
	}

	Hash ::= OCTET STRING

	IssuerSerial ::= SEQUENCE {
		issuer                   GeneralNames,
		serialNumber             CertificateSerialNumber
	}
@mspncp
Copy link
Contributor

mspncp left a comment

@opensignature your pull request makes a very good impression now and it has improved a lot since the first uploaded version. So it's only a matter of time until it will be accepted, but I still need a few days to complete my review. I'll try to finish it this week. Here for the moment just three more little remarks.

And another nit: The pod files have four lines with trailing spaces, could you please remove them? The git diff command will highlight them for you if you run the command

git diff master... doc
Show resolved Hide resolved doc/man1/cms.pod
Show resolved Hide resolved doc/man1/cms.pod
Show resolved Hide resolved doc/man1/cms.pod
@opensignature

This comment has been minimized.

Copy link

opensignature commented Jan 14, 2019

and the three SEQUENCEs are from the following rules (marked [1] - [3])?

Yes

@mspncp
Copy link
Contributor

mspncp left a comment

Is my understanding correct that with your changes the ts_RESP_sign() function now effectively uses CMS_signed_add1_attr_by_NID(...) instead of PKCS7_add_signed_attribute() to add the signing certificate? If yes, is this change correct? Are those two function calls equivalent? And if they are, why are there two independent implementations for it?

Show resolved Hide resolved crypto/cms/cms_ess.c
Show resolved Hide resolved crypto/ts/ts_rsp_sign.c
Show resolved Hide resolved crypto/ts/ts_rsp_sign.c
@mspncp
Copy link
Contributor

mspncp left a comment

Up to now, the CAdES support is only implemented for signing but not for signature verification. I'm not saying that this should be implemented here. On the contrary, it would be a subject for a separate pull request (if it will be implemented at all). But it might confuse the casual user that openssl cms -sign -cades ... creates the signing certificate attribute, but openssl cms -verify -cades ... ignores it. So there should at least be some documentation in the manual pages.

(@FdaSilvaYY I'm not sure whether the openssl cms app should issue a warning or not in the case when the -cades option is ignored. What do you think?)

Show resolved Hide resolved doc/man1/cms.pod Outdated
Show resolved Hide resolved doc/man1/cms.pod
Show resolved Hide resolved crypto/ess/ess_lib.c Outdated

@opensignature opensignature force-pushed the opensignature:add-cades branch from 6a92ec9 to 5398de3 Jan 17, 2019

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Jan 20, 2019

@opensignature with your last force-push you removed commit 6a92ec9. I suppose this removal was accidentally? If yes, then you should be able to add it back in as follows:

git checkout add-cades
git cherry-pick 6a92ec9d0a4ef07b2cd5137438f85f26757286c7
git push origin add-cades
@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Jan 20, 2019

Oh, and e97839f is missing, too. You would need to cherry-pick that one first:

git checkout add-cades
git cherry-pick e97839fb9bbea962f6ce19e46d32602fc5218ce2 
git cherry-pick 6a92ec9d0a4ef07b2cd5137438f85f26757286c7
git push origin add-cades
@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Jan 20, 2019

(Note: it's better to avoid --force unless you explicitly did some rebasing. Then git will warn you whenever you are about to overwrite some changes on the remote.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment