CAdES: implement CAdES-BES signed attributes validation by FdaSilvaYY · Pull Request #8098 · openssl/openssl

FdaSilvaYY · 2019-01-27T21:22:50Z

CAdES-BES Signature Validation feature following #7893.

The core validation code comes from PR #771. this PR mostly shared and reuse its code.

Checklist

documentation is added or updated
tests are added or updated

levitte

First quick read

crypto/cms/cms_ess.c

levitte · 2019-01-28T09:43:54Z

Apart from my comment, can you have a look at Travis and figure out what's going on? Them are a few too many unresolved symbols for my comfort...

FdaSilvaYY · 2019-01-28T21:44:07Z

Apart from my comment, can you have a look at Travis and figure out what's going on? Them are a few too many unresolved symbols for my comfort...

Fully fixed, I should not edit my code so late ;)
CI's are Green.

maxcuttins · 2019-02-17T15:32:51Z

any news about this?

FdaSilvaYY · 2019-02-17T19:04:19Z

@maxcuttins : needs a bit of documents about the two added methods.
May be it is possible to get the signed attributes in a more fashion way .
Otherwise, the core changes are here.
I rebased this PR on top of #8117 to ease the merge.

FdaSilvaYY · 2019-05-25T19:00:27Z

Ping @slontis : suggested tests added.

slontis · 2019-05-27T00:56:22Z

Is this PR dependant on PR #8117 or should #8117 just be closed?

apps/cms.c

slontis · 2019-05-27T01:15:24Z

Just a general comment about the generated files..
I have started keeping a seperate commit for the generated files (since they quite often need to be updated when other PR's are merged). That way you can then

rebase and then remove the generated files commit
make update and then add the generated files again and commit.

apps/cms.c

FdaSilvaYY · 2020-05-11T16:33:12Z

Fully rebased. CI are all green.
Just need to think a bit more about no-ts, no-cms interaction with these changes.

opensignature · 2020-05-17T21:07:31Z

with make test TESTS=test_x509 V=1
there is this error:
crypto/pem/pvkfmt.c:42:18: runtime error: left shift of 137 by 24 places cannot be represented in type 'int'

mspncp · 2020-05-17T21:25:09Z

with make test TESTS=test_x509 V=1
there is this error:
crypto/pem/pvkfmt.c:42:18: runtime error: left shift of 137 by 24 places cannot be represented in type 'int'

Thanks @opensignature. @FdaSilvaYY already reported this issue in #11853.

t8m

Some minor things to resolve.

t8m · 2020-05-19T10:02:27Z

crypto/cms/cms_smime.c

Nit: It is weird to "optimize" boolean value into unsigned char. The int type is usually used in place of boolean throughout the OpenSSL code. Also I would slightly prefer using (flags & CMS_CADES) != 0 instead of the ? operator.

crypto/cms/cms_ess.c

t8m · 2020-05-19T10:10:06Z

crypto/cms/cms_smime.c

Optional: at this point scount == sk_CMS_SignerInfo_num(sinfos) so you could use scount here to optimize.

crypto/ess/build.info

t8m · 2020-05-19T10:27:31Z

crypto/ess/ess_lib.c

Would it make sense to just return ASN1_INTEGER_cmp(is->serial, X509_get_serialNumber((X509 *)cert)); here?

t8m · 2020-05-19T10:30:03Z

crypto/ess/ess_lib.c

This was now changed. You can also use X509v3_cache_extensions() directly.

t8m · 2020-05-19T10:31:24Z

crypto/ess/ess_lib.c

This will require change to use fetch. Perhaps just mark with TODO 3.0.

t8m · 2020-05-19T10:32:58Z

crypto/ess/ess_lib.c

This will have to be changed for 3.0 to use fetch. Please add TODO 3.0 comment.

bernd-edlinger · 2020-05-22T18:12:23Z

crypto/ess/ess_lib.c

I think that X509_digest should do this automatically.

for signing certificate V2 and signing certificate extensions. CAdES : lowercase name for now internal methods.

about the two new added methods and two new error codes.

[extended tests]

…ning certificate V2 and signing certificate extensions.

FdaSilvaYY · 2020-05-23T20:38:42Z

Travis builds are Green, except usual s390 "no space left" ...

t8m

Good work!

t8m · 2020-05-25T09:37:28Z

@slontis Can you please re-approve?

slontis

Reapproved..

Thanks for your patience with this PR @FdaSilvaYY.

mspncp · 2020-05-26T23:25:00Z

This pull request is ready to merge.

for signing certificate V2 and signing certificate extensions. CAdES: lowercase name for now internal methods. crypto/cms: generated file changes. Add some CHANGES entries. [extended tests] Reviewed-by: Shane Lontis <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #8098)

t8m · 2020-05-27T11:21:31Z

Squashed and merged as 9e3c510. Thank you very much for the PR and your patience when waiting for the reviews and merge.

FdaSilvaYY · 2020-05-27T16:12:26Z

Thanks for all your reviews and remarks.

FdaSilvaYY force-pushed the add-cades-verify branch 2 times, most recently from 1b03cee to 19626ad Compare January 28, 2019 00:08

mspncp mentioned this pull request Jan 28, 2019

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

Closed

2 tasks

levitte requested changes Jan 28, 2019

View reviewed changes

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

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

FdaSilvaYY force-pushed the add-cades-verify branch 2 times, most recently from 2584aa5 to f4cb69e Compare January 28, 2019 21:31

FdaSilvaYY force-pushed the add-cades-verify branch from f4cb69e to 42a6476 Compare January 28, 2019 21:50

FdaSilvaYY force-pushed the add-cades-verify branch 2 times, most recently from b20fafe to 53c49de Compare February 17, 2019 18:54

FdaSilvaYY force-pushed the add-cades-verify branch 2 times, most recently from fb5c13b to 404e7f3 Compare February 19, 2019 19:49

FdaSilvaYY force-pushed the add-cades-verify branch from 404e7f3 to 572f67c Compare February 27, 2019 20:35

FdaSilvaYY force-pushed the add-cades-verify branch from 572f67c to f715df7 Compare March 10, 2019 10:36

FdaSilvaYY force-pushed the add-cades-verify branch 2 times, most recently from 288d872 to 40ad55c Compare April 4, 2019 20:13

FdaSilvaYY changed the title ~~CAdes: implement CAdES-BES signed attributes validation~~ CAdES: implement CAdES-BES signed attributes validation May 25, 2019

FdaSilvaYY mentioned this pull request May 25, 2019

CAdES: Fix SignerInfo attribute construction order. #8117

Closed

2 tasks

FdaSilvaYY force-pushed the add-cades-verify branch 3 times, most recently from 02258ec to 739f0d1 Compare May 25, 2019 18:59

FdaSilvaYY force-pushed the add-cades-verify branch from 739f0d1 to b2befc8 Compare May 25, 2019 19:08

slontis reviewed May 27, 2019

View reviewed changes

apps/cms.c Outdated Show resolved Hide resolved

slontis reviewed May 27, 2019

View reviewed changes

apps/cms.c Outdated Show resolved Hide resolved

FdaSilvaYY force-pushed the add-cades-verify branch 2 times, most recently from 60589f2 to eea4f13 Compare May 10, 2020 19:26

FdaSilvaYY force-pushed the add-cades-verify branch from eea4f13 to d6a6f12 Compare May 16, 2020 21:28

t8m requested changes May 19, 2020

View reviewed changes

FdaSilvaYY force-pushed the add-cades-verify branch from d6a6f12 to a8354d2 Compare May 22, 2020 17:11

bernd-edlinger reviewed May 22, 2020

View reviewed changes

crypto/ess/ess_lib.c Outdated

Copy link

Member

bernd-edlinger May 22, 2020

Choose a reason for hiding this comment

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

I think that X509_digest should do this automatically.

FdaSilvaYY added 6 commits May 23, 2020 18:52

crypto/cms: add CAdES-BES signed attributes validation

244fc82

for signing certificate V2 and signing certificate extensions. CAdES : lowercase name for now internal methods.

crypto/cms: generated file changes.

7e5e002

about the two new added methods and two new error codes.

Style nit fix, plus arguments constification.

9cb05d7

Fix minimal build vs ESS.

2d0106f

Add some CHANGES entries.

76f1a05

[extended tests]

fixup! crypto/cms: add CAdES-BES signed attributes validation for sig…

b08c846

…ning certificate V2 and signing certificate extensions.

FdaSilvaYY force-pushed the add-cades-verify branch from a8354d2 to b08c846 Compare May 23, 2020 16:53

t8m approved these changes May 25, 2020

View reviewed changes

slontis approved these changes May 25, 2020

View reviewed changes

slontis 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 May 25, 2020

mspncp 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 May 26, 2020

t8m closed this May 27, 2020

FdaSilvaYY deleted the add-cades-verify branch May 27, 2020 15:42

Uh oh!

Conversation

FdaSilvaYY commented Jan 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

levitte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

levitte commented Jan 28, 2019

Uh oh!

FdaSilvaYY commented Jan 28, 2019

Uh oh!

maxcuttins commented Feb 17, 2019

Uh oh!

FdaSilvaYY commented Feb 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FdaSilvaYY commented May 25, 2019

Uh oh!

slontis commented May 27, 2019

Uh oh!

Uh oh!

slontis commented May 27, 2019

Uh oh!

Uh oh!

FdaSilvaYY commented May 11, 2020

Uh oh!

opensignature commented May 17, 2020

Uh oh!

mspncp commented May 17, 2020

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FdaSilvaYY commented May 23, 2020

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

t8m commented May 25, 2020

Uh oh!

slontis left a comment

Choose a reason for hiding this comment

Uh oh!

mspncp commented May 26, 2020

Uh oh!

t8m commented May 27, 2020

Uh oh!

FdaSilvaYY commented May 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

FdaSilvaYY commented Jan 27, 2019 •

edited

Loading

FdaSilvaYY commented Feb 17, 2019 •

edited

Loading