Develatio / django-walletpass

Django .pkpass build, sign, push for updates, serve and more...
Other
23 stars 12 forks source link

Awesomeness? #1

Open alexandernst opened 5 years ago

alexandernst commented 5 years ago

How can this library be so fucking awesome?! Who made it?!

NachE commented 5 years ago

Hey!, we know each other?

alexandernst commented 5 years ago

¯\(ツ)/¯ how could I know such a magnificent person that is able to create a Django package that is able to sign Apple related stuff with PKCS7 ?!

captain-fox commented 4 years ago

Since this is (sort-of) open conversation issue I'd like to give my complements to creators/maintainers, but also I've dug through source code of this library and I have couple of questions:

  1. Is it possible and have you tried to implement pass signing using .pem without providing password using same libraries (or about)? This is somewhat painfull subject for me because for quite a long time I've been trying to achieve this without a need for M2Crypto (which is VERY nasty library and it is a pain in the ass to work with).
  2. This may be the case only for me, but I find storing keys and certs locally on the server is somewhat unnecessary. I'm signing several different pass types with several different certificates, so storing this data in database is more handy. This way you can pull related certs and keys from DB rather then having only one sert of credentials.
alexandernst commented 4 years ago

Hi! All complements to @NachE !

  1. Yes, this should be possible. Check https://github.com/Develatio/django-walletpass/blob/b54f5a5f4f95a092ab3a4d395b40c83a0c9872a9/django_walletpass/crypto.py#L21

  2. I don't think I'm understading your question. You're free to store your certs wherever you like. When using the lib, just pull them from where you stored them and pass them as IO objects. You don't need to store them on the FS if you don't want to.

NachE commented 4 years ago

Hi @captain-fox.

As @alexandernst point out, the signing logic of django-walletpass has been encapsulated in https://github.com/Develatio/django-walletpass/blob/master/django_walletpass/crypto.py and can be used to sign wathever you want (and password=None is allowed). The limitation here is that this code expects a separeted key and cert, so, if you want to sign with .pem files, you have to parse it and separate both parts.

The problem of using certs from DB comes from hyper library that needs a file to handle tls:

https://github.com/python-hyper/hyper/blob/master/hyper/tls.py#L71

and django-walletpass depends on apn2 wich depends on hyper to send push notifications:

https://github.com/Develatio/django-walletpass/blob/master/django_walletpass/services.py#L38

captain-fox commented 4 years ago

@NachE it's great that we're on the same page here!

I'm using very simple solution for hyper by creating tempfile directory server-side and instantiating FileSystemStorage for certs inside it:

from django.core.files.storage import FileSystemStorage
...
temp_path = tempfile.mkdtemp(dir='media')
storage = FileSystemStorage(location=temp_path)
...
storage.save(name='cert.pem', content=cert_stored_as_model_field.open())
...

(Obviously this is ovesimplified implementation)

So this method would be more convenient accept certs, key, data and password as agruments rather than reference them from within itself.

NachE commented 4 years ago

Implementing a KeyStore sounds good to me. A convenient approach could be using a "configurable backend strategy" for getting certs. Also, keeping a default backend SingleFileKeyStore reusing current configuration schema in order to avoid breaking backward compatibility, will be an appropriate solution to be merged in a minor version.

captain-fox commented 4 years ago

@NachE I also think KeyStore would be a great improvement. Have you considered making all models abstract?

...
class Meta:
        abstract = True

This way user of the library would be able to have his/her own models that may contain other fields unrelated to wallet directly and still use all the great features that are already implemented in this package.

NachE commented 4 years ago

Making all models abstract will break the purpose of this package (a working solution).

If you need to implement your own fields to models, you can do it extending models:

example:

from django_walletpass import models as walletpass_models

class AbstractCustomPass(walletpass_models.Pass):
   class Meta(walletpass_models.Pass.Meta):
       abstract = True

class AbstractCustomRegistration(walletpass_models.Registration):
    class Meta():
        abstract = True

class CustomPass(AbstractCustomPass):
    # my own fields here

class CustomRegistration(AbstractCustomRegistration):
    # my own fields here
    pazz = models.ForeignKey(
        CustomPass,
        on_delete=models.CASCADE,
        related_name='registrations',
    )

Of course if you want to use REST feature yo also need to extends class from classviews.py and point to new models.

Anyway, pull requests are allowed if yo want to send it and your proposed code don't break backward compatibility.

captain-fox commented 4 years ago

For some weird reason I'm getting [SSL: CA_MD_TOO_WEAK] ca md too weak (_ssl.c:3880) when trying to send push notification to pass. I understand that same cert and key that are used to sign passes are later used to access APNS? Is there something extra that needs to be done with certs to be able to use APNS?

NachE commented 4 years ago

Same certs for sign are used to access APNs, so no extra work is needed. This error appears to be related to security checks performed by openssl lib (https://github.com/openssl/openssl/blob/master/ssl/t1_lib.c#L2635).

Maybe your certs are enough strong for openssl 1.0.x but too weak for openssl 1.1.x. Review your openssl version and/or regenerate your certs using openssl 1.1.x.

captain-fox commented 4 years ago

This is my openssl config:

~ > openssl version -a
OpenSSL 1.1.1d  10 Sep 2019
built on: Sat Sep 28 13:18:07 2019 UTC
platform: darwin64-x86_64-cc
options:  bn(64,64) rc4(16x,int) des(int) idea(int) blowfish(ptr) 
compiler: clang -fPIC -arch x86_64 -O3 -Wall -DL_ENDIAN -DOPENSSL_PIC -DOPENSSL_CPUID_OBJ -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DKECCAK1600_ASM -DRC4_ASM -DMD5_ASM -DVPAES_ASM -DGHASH_ASM -DECP_NISTZ256_ASM -DX25519_ASM -DPOLY1305_ASM -D_REENTRANT -DNDEBUG
OPENSSLDIR: "/usr/local/etc/openssl@1.1"
ENGINESDIR: "/usr/local/Cellar/openssl@1.1/1.1.1d/lib/engines-1.1"
Seeding source: os-specific

And I'm running those two commands to generate .pem files:

  1. openssl pkcs12 -in "key.p12" -clcerts -nokeys -out certificate.pem
  2. openssl pkcs12 -in "key.p12" -nocerts -out key.pem

With this done I'm still getting [SSL: CA_MD_TOO_WEAK] error.

alexandernst commented 4 years ago

Can you try adding -macalg SHA1 to both commands?

captain-fox commented 4 years ago

I've re-generated certificate from ground up and ran:

  1. openssl pkcs12 -in "Certificates.p12" -clcerts -macalg SHA1 -nokeys -out certificate.pem
  2. openssl pkcs12 -in "Certificates.p12" -macalg SHA1 -nocerts -out key.pem

Result is the same: [SSL: CA_MD_TOO_WEAK] ca md too weak (_ssl.c:3880)

alexandernst commented 4 years ago

Can you check your openssl*.cnf file , under the [CA_default] section there should be a default_md option. Can you check if it's value is sha256?

captain-fox commented 4 years ago

@alexandernst It just says default

alexandernst commented 4 years ago

Can you change it to sha256?

captain-fox commented 4 years ago

Changed it, re-generated keys – problem remains

alexandernst commented 4 years ago

What distro is this? Can you try on another machine with Debian 9 or 10?

captain-fox commented 4 years ago

I'm running this in Docker using FROM python:3.7.6 image

JamesKey-iq commented 4 years ago

For some weird reason I'm getting [SSL: CA_MD_TOO_WEAK] ca md too weak (_ssl.c:3880) when trying to send push notification to pass. I understand that same cert and key that are used to sign passes are later used to access APNS? Is there something extra that needs to be done with certs to be able to use APNS?

did you manage to fix this?

alexandernst commented 4 years ago

We found a way to repro this, stay tuned for a possible fix.

captain-fox commented 4 years ago

For some weird reason I'm getting [SSL: CA_MD_TOO_WEAK] ca md too weak (_ssl.c:3880) when trying to send push notification to pass. I understand that same cert and key that are used to sign passes are later used to access APNS? Is there something extra that needs to be done with certs to be able to use APNS?

did you manage to fix this?

@alexandernst

Several people faced the same issue in a bunch of apns-related projects such as django-push-notifications issue 532

But all narrows down to solution that has to be either explaned or fixed in scope of pyapns2 implementation.

I'd suggest we continue investigation under an issue in APNS repo: https://github.com/Pr0Ger/PyAPNs2/issues/103#issue-558172230

NachE commented 4 years ago

The recently uploaded 1.2 version of django-walletpass implements JWT auth for push notifications. You could use this new feature to bypass this problem.

captain-fox commented 4 years ago

Te recently uploaded 1.2 version of django-walletpass implements JWT auth for push notifications. You could use this new feature to bypass this problem.

As I understand xyz.p8 certificate is yet another cert that needs to be generated, this time specifically for APNS, to allow JWT auth?

NachE commented 4 years ago

As I understand xyz.p8 certificate is yet another cert that needs to be generated, this time specifically for APNS, to allow JWT auth?

You are right. The CA_MD_TOO_WEAK comes from the new openssl version wich indroduces a security cert check (https://github.com/Develatio/django-walletpass/issues/1#issuecomment-590535718). Implementing JWT auth for push notifications is our best aproach to keep this app working.

captain-fox commented 4 years ago

@NachE

Isn't JWT auth meant to serve as APNS communication for an actual app rather than means of sending pass updates? Apple explicitly says that PassTypeID certificate serves to sign and update passes:

Screenshot 2020-03-26 at 10 24 05

To create APNS cert, one also needs to create App ID, and this is comparable to hammering a nail with a wrench. Unless I was confused about type of file one needs to generate and it's actually not a certificate, but a key:

Screenshot 2020-03-26 at 10 57 14

I do not know how OpenSSL works, apart from the fact that there are always problems with it, but is there a way to comply with this new security checks to do things the right way, rather than using JWT as a workaround?

If I knew how to address this issue myself, I'd gladly do it and share the solution with everyone, but SSL is not something you can learn and understand in a matter of minutes, so I can only help test and try out proposed solutions.

NachE commented 4 years ago

The problem with the certs probably comes from the base cert that has been used to generate key and cert. If you regenerate your base cert (with more strong settings) and regenerate key/cert with it, this problem should be fixed.

You can confirm this reducing security check level in /etc/ssl/openssl.cnf

from:

CipherString = DEFAULT@SECLEVEL=2

to:

CipherString = DEFAULT@SECLEVEL=1

The introduction of JWT for push is a configurable option and you can still use legacy (default) mode. For push notifications (JWT) you need to currently generate a .p8 until we get more time to merge the code and use just one for both.

captain-fox commented 4 years ago

@NachE That is perfectly understandable. The question that arises is what is the meaning of "stronger settings"? I'm following the same steps as everyone else:

  1. Create a certificate beginning with a request to certifying authority
  2. Uploade request file to apple developer
  3. Import obtained certificate into KeyChain
  4. Export certificate as .p12 file
  5. Convert p12 file into key and cert files with .pem extension

There's only one step in entire process that allows some configuration related to stregth of certificate and it is available during generation of request to certifying authority:

Screenshot 2020-03-26 at 12 59 39 Screenshot 2020-03-26 at 12 52 51

It this the step which can potentially make the main issue go away?

NachE commented 4 years ago

You can get more information about cert sec limitation introduced in openssl 1.1.1 with debian installation here: https://wiki.debian.org/ContinuousIntegration/TriagingTips/openssl-1.1.1

wich bring some clarifications:

This can be disabled with CipherString = DEFAULT@SECLEVEL=1 into /etc/ssl/openssl.cnf as described abobe.

Looking at the code that sends push notifications (https://github.com/Develatio/django-walletpass/blob/master/django_walletpass/services.py), the certs used to do this work are your two cert and key files. If your two files are strong enough, and this issue persist, we need to dig more deeply into this issue.

Besides, apple has published an update note: https://developer.apple.com/news/?id=11042019a

If you send push notifications with the legacy binary protocol, we recommend updating to the HTTP/2-based APNs provider API as soon as possible. You’ll be able to take advantage of great modern features, such as authentication with a JSON Web Token, improved error messaging, and per-notification feedback. The Apple Push Notification service (APNs) will no longer support the legacy binary protocol as of November 2020.

As you can see, binary protocol will stop working on November 2020, so implementation of JWT seems to be the right path to follow in order to keep a good health of django-walletpass

captain-fox commented 4 years ago

@NachE many thanks for this update, the explanations in documentation for openssl 1.1.1 makes everything clear and resolced; I wasn't aware that apple has made statement about JWT – then JWT it is from now on.

captain-fox commented 4 years ago

Hi, guys! @NachE @alexandernst I've created this over the weekend: https://github.com/captain-fox/airpress Maybe something you'd be willing to use for this project as a library for pkpass compression? The description isn't very long (yet), so please checkout the source from repo to see all the features I've added.

I'd be glad to hear your feedback.

P.S. I've copied implementation of crypto from your project as there's really not many ways to implement pkcs7 signature and you've had it nice and clean.

Cheers!