Skip to content
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

Architecture for external dependencies #251

Open
mitchins opened this issue Jan 11, 2021 · 9 comments
Open

Architecture for external dependencies #251

mitchins opened this issue Jan 11, 2021 · 9 comments

Comments

@mitchins
Copy link

Apologies for the lengthiness, there's a fair bit to compress:

I am working for a digital bank and we use this library, the only caveat is that AES256GCM isn't supported.
We've had our upstream vendor give us a hand and it looks like we would be able to expand this library to supported it...

Now ideally I'd like to contribute this back upstream and not keep it in a silo.
Here comes the gotcha: CryptoKit is only supported on iOS >= 13.0, and many companies/people still support 12 or even 11 due to the number of devices still out there such as iPhone 6 etc.

Long story short, this functionality - even if optional would require https://github.com/krzyzanowskim/CryptoSwift.
Now the tricky bit:

  • Currently there are no external dependencies for the library building itself.
  • JOSESwift is supporting both Carthage and CocoaPods. (So does CryptoSwift)

Here's my thinking....
We don't want to force anything upon a user but if they want AES256GCM we'd need to add this library, the library would be needed.

I know for a fact cocoa pods wouldn't require the project files as it constructs a new one based on the pod spec.
I am less familiar with Carthage but believe that it will use the existing xcproject file, and so the current project.

Here are the two viable options as far as I can see it:

  1. As a direct dependency:
    CryptoSwift module is added as a dependency for this JOSESwift via pod and Carthage respectively. Now if someone was to open up the project itself and build it from Xcode it would need a (1) pod or (2) cartfile to install the module, or swift package manager. My only holdout here is that the project needs to utilise one of the 3 package managers if someone directly opens the project (pod of course won't care since it creates a new one from scratch every time based on the spec).

  2. As an injectable dependency
    This would be more work for the client perhaps, but would alleviate changing dependencies.
    I imagine this JoseSwift exposes a protocol for what it needs to be like GenericEncrypterStrategy

In order to actually use AES256GCM, we'd need to fulfil the dependency:
There is both JWEHeader and Encrypter, which we'd either:
a) Inject the encryption provider per instance (in the constructor most likely)
b) Inject it into the module via some static property (JoseSwift.provider = Foo.Bar or something)

While one needs to be careful with static stuff, at a module/library level is possibly ok (option b). The first option (a) would lead to longer initialisation etc.

It would also be possible to implement a default provider of CryptoKit allowing iOS >= 13.0 to not do any work.
Do not have this as of yet, but it would merely conform to the expected protocol for a GenericEncrypterStrategy .

Eager to hear the thoughts of people who maintain this @daniel-mohemian et. al.

@ghost
Copy link

ghost commented Jan 12, 2021

Hey @mitchins!

Thanks for your well thought out suggestion. We really appreciate that you are willing to put work into contributing your changes back upstream for everyone to use.

When we open sourced JOSESwift we decided that we would only trust cryptographic libraries that come straight from Apple (Security.framework, CommonCrypto, CryptoKit). That's why we haven't added any features that would require us to include third party cryptographic implementations like CryptoSwift. We'd like to keep it this way. Therefore, adding CryptoSwift as a direct dependency to JOSESwift is not an option for us at this time.

Option 2, as suggested by you, would be our preferred way to tackle this. Since the beginning, we have considered to support injectable, replaceable cryptographic implementations to work around the fact that Apple doesn't support all algorithms that the RFCs define. Unfortunately, we never got around to implement it but it should definitely be doable.

I'm thinking of something like an optional parameter in the Encrypter's/Decrypter's encrypt/decrypt functions. We already have ContentEncryper/ContentDecrypter protocols for which we instantiate concrete implementations based on the specified algorithm in the above mentioned encrypt/decrypt functions (e.g. here). Assuming we allow the user to specify their own implementation of this protocol we could use that implementation instead of instantiating the default one.

A good way of solving this would probably be to first add native A*GCM support using CryptoKit for iOS13+ and then in a second pull request allow the user to optionally overwrite this default implementation by specifying their own content encryption implementations.

Let me know what you think!

@ghost ghost self-assigned this Jan 12, 2021
@fdstevex
Copy link

fdstevex commented Aug 3, 2021

Now that we have https://github.com/apple/swift-crypto would that be acceptable as a library coming straight from Apple?

@ghost
Copy link

ghost commented Aug 4, 2021

Good point. It's an Apple maintained project so yeah I think we could probably get behind that.

@tobihagemann
Copy link
Contributor

I would love to see support for A256GCM since I need that for one of our projects. Thanks to @mitchins, I was able to get a prototype working and it seems to be fairly easy with CryptoKit. However, this would require this project to bump its minimum targets (iOS from 10 to 13 and watchOS from 4 to 6). Would that be acceptable?

@mitchins
Copy link
Author

mitchins commented Aug 11, 2022 via email

@tobihagemann
Copy link
Contributor

That would be great! But sounds like using private API of CommonCrypto, is that right? https://stackoverflow.com/a/36634956

@mitchins
Copy link
Author

@tobihagemann we used the CryptoSwift library.
Actually I can't take the credit it was done by our partners in order to use their APIs.

This was done back on 2.3.1

https://github.com/mitchins/JOSESwift/tree/release/2.3.1

Differences between 2.3.1 and changes we made:
https://github.com/mitchins/JOSESwift/pull/1/files

It might be problematic to merge back.

We are expecting to ditch iOS 12.x support soon and focus on 13+, so that means the CryptoKit would be fine...
For anyone else requiring iOS 12 support, the above fork of 2.3.1 should work fine.

@tobihagemann
Copy link
Contributor

Ah, I understand. I wouldn't consider CryptoSwift "standard" though (as have been mentioned here) but I appreciate you sharing your code. It will help me a lot and I'll try to open a new PR that uses CryptoKit. I leave it to JOSESwift's maintainers if they're willing to increase the minimum targets.

@thiagoalves
Copy link

@mitchins I see you implemented A256GCM in your fork. I need A128GCM though, and I am no crypto expert.

Is it just a matter of performing a few changes on your code, or do I need to completely rewrite your code?

Any help will be much appreciated.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants