trailofbits / SecureEnclaveCrypto

Demonstration library for using the Secure Enclave on iOS
Apache License 2.0
279 stars 41 forks source link

add swift example #4

Closed japesinator closed 7 years ago

japesinator commented 7 years ago

This moves some files around so that we can also show off the work @hfossli did (see: #3)

hfossli commented 7 years ago

Thanks :) I don't know how familiar all devs are with submodules in git. I would rather recommend embedding the repo directly or just pointing to the swift version in the readme.

dguido commented 7 years ago

+1 on copying the code over and not using submodules. I also noticed you removed a lot of the readme content. I'd like to have as much of that remain on the main readme as possible to showcase what you can do with the code.

hfossli commented 7 years ago

@dguido would you consider changing license to MIT?

dguido commented 7 years ago

Yes, we generally choose between either MIT or Apache for all of our projects. What's your concern with Apache?

hfossli commented 7 years ago

Cool. I'm not that familiar with Apache. It is a lot more text. MIT is 4 times more popular than Apache on github. The difference between MIT and Apache seems to be subtle. MIT is very permissive and it is widely known to developers how to relate to an MIT license where as with other licenses (like Apache) I feel like I would need to contact my lawyer to know what's okay and not.

Bottom line: Just for the sake of simplicity.

How do you generally decide how to choose license when choosing between Apache and MIT and why do you think Apache is a better fit for this project?

dguido commented 7 years ago

Apache protects you from patent lawsuits by asking contributors to assert they have those rights prior to making a commit. That's the only major difference IMHO. I'd rather stick with that if you don't mind.

hfossli commented 7 years ago

Sure. :) Just asking.

hfossli commented 7 years ago

Use my swift code anyway you like. If you want it embedded, then that's fine by me.

dguido commented 7 years ago

Awesome! I really appreciate you taking the time to go through it and do the port. We wanted to but we ran out of energy. Let's work on the details a little bit more, I want to make sure you get proper credit in the readme and we make a pointer back to your Github. Thanks!!

hfossli commented 7 years ago

Awesome. One question I feel needs clarification is: should this be some example code for how to use the API or should it be a framework/helper class?

hfossli commented 7 years ago

The way I structured the swift port:

The reason for dividing like this is just because I find it a little bit cleaner. In https://github.com/trailofbits/SecureEnclaveCrypto/issues/1 there's one that would like to specify other access objects. Keeping the access object on the outside puts the responsibility for ensuring correct keypair in the right place (there would simply be too much responsibility and complexity for the helper to assert correctness IMO).

japesinator commented 7 years ago

OK, I've re-done the Swift inclusion as separate files (then just adjusted the README and removed the redundant LICENSE). Right now there's a separate README in both subdirectories to show what you can do, but I can add that content to the main README if you'd like. As for example code, I don't fluently speak Swift, but I can definitely try to throw something together if we need.

hfossli commented 7 years ago

Looks nice. I've updated the repo with some changes – please use these.

I think the "key_builder.rb" and its notes in the readme could be moved to root level. In the updated repo I am printing to console just like in the objc version.

japesinator commented 7 years ago

Alright, I've moved key_builder.rb and the relevant README.md section to the root

hfossli commented 7 years ago

@dguido may I get read write access to this repo?

hfossli commented 7 years ago

Another take at this could be to completely replace the objective-c code with the swift code. Swift is the new standard for ios development. We could provide a link in the readme to the current revision for those who are interested in the objective-c version.

dguido commented 7 years ago

I'd like to keep the Objective-C and Swift versions, even though I realize that Swift is the new standard. Developers may need to interact with an old non-Swift library, and plenty of people out there still understand Objective-C better than Swift.

hfossli commented 7 years ago

Well, it's all here in older revisions for those objc lovers to find. With link in readme I think they should be good to go. :) Aaanyways, just an idea.

I figure I will be the one updating the swift code the most, approving changes and answering issues. Might as well add me as a contributor. What do you think?

hfossli commented 7 years ago

@japesinator as mentioned: I updated the swift repo after you embedded the code. :)

dguido commented 7 years ago

I figure I will be the one updating the swift code the most, approving changes and answering issues. Might as well add me as a contributor. What do you think?

Yes, definitely. Let's get this reviewed and merged, and I will get you on as a read/write contributor. Thanks for offering!

hfossli commented 7 years ago

I built upon @japesinator great work on this restructuring. See my fork of his branch here https://github.com/hfossli/SecureEnclaveCrypto/tree/swift-example

I'm all set now and think this looks good. Merge my branch to master at will.

hfossli commented 7 years ago

Here's the pull request https://github.com/trailofbits/SecureEnclaveCrypto/pull/5

hfossli commented 7 years ago

All set? :)

hfossli commented 7 years ago

http://i.giphy.com/qQh0DBncuFJwQ.gif