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

Use CryptoKit #214

Merged
merged 10 commits into from Sep 21, 2019
Merged

Conversation

ericlewis
Copy link
Contributor

Use the new Apple CryptoKit under the hood when running iOS/iPadOS 13, macOS 10.15, or watchOS 6.0.

@ericlewis
Copy link
Contributor Author

one of the tests failing here doesn't really seem like it should be, seems like wonky CI.

@mattrubin
Copy link
Owner

From the error code, it looks like one of Travis CI's typical inexplicable xcodebuild failures. I've restarted that CI job, which usually works for a flaky build.

Thanks for the PR! It's a busy week, but I'll try to review and merge it soon.

@ericlewis
Copy link
Contributor Author

ericlewis commented Sep 16, 2019

@mattrubin no problemo, I have a few more ambitious things to accomplish.

@ericlewis
Copy link
Contributor Author

I am interested in moving towards a more protocol oriented composition and pulling a few things out, like legacy support (for now) & the token persistence mechanisms. I think with a protocol approach, it will be easier for folks to roll their own persisters. Such as secured CoreData stores and the like.

@ericlewis
Copy link
Contributor Author

would also like to use your work on bases, modernize with a swift package, etc. so I might just use this as a reference.

@mattrubin
Copy link
Owner

While you're certainly welcome to use this project as a basis for your own implementation, most of the changes you mentioned (moving to Bases for base32 decoding, adding SPM support, pulling out the keychain integration and supporting alternative persistence stores) are on my roadmap for OneTimePassword. I can't promise any delivery dates for those features since I work on this project in my scarce free time, but PRs are always welcome!

I'm curious what your vision is for "protocol oriented composition". Just swappable persistence methods, or something more?

@codecov
Copy link

codecov bot commented Sep 18, 2019

Codecov Report

Merging #214 into develop will increase coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop     #214     +/-   ##
==========================================
+ Coverage    97.17%   97.38%   +0.2%     
==========================================
  Files            6        6             
  Lines          389      420     +31     
==========================================
+ Hits           378      409     +31     
  Misses          11       11
Impacted Files Coverage Δ
Sources/Crypto.swift 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 bf086fa...da31769. Read the comment docs.

@ericlewis
Copy link
Contributor Author

ericlewis commented Sep 19, 2019

This is my vision: https://github.com/ericlewis/THOTP

Heavily inspired, but the Generators & Passwords are protocol based. This means we can switch out the actual implementation. So we could create a concrete type (NSManagedObject for instance) that conforms to PasswordProtocol and now we can use CoreData to persist a password since all of the functional generation implementation lives as protocols / extensions. You could do similarly for a keychain persistence layer as well. The more composable it is, the more powerful :)

I intend to release another framework that is a persistent layer, but it’s actually achievable super simply: using valet and a clever extension, plus some serialization code, and I had persistence parity in around 20 lines of code or so. On top of that, I conform to Identifiable, to make it easier to persist (since it’ll have an ID) as well as display in SwiftUI applications.

My framework is not exactly the same as yours, I have no real desire for backwards compatibility currently. It wouldn’t really be too difficult to add it, tho. But for me it was easier to start from scratch, plus use all modern features directly.

@mattrubin mattrubin added this to the 3.3.0 milestone Sep 20, 2019
@mattrubin mattrubin merged commit 7af91b9 into mattrubin:develop Sep 21, 2019
@mattrubin
Copy link
Owner

@ericlewis Thanks for sharing this! It's interesting to see a different approach to the same functionality. I think the struct-based approach OneTimePassword takes is a better fit for my use case, but I may look more closely at using the protocol-oriented approach to make persistence extensible.

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

Successfully merging this pull request may close these issues.

None yet

2 participants