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

openssl-machine · 2020-03-03T09:28:13Z

Automated Ping: This issue is in a state where it requires action by @openssl/committers but the last update was 54 days ago

FdaSilvaYY · 2020-03-03T11:08:31Z

Automated Ping: This issue is in a state where it requires action by @openssl/committers but the last update was 54 days ago

I just rebase my branch, last weekend to integrate CHANGES and cms tests evolutions.
No code change.
I can squash this, if needed.
CI failures look the same as on some master builds.
I did not dig them deeper.

_{Sent with GitHawk}

levitte

Nits

CHANGES.md

FdaSilvaYY · 2020-03-04T07:14:04Z

Nits in CHANGES fixed.
I see the same CI errors as in last master build

maxcuttins · 2020-03-10T16:30:48Z

it smell like it's almost ready! 👍

openssl-machine · 2020-04-10T00:08:12Z

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

bernd-edlinger · 2020-04-10T00:27:20Z

Oh, I feel so sorry @FdaSilvaYY If I only knew how to help...

FdaSilvaYY · 2020-04-10T21:21:35Z

@bernd-edlinger : Thanks for your support. I'm used to the OpenSSL merge delays.
This PR is mostly code moving, and additional tests, I don't see what I can do more too.

By the way, my last force-push was merging your changes from #10755
I'm not rebasing and pushing this so often ; only and just on real conflict, to avoid wasting CI build time.

bernd-edlinger · 2020-04-11T01:23:48Z

Nice, that one merged that to 1.1.1f (and it broke something)
but it was worth it @t8m, Tomáŝ (did I write your name correct?)

bernd-edlinger · 2020-04-11T01:26:53Z

@FdaSilvaYY one thing I never understood, given how friendly you are all the time,
and how you give support to others here, have you ever been asked to become a
member of @openssl/committers ?

t8m · 2020-04-14T09:41:11Z

Nice, that one merged that to 1.1.1f (and it broke something)
but it was worth it @t8m, Tomáŝ (did I write your name correct?)

offtopic: The correct spelling is Tomáš. However using just Tomas is fine.

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.

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 from 60044db to 1d6891e Compare February 28, 2020 23:20

levitte requested changes Mar 3, 2020

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

CHANGES.md Outdated Show resolved Hide resolved

t8m requested changes May 19, 2020

View reviewed changes

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.

t8m approved these changes May 25, 2020

View reviewed changes

slontis approved these changes May 25, 2020

View reviewed changes

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!

openssl-machine commented Mar 3, 2020

Uh oh!

FdaSilvaYY commented Mar 3, 2020

Uh oh!

levitte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

FdaSilvaYY commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maxcuttins commented Mar 10, 2020

Uh oh!

openssl-machine commented Apr 10, 2020

Uh oh!

bernd-edlinger commented Apr 10, 2020

Uh oh!

FdaSilvaYY commented Apr 10, 2020

Uh oh!

bernd-edlinger commented Apr 11, 2020

Uh oh!

bernd-edlinger commented Apr 11, 2020

Uh oh!

t8m commented Apr 14, 2020

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

FdaSilvaYY commented Jan 27, 2019 •

edited

Loading

FdaSilvaYY commented Feb 17, 2019 •

edited

Loading

FdaSilvaYY commented Mar 4, 2020 •

edited

Loading