asn1crypto icon indicating copy to clipboard operation
asn1crypto copied to clipboard

Feat/0039 rfc5126: support for CADES-BES/EPES with long term support

Open erny opened this issue 7 years ago • 22 comments

Basic tests are included, at least for signature parsing. Still no tests for building new structures.

erny avatar Apr 05 '18 05:04 erny

The Circleci job failed, but it seems unrelated to the commits.

erny avatar Apr 05 '18 05:04 erny

Do we need still python 2.6 compat? I can remove dict comprehension of the tests.

erny avatar Apr 05 '18 09:04 erny

Yes, Python 2.6 compatibility is important, because this library is used in Package Control, which still supports Sublime Text 2, which runs on Python 2.6. https://travis-ci.org/wbond/asn1crypto/jobs/362466545

Also, all strings are unicode by default and should not use the u prefix since it doesn't work with Python 3.2: https://travis-ci.org/wbond/asn1crypto/jobs/362466547.

The L suffix on integers is not supported in Python 3.3: https://travis-ci.org/wbond/asn1crypto/jobs/362466548.

If you can rebase on top of master, that will help get CircleCI running properly against the latest PyPi SSL changes. However, while my setup worked in a branch, it seems to be funky right now, so maybe hold off on the rebase until master is green.

wbond avatar Apr 05 '18 11:04 wbond

Ok, thanks a lot. I'll revisit my changes to assure I'm not using Python 2.7 syntax, and get rid of any "unicode" string constants.

Regards.

Ernesto Revilla Área Técnica TangramBPM.es Tlf: 630 244 136

2018-04-05 13:44 GMT+02:00 Will Bond [email protected]:

Yes, Python 2.6 compatibility is important, because this library is used in Package Control, which still supports Sublime Text 2, which runs on Python 2.6. https://travis-ci.org/wbond/asn1crypto/jobs/362466545

Also, all strings are unicode by default and should not use the u prefix since it doesn't work with Python 3.2: https://travis-ci.org/wbond/ asn1crypto/jobs/362466547.

The L suffix on integers is not supported in Python 3.3: https://travis-ci.org/wbond/asn1crypto/jobs/362466548.

If you can rebase on top of master, that will help get CircleCI running properly against the latest PyPi SSL changes. However, while my setup worked in a branch, it seems to be funky right now, so maybe hold off on the rebase until master is green.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/wbond/asn1crypto/pull/88#issuecomment-378908447, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEtcB_zQhhPlAoamaAbXpkxbGxPVaQpks5tlgOSgaJpZM4TH4k6 .

erny avatar Apr 05 '18 11:04 erny

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@291cd95). Click here to learn what that means. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #88   +/-   ##
=========================================
  Coverage          ?   85.48%           
=========================================
  Files             ?       26           
  Lines             ?     5541           
  Branches          ?        0           
=========================================
  Hits              ?     4737           
  Misses            ?      804           
  Partials          ?        0
Impacted Files Coverage Δ
asn1crypto/cades.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 291cd95...7f9cc31. Read the comment docs.

codecov[bot] avatar Apr 05 '18 12:04 codecov[bot]

Gah, the tests are breaking because PyPi is doing brownouts of TLS 1.0 and 1.1 support, but the stable pip is broken on Mac with Python 2.6. I have a PR in to try and fix the underlying issue, but it may take some time before that is worked out.

wbond avatar Apr 05 '18 18:04 wbond

Ok, I corrected the tests. Now it should work with python 2.6 and 3.x.

2018-04-05 20:05 GMT+02:00 Will Bond [email protected]:

Gah, the tests are breaking because PyPi is doing brownouts of TLS 1.0 and 1.1 support, but the stable pip is broken on Mac with Python 2.6. I have a PR in to try and fix the underlying issue, but it may take some time before that is worked out.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/wbond/asn1crypto/pull/88#issuecomment-379025856, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEtcOm0GqIRJii8g4e2NnLUfa11_PJqks5tllz8gaJpZM4TH4k6 .

erny avatar Apr 05 '18 21:04 erny

Ok. travis finally passed (after some minor commits).

erny avatar Apr 05 '18 21:04 erny

Any news on this?

erny avatar May 11 '18 17:05 erny

Hi, since 2019 Italy needs cades xml files for invoices with public administrations entities, this would be very useful, I've tested it and I'm able to parse .xml.p7m files as needed, this pr can be rebased to master without problems, and test are ok

sherpya avatar Jan 24 '19 23:01 sherpya

Hi, since 2019 Italy needs cades xml files for invoices with public administrations entities, this would be very useful, I've tested it and I'm able to parse .xml.p7m files as needed, this pr can be rebased to master without problems, and test are ok

Hey, thanks! Cool that you could do that.

I wrote the CAdES long term support initially to be able to parse EPES, some new signature attributes, and all the long term attributes that I receive from spanish signature validation / upgrade infrastructure.

The CAdES and all the ETSI other signature formats (PAdES, XAdES) are standards mostly used the European Union (due to EU directives). But asn1cryto was designed initially for use in Sublime Text editor and it may be a bit overkill to include the CAdES support in the core. It would be nice to have a plugin support, so you could do something like "pip install asn1crypto[cades]", a upstream compatible fork (eg. asn1crypto-cades?) or something similar.

I hope that Will Bond could give us any ideas on how to proceed. Best regards.

erny avatar Jan 25 '19 10:01 erny

In my opinion, there's no reason not to include CAdES etc. in asn1crypto. But I'm not familiar with those standards (although I live in the EU) and couldn't find the time to take a close look at this PR.

35 commits is a lot (even though only 10 changed filed). And to review this, one would also need to read+understand the RFC(s). There might be some important subtles.

I don't see the need for a plugin system. CAdES (and other public asn.1 types) should be in the core. And it's always possible to create another python package that requires asn1crypto and only defines the new asn.1 types. I do exactly that at my day job where I invented some proprietary asn.1 types.

joernheissler avatar Jan 25 '19 10:01 joernheissler

35 commits is a lot (even though only 10 changed filed). And to review this, one would also need to read+understand the RFC(s). There might be some important subtles.

I think it can be squashed in a better way

sherpya avatar Jan 25 '19 15:01 sherpya

... I don't see the need for a plugin system. CAdES (and other public asn.1 types) should be in the core. And it's always possible to create another python package that requires asn1crypto and only defines the new asn.1 types. I do exactly that at my day job where I invented some proprietary asn.1 types.

Could you provide a concrete example? How could be proceed? I would be willing to move all this stuff into another package. Thx.

erny avatar Jan 25 '19 15:01 erny

... 35 commits is a lot (even though only 10 changed filed). And to review this, one would also need to read+understand the RFC(s). There might be some important subtles. ...

As could be seen in the "files" section, only one file is changed (tests/init.py) to allow running new tests, only one core file is added (cades.py). All other files are for testing or documentation. IMHO, on the other hand, any tests should be done on real world examples, as done here, because the spec is really complex. Perhaps, this should not be seen as 100% error free (sure it isn't), but if it provides value for those people who need CAdES support, why not release it and include a "beta quality cades long term signature support" comment?

Regards

erny avatar Jan 25 '19 15:01 erny

Here are the reasons I haven't reviewed/merged yet:

  1. asn1crypto deals with cryptography and has my name plastered all over it, hence I don't want to just throw stuff in. Granted, it has had plenty of bugs and will continue to for a while, but I am more apprehensive with this project than something like pybars3.
  2. This is a sizable change. I'm not familiar with CADES, and this is big first changes for someone who hasn't made a lot of changes to asn1crypto.
  3. I had my fifth child earlier this year, and barely have time for open source these days. I have so many projects that need attention that I mostly spot a hour or two here to there to prevent things from blowing up or disintegrating.

When I started this project I was self-employed and focused on something in the security space, but now I work on Sublime Text, so this is sort of a side-project. @joernheissler has shown a good handle on ASN.1 in general and making a bunch of contributions so I've added him as a collaborator and he's been the one keeping things from becoming frozen in time. I'd love to have more people involved, but I'm the sort of person who likes contributors to ease into a project one small bit at a time and then I give them access to contribute. Afterward I typically check in on changes to make sure things look reasonable. This is probably means my projects won't ever reach their full potential, but I'd rather not get into a situation like event-stream in npm.

In summary, I apologize that this hasn't made it in. I wish I had the time to go through and review it and get it merged in. I'm sure lots of people would find it beneficial. Perhaps someday I'll find a way to take useful projects like these and turn them into an reliable income source providing enhancements and support to companies using them, but for now they are just "best effort" as it fits into the rest of my life.

wbond avatar Jan 25 '19 16:01 wbond

Thanks Will for your response. Everything sounds very reasonable. Congrats for your new kid. Your suggestions to make the cades support external sounds also reasonable to me. I'm just thinking about the best way to do this. Jörn pointed me already into this direction.

Today I did a new merge from master, and now all test fail (still don't know why). Best wishes for 2019. Regards.

erny avatar Jan 25 '19 20:01 erny

Congrats, @wbond! And btw, thanks for adding me as collaborator. I wish I'd find more time, too. E.g. I still didn't complete #86, maybe I should split it up a bit and describe the trouble I'm facing.

@erny, I think that cades support SHOULD NOT be made external. It should be in the asn1crypto package. I merely said that it's technically possible.

I'll try to review your PR in a few days.

joernheissler avatar Jan 25 '19 21:01 joernheissler

all test fail

https://github.com/wbond/badtls.io/issues/6

joernheissler avatar Jan 25 '19 21:01 joernheissler

$ openssl s_client -connect dh1024.badtls.io:10005 -servername dh1024.badtls.io -showcerts </dev/null 2>/dev/null | openssl x509 -noout -enddate 

notAfter=Jan  1 00:00:00 2019 GMT 

erny avatar Jan 26 '19 00:01 erny

I just rebased onto master to hopefully get all of the CI going through cleanly.

I like all of the tests that there are. I think we can probable de-dupe some of the structures with x509 and as long as there aren't tagging conflicts use things like x509.DisplayText instead of cades.DisplayText.

I'd also rather reference RFC URLs than copy ANS.1 definitions wholesale into the source code, for consistency with other parts of the library.

I'm hoping sometime in the next month or so to go through this with a fine tooth comb and clean it up a bit for consistency. It doesn't seem like there are all that many additions in cades.py, and there are a bunch of tests which is helpful.

Thanks for all of your work on this @erny.

wbond avatar Aug 02 '19 15:08 wbond

Thanks a lot for your great work!

I hadn't time ultimately to work on this.

It may be a bit out of scope as this RFC is quite complex and long-term signature verification and storage may be not needed everywhere.

I wish I had more time to work on this.

Thanks again. Best regards.

El 2 ago. 2019 5:07 p. m., "Will Bond" [email protected] escribió:

I just rebased onto master to hopefully get all of the CI going through cleanly.

I like all of the tests that there are. I think we can probable de-dupe some of the structures with x509 and as long as there aren't tagging conflicts use things like x509.DisplayText instead of cades.DisplayText.

I'd also rather reference RFC URLs than copy ANS.1 definitions wholesale into the source code, for consistency with other parts of the library.

I'm hoping sometime in the next month or so to go through this with a fine tooth comb and clean it up a bit for consistency. It doesn't seem like there are all that many additions in cades.py, and there are a bunch of tests which is helpful.

Thanks for all of your work on this @erny https://github.com/erny.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wbond/asn1crypto/pull/88?email_source=notifications&email_token=AAAS24CZFO2EK23P4LKOGWTQCRERLA5CNFSM4EY7RE5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3OAMHQ#issuecomment-517735966, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAS24D2QOXQGXVY2NA3RFLQCRERLANCNFSM4EY7RE5A .

erny avatar Aug 02 '19 15:08 erny