Standard PK (weak) decryption support, with basis for future AES decryption support.

[danielgindi]

I may work on this a little more soon, to add AES.
I do not really need encryption right now, only decryption. So I'm not sure If I'll implement.

[pixelglow]

Thanks for your contribution, it looks like you've put a lot of effort into it, especially the decryption guts, and it generally looks good. Some comments:

General Architecture

• Try to stay away from smart C++ classes e.g. virtual member functions and the like. I eventually want to migrate to a no C++ runtime (libc++) solution. I don't mind C++ that doesn't link to libc++ but I'll have to test to see what is kosher. For example the ZZDecrypter hierarchy should be recast in Objective-C, possibly as a class cluster.
• The NSStream implementations should be single-responsibility and composable. For example, the ZZInflateInputStream changes conflate both inflating and decrypting, which then limits its flexibility outside of the immediate context, and possibly outside of libzipzap. You may still need to change ZZInflateInputStream to take another stream instead of a raw NSData but that will allow composing with a pure ZZDecryptInputStream for example.
• Don't throw exceptions: Apple guidance is moving away from exceptions in favour of explicit NSError -- e.g. in ARC, thrown exceptions may end up with dangling objects. I still have try-finally blocks but no catch blocks.
• We'll need some unit/system tests for encrypted files, at the least with the standard/weak encryption we'll be supporting.

Some Detail

• Is the ZZConstants.h header meant to be public or private? If it is private, merge it with ZZHeaders.h.
• ZZArchive.h:59 should be a constructor parameter, not a visible/settable property. If you feel it needs to be a property, keep it readonly like the other ZZArchive properties.
• ZZArchive.h:64 isn't necessary, we want a minimal interface to ZZArchive.
• ZZDecryptInputStream.h:14 should not need a dataStartOffset… just pass use dataWithBytesNoCopy:length:freeWhenDone to pass in a slice of data instead. This avoids any anxiety reading the code and wondering if it will touch the initial bytes on purpose or by mistake.
• ZZHeaders.h:107-121,187-201 should be bool-valued functions.
• ZZHeaders.h:137 has a magic constant, make it a sign static member.
• ZZHeaders.h: nice implementation of ZZExtraField and ZZAesExtraDataRecord, in particular I like that you've moved some generalPurposeBitFlag manipulation logic into the header here where it really belongs. However ZZExtraField should have a nextExtraField member function in the spirit of ZZLocalFileHeader::nextLocalFileHeader etc. Then the central file header and local files should return a ZZExtraField* from the extraField member function. The scanning algorithm in 126-143 and 214-230 could either be reshaped to a C++ generic/template function (templates functions don't require libc++ !) for future expansion to other structs, or perhaps moved to ZZOldArchiveEntry.

Style

• Casts should be without whitespace e.g. (ZZArchiveEntry*) instead of (ZZArchiveEntry *).
• Blocks with single lines should just omit the surrounding braces. (I'm probably the lone coder out there who thinks so :-) .)

We'll take another pass or two on the code before I'll merge it in.

[danielgindi]

OK! my responses in the same order :-)
I actually did not know if you are active on the project and will accept contributions. Nice to know that you do! Now I know that I also have a go at modifying your architecture and interface as needed - so from now on it will be easier!

General Architecture

• Regarding the C++11 linkage- I actually started to move it to plain C++, but when I found out how to compile it I reverted to your original code... I'll make some effort to convert it again. Anyway I do not think we should move to plain ObjC, especially the ZZDecrypter class, as we want the maximum performance available for the decryption. But we should remove the enum classes. They gave me hell trying to compile in another project.
• I did want to make the NSStream single responsibility, but first I want the stream to really take a stream, not a whole NSData which can generate pressure on the memory. After that - it's relatively easy to separate responsibilities.
• I hate exceptions. I did not want to change much in your architecture so that was my option :-)
• Unit tests - I'll add some in. We generally need to validate decryption on small and large chunks, and to measure decryption time vs. plain mode.

Some Detail

• ZZConstants.h is meant to be public, because in the future we would specify the encryption method, and right now we might want to allow querying entries for their encryption method. Some will move from ZZHeaders to the ZZConstants too; because Aes strength should also be specified.
• I'm not sure yet about the password property, because theoretically we could encrypt each entry with a different password! I did not want to make a lot of changes to your architecture, especially the public interfaces, so I did this. Of course this will have to change.
• Why do you feel that the isEncrypted isn't necessary? Someone might want to query that. I do not get the point of a minimal interface favored over functionality, but if that's your style we'll stick to that!
• dataWithBytesNoCopy:length:freeWhenDone is a good idea - but I do not like that idea of passing an NSData at all! We need to pass a real stream to the file, instead of reading it all to memory. (I, for example, am testing on large zip files)
• I think your idea about the extra fields are good - I'll look into implementing them.

Style

• Coming from the C / C++ world, I know that the * belongs actually to the var's name. For example: int *abc, *def;. So my gut always tell me to space between the typename and the *. That's just a general note. Aside from that - I'll stick to your style, it's your project! :-)
• Blocks with single line - omitting the surrounding brackets is an opening for bugs when someone adds a line... From experience with teams, stay away from it. There only a few cases where I do keep it a single line, one of them is an if- return;

[pixelglow]

Regarding the C++11 linkage

zipzap's interface must be pure Objective-C, and there should be no need for C++ runtime support (coming in a future commit). For all intents and purposes it should appear as an Objective-C library. Internally, we can use C++/Objective-C++ but we have to be careful not to use features that link in the C++ runtime a.k.a. libc++ or libstdc++ -- these would include virtual member functions, exception handling and the like. You will be able to test whether the feature links in the C++ runtime by simply testing the library; the test will no longer link in libc++ so it will fail to build if we ever inadvertently use such C++ features.

I appreciate the speed and performance of C++ -- I've too have also come from the C/C++ world so I am with you here. So long as the performance code or tight inner loop is either C++ or straight C, you can still wrap it up in a Objective-C class without losing performance.

but first I want the stream to really take a stream, not a whole NSData which can generate pressure on the memory

We are agreed on the stream composability. But do note that almost all the NSData use is either a memory map or a reference to a memory map, so there should be less memory pressure than you might expect.

I hate exceptions

I actually like exceptions, so we must agree to disagree :). However, I would trade exception support for removing libc++ dependency, and will do so in a future commit.

ZZConstants.h is meant to be public

OK, no worries. You'll need to follow the enum style in ZZError.h then, and prepend the public BSD-style license found there.

I'm not sure yet about the password property, because theoretically we could encrypt each entry with a different password!

Hmmm… you really want to have a -[ZZArchiveEntry newStreamWithPassword:] etc. method instead of having a fixed password for the entire archive to decrypt. This would make multiple passwords, even multiple password attempts, much easier and straightforward for the library user.

Why do you feel that the isEncrypted isn't necessary?

Based on some of the concepts (although this is C++) here.

Essentially all non-essential functions i.e. functions that can easily be composed from the core functions and otherwise do not touch class internals, should be external to the class. In Objective-C we have a slightly friendlier way of doing this, using a "category".

I haven't decided whether to support extensive ZZArchive categories for such as writing and reading entries to files, but the point remains it should not be part of the core class definition.

Blocks with single line

Consider it an idiosyncrasy of mine. I hate wasted space :-) .

[pixelglow]

Please rebase against the latest master, and if you're making changes that conceptually belong to an your earlier commits, would appreciate if you git rebase -i and squash that commit and its change together. Would prefer a clean, atomic list of commits without any merges, if possible.

Thanks for your help!

[danielgindi]

Excuse me but how do I do that safely? Rebase the repository on your HEAD, and keep the changes that I have made?

And I apologize deeply for the rhyming - didn't mean to lol!

[pixelglow]

See this, but replace the last step: git checkout master; git merge upstream/master with git checkout master; git rebase upstream/master.

[danielgindi]

Great! That worked. I'll work on it a little more and do a pull request

[pixelglow]

@danielgindi, excellent! You don't have to make a new pull request, any changes in your branch will end up in the original pull request and we can continue conversing on it.

Preferably also remove the last commit @47f7052 since it's a redundant merge e.g. if it is the last commit in your master branch, then git checkout master; git reset --hard HEAD~1 will kill it.

[danielgindi]

OK so now I have completely removed any libc++ dependencies, so the hell of compiling in a new project is gone...

I have taken care of most of the stuff we talked about, and now it's mostly ready to be "official", with interfaces that will allow easy overrides for when we support more encryptions.
The one thing I did not yet do - is separate the Inflate stream to two streams. Actually that stream can use the Decrypt stream internally. But I'll save that for later.

[danielgindi]

BTW that "redundant" merge was required to make SourceTree behave. I guess theoretically it IS redundant, but GUI apps cannot deduce that.

[pixelglow]

Thanks for the hard work you've put into this. It looks much cleaner now, we're almost there!

Source Control

• Commit @47f7052 actually ended up merging in the previous version of your initial commits, which is why it is redundant. I would prefer a clean commit history rather than have previous versions around, this makes bisecting and debugging easier. Here's what I need you to do to fix this:
  • In your local master branch, do git rebase -i -p HEAD~6. This selects the last 6 commits, including the merge commit @47f7052 (-p), in an interactive (-i) rebasing session. An editor should pop up with the history as a file. If you're doing this on a Mac, the editor will be vi.
  • Navigate to the line that starts with pick 47f7052. Delete the line -- if in vi, just type d.
  • Save the file and quit. In vi, that would be :w! followed by :q!. git will then saying it is rebasing.
  • You'll need to push local back to github. Do this with git push --force, since you have rewritten history you'll need the --force.
  • If you now refresh SourceTree on your working copy, you should see a clean, linear history starting from my last commit @00b7925.
  • I have tested this on a local clone of your fork. But if you have any doubt about the above, you can iChat/AIM me at pixglen or iMessage glen dot low at pixelglow dot com and I can guide you through it. Otherwise you can (temporarily) make a contributor on your project and I can remove the offending commit myself -- your contribution is too valuable for us to stumble over source control issues!
• I would prefer atomic commits. Each commit should express a single idea, be fully build-able and not be incomplete if possible, e.g. if the remove file commit is fixing a redundant file you added in a previous commit, those two commits should be squashed together into a single commit. You can do this by using git rebase -i HEAD~n where n is the # of commits to the commits you need to work with, and then reordering the commits and/or changing the pick to a squash. Again if the commits are not atomic, it makes it difficult to bisect if I need to isolate a bug -- a nonatomic commit may not be build-able which makes it impossible for bisect to work on it. Nevertheless, I'm not requiring atomic commits for submission, since it might involve some extra work on your part.
• Philosophically, you might be adverse to revising commit history as above. I know I certainly was, coming from a SourceSafe and then Subversion background. However with DVCS like git, generally private history of your own WIP can and should be rebased into something more logical/atomic when made public. It helps when strangers look at a commit and can figure out what is happening irrespective of the context i.e. the previous and subsequent commits or even the entire topic of branch.

[pixelglow]

General Architecture

• Thanks for removing any libc++ runtime dependency. However, you'll need to keep the CLANG_CXX_LANGUAGE_STANDARD = c++0x, CLANG_CXX_LIBRARY = libc++ and GCC_C_LANGUAGE_STANDARD = c99 in the Xcode project file:
  • Without the language standard option, the build reverts to C++98 or C++11 + GNU extensions. We are still using C++11, albeit without runtime support.
  • Without the library option, the build reverts to using libstdc++. Our code still incorporates several libc++ headers, so we need to make sure the right ones (libc++ and not libstdc++) are compiled in.
  • Ditto the C99 usage, which is for the Objective-C compile.
• Categories e.g. ZZArchive (Encryption) should be in separate files. Typically these are: ZZArchive+Encryption.h and ZZArchive+Encryption.m.
• Omit any out and autoreleasing in the parameter lists of methods. The compiler generally does the right thing with these, otherwise they end up being noise when reading the public API.
• Good work converting from exception throw to explicit error parameters! However, for each set of API like newData, newDataWithError:, newDataWithPassword: and newDataWithPassword:error::
  • Delete the newData. This is a breaking change meant to enforce that we now need to check for errors.
  • Delete the newDataWithPassword:. This is covered by newDataWithPassword:error: and again enforces we need to check for errors.
• ZZDecrypter polymorphic hierarchy needs to be in Objective-C. The use of virtual functions needed to be supported by libc++ runtime, which is no longer present. Also it's not clear which class/function has responsibility for deleting a decrypter, since deleting and creation are in different places in source: if in C++ we would use shared_ptr but Objective-C's ARC strong pointers are the next best thing.
  • One possibility is just to use potentially different ZZDecryptInputStream instead of using different ZZDecrypt -- a couple of less objects to reason about, and the decrypt input stream is simple enough as it is. Potentially the future AES implementation would use OpenSSL directly and might need to be different from the ZZDecrypt interface at any rate. I'll leave it to you to decide this one.
• Don't pass @"description" to the userInfo for ZZRaiseError. Typically NSError has localised descriptions which are not worth the trouble doing. Instead, use custom fields to report "sub-errors", see e.g. ZZEntryIndexKey or just expand the number of errors.
• Thanks for adding tests!

Some Details

• ZZConstants.h:37 should be ZZEncryptionModeAES. And elsewhere, instead of lowercasing it.
• ZZError.h:97 needs to be reconciled with the possibility of CRC errors in unencrypted code. Either:
  • Drop the distinct error and just return the appropriate local file read error, or:
  • Ensure the original check: method returns a CRC error instead of the generic local file read error.
  • The check: method bundles together these expensive checks -- they are expensive because they touch both local and central file headers, so if we put them in init, a simple scan through the array of zip entries ends up paging in the entire memory map! That is why the init method is as simple as possible.
• ZZHeaders.h:12-32 please keep these in C++11 enum class style, since they are internal to zipzap.
• ZZHeaders.h:133 magic constant should be a sign static field.
• ZZOldArchiveEntry.h:30,31 -- why expose these properties and make them assignable? The archive entries are meant to hide/encapsulate these details and not expose them.
• ZZOldArchiveEntry.h:36 should not have an error parameter -- the init method should typically do minimal, non-erroneous initialization. If you want to expose an error, use the check: method.
• ZZOldArchiveEntry.h:108,120 should be member functions on ZZLocalFileHeader etc. as you've done with the rest.

I assume you'd be revising the stream hierarchy before considering this work final. Otherwise, it looks neater and a worthy addition to the codebase!

[danielgindi]

• I do not see why you need to keep the libc++, c99 in the project file. It compiles without it. And the most annoying thing in an open source project, is when you have to go through hoops to find out how to make it compile. An "it just compiles" approach is a basic for any open source project, in my opinion.
• I could separate the Categories to separate files, however note that: A) Apple tends to declare the categories in the same file, so you can find all of the available functionality without looking around too much. B) Some future categories might need access to private members of the ZZArchive.
• The autoreleasing is not required, and actually added automatically by XCode when I looked away... But the out is standard, and tells the compiler to do the right thing. I'm not sure if ARC needs it or not. The implying of autoreleasing tends to make me think it is required to prevent bugs.
• Well, I do not believe that the ZZDecrypter should be ObjC. It is faster in clean C++, and the virtual functions do not require the user to make special configuration changes to his project.
  I think the responsibility for deleting a ZZDecrypter is pretty clear; The ownership is transferred to the NSInputStream, which theoretically is the one who persists longer than any of the other objects, and is the one actually using the ZZDecrypter. Why do you insist on converting ZZDecrypter to ObjC? It is internal anyway...
• About AES, I actually think about implementing our own code, instead of having dependencies. openssl is an overkill just to be using AES...
• Actually I do not understand your current method of CRC. The CRC should be generated as you read the entire entry - and then checked against the CRC in the headers. So in your current approach, if I wanted to generate an error the the CRC is invalid (and so the password is possibly wrong) - I will need to decrypt/uncompress the whole entry again. CRC is kind of expensive, but the way most apps do it, is calculating it on the way, to ensure data integrity (which is kind of the point...).
• The ZZHeaders, currently after moving away from enum classes, do not reqiure that you change your compiler configuration. With enum class it does not matter that it is internal - still if you add the library (as sources) to your project, you have to change your project's configuration...
• ZZOldArchiveEntry.h:30,31 - I have exposed them because we need access to them in the ZZArchive (Encryption). They are readonly. Of course they are assign - they are not an ObjC pointer~
• ZZOldArchiveEntry.h:36 - Actually, Apple's recommendation is that init checks for error and returns nil if the object could not be initialized, or for example, was passed invalid data that it cannot work with. This is a classic init returns nil situation. And if I fail to init - I want the user to be able to know why. There are errors that are detectable regardless of the CRC, and you just know that you can't go ahead.
• ZZOldArchiveEntry.h:108,120 - I think you have pointed me to the wrong file :-)

I'm not sure yet about the stream hierarchy, but after I play with it a little I'll be able to decide...

[pixelglow]

I do not see why you need to keep the libc++, c99 in the project file. It compiles without it.

The libc++ option is to incorporate the libc++ headers, which are C++11 compliant. If you omit it, it tries to incorporate the libstdc++ headers which are pre-C++11. For example, try the following:

#include <regex>

int main()
{
}

Compiling with clang test.cpp -otest -stdlib=libc++ succeeds but with clang test.cpp -otest fails with 'regex' file not found, since <regex> is a C++11-only header.

Since gcc 4.2 (Xcode 3?), Apple has not updated the system libstdc++, but only libc++. Therefore omitting -stdlib=libc++ will end restricting our C++11 usage to all the pre-C+11 headers. For now, we are OK because we're just minimally using libc++ but I don't want to restrict future expansion e.g. some nice algorithm that is C++11 but not pre-C++11.

The C99 option is mainly for consistency, since we already define C++11 and libc++ usage. But I also wanted to avoid inadvertently using too many non-standard C features, see this.

An "it just compiles" approach is a basic for any open source project, in my opinion.

I fully agree with you. I believe that these options, as present in the Xcode project file, allow the project to be correctly built without any further mucking around. Also these options need not be set on any project that uses zipzap, we have effectively made it a black box by dropping the libc++ runtime dependency.

[danielgindi]

Trying to rebase exactly as you detailed - ended up in a hell of merging. Before even going to source tree - it failed to rebase, saying I need to manually tell it how to apply b78c500967, then it requires a merge, then another... Then I gave up and reverted everything.

[pixelglow]

OK, grant me contributor status on your project and I'll try to fix it.

[pixelglow]

I could separate the Categories to separate files, however note that: A) Apple tends to declare the categories in the same file, so you can find all of the available functionality without looking around too much.

You can see for yourself in some of the most popular Objective-C github etc. repos: fmdb, cocos2d, appledoc, sparkle.

I'll accept declaring the categories in the same header file, but the implementation should at least be in a separate .m file.

B) Some future categories might need access to private members of the ZZArchive.

Not generally a good idea.

For example, in the NSArray design, the core functionality is expressed with just count: and objectAtIndex:, all the other methods are in the category (see the NSArray header file) and don't call into any private methods.

Effectively, categories are a "second-class" interface which needs to use the "first-class" interface of the object itself, which is a good thing for extensibility.

[pixelglow]

The autoreleasing is not required, and actually added automatically by XCode when I looked away... But the out is standard, and tells the compiler to do the right thing. I'm not sure if ARC needs it or not. The implying of autoreleasing tends to make me think it is required to prevent bugs.

autoreleasing is implied by double-star arguments. See this -- note that T is like NSError* and T* is like NSError**.

You have a point with out arguments, it does seem to affect the way the compiler generates code! See this. Please do you intended, leaving the out in the method API.

[pixelglow]

Well, I do not believe that the ZZDecrypter should be ObjC. It is faster in clean C++, and the virtual functions do not require the user to make special configuration changes to his project.

Virtual functions require linking in libc++. For proof, try the following:

struct C
{
    virtual void m() {}
};

int main()
{
    C c;
    c.m();
}

When compiled with clang test.cpp -otest it fails to find some __class_type_info class. However using clang test.cpp -lc++ -otest (which links in libc++) succeeds. Also try dropping the virtual qualifier, and you'll see either compiler invocation will succeed.

Therefore if you use virtual functions you necessarily force the library user to also link in libc++, which is what we wanted to avoid in the first place.

I think the responsibility for deleting a ZZDecrypter is pretty clear; The ownership is transferred to the NSInputStream, which theoretically is the one who persists longer than any of the other objects, and is the one actually using the ZZDecrypter.

There is no explicit "transfer of ownership" -- either an ARC object pointer or a shared_ptr or unique_ptr would do this, but not a naked C++ pointer. Therefore it's by convention that your NSInputStream subclass cleans up the decrypter, and since creation and deletion are far apart in the source code, it's easy for the next guy to use the subclass to forget that it now owns the decrypter.

[pixelglow]

About AES, I actually think about implementing our own code, instead of having dependencies. openssl is an overkill just to be using AES…

We can cross that bridge when we come to it.

However consider that all Mac OS X and iOS machines already have openssl on them, so it's a dependency that's already present. (Unlike libc++, which is only present on modern Mac and iOS machines, whereas libstdc++ is present on older machines…)

[pixelglow]

Actually I do not understand your current method of CRC. The CRC should be generated as you read the entire entry - and then checked against the CRC in the headers. So in your current approach, if I wanted to generate an error the the CRC is invalid (and so the password is possibly wrong) - I will need to decrypt/uncompress the whole entry again. CRC is kind of expensive, but the way most apps do it, is calculating it on the way, to ensure data integrity (which is kind of the point…).

The CRC check in check: is really only a sanity check -- check: is not meant to do any decompression or decryption. The actual CRC check against data should be done when the stream/data/provider is extracted, but only the newStream method reports this via the streamError method. It's hard to do this with either data or provider since as you've said, we would have to extract twice just to verify the CRC, so I just ignored it :( -- it probably makes sense to pass out an error with newData since all the data is extracted by the end of method call, but it would be hard to do this for newDataProvider.

[danielgindi]

OK I'm going to add a commit that addes the out to all NSError** in the project

[pixelglow]

The ZZHeaders, currently after moving away from enum classes, do not reqiure that you change your compiler configuration. With enum class it does not matter that it is internal - still if you add the library (as sources) to your project, you have to change your project's configuration…

Yes, but our internal standard is C++ enum classes, because of their convenience/neatness in C++ code.

The library is not meant to be added as sources to other projects -- but if other projects want to do that to access the internal goodies, they have to play by the internal rules...

[pixelglow]

ZZOldArchiveEntry.h:30,31 - I have exposed them because we need access to them in the ZZArchive (Encryption). They are readonly. Of course they are assign - they are not an ObjC pointer~

Don't expose these internals. ZZArchive(Encryption) will need some other method to get what it needs.

[pixelglow]

ZZOldArchiveEntry.h:36 - Actually, Apple's recommendation is that init checks for error and returns nil if the object could not be initialized, or for example, was passed invalid data that it cannot work with. This is a classic init returns nil situation. And if I fail to init - I want the user to be able to know why. There are errors that are detectable regardless of the CRC, and you just know that you can't go ahead.

The trouble is this: whenever you first access the ZZArchive.entries properties, it reads all the central file header into memory by creating the backing ZZOldArchiveEntry objects. If any of this reading fails e.g. the archive entry fails to be created, then the entire entries property is invalid (or is missing entries?). This means for example:

• Any utility or zip file recovery that needs to get partial results will not work.
• If the file contains unsupported encryption, it will fail to have correct entries property.

Yes, I agree it's not ideal that an "erroneous" object ends up being created, it goes against basic object-oriented principles. However in this case it's a tradeoff to allow familiar container-based navigation of the zip file with little memory overhead i.e. by just iterating through entries properties not worrying about whether they are good or bad, until you need to find it out.

[danielgindi]

I've added you as a collaborator. I hope you can fix the Git as you like :-)

[danielgindi]

I've done some changes according to our conversations from today; The streams are now single responsibility, error handling is not done in the init so you can keep the entries full and ready to traverse. And some other stuff as you can see in the commits.

[pixelglow]

I've added you as a collaborator. I hope you can fix the Git as you like :-)

Thanks for your trust. Give me about 10-15 mins and don't commit anything to it. I'll tell you how it went.

[pixelglow]

OK, I've removed the extraneous commit and forced an update. To update your local branch, look at this.

In particular, you'll need to:

git fetch
git reset origin/master --hard

[danielgindi]

Worked great :-)

[danielgindi]

Do you know, if one day I delete my copy of your git, will it harm your git's history?

[mikeabdullah]

Please don't rely on the OS's version of openssl. Doing so has been deprecated by Apple some time now, and they recommend you ship your own if needed, instead.

[pixelglow]

@danielgindi -- if you delete your git, it shouldn't harm the main git, since branches effectively maintain a reference count to their commits, therefore so long as there some branch somewhere referring to it, the commits will remain.

Typically, github contributors will maintain a master branch sync'ed to the original project branch, and another branch that expresses the pull request. That way, you could do separate development of another branch for another pull request, and always keep the master "clean". See for example this and that.

[pixelglow]

@mikeabdullah -- good point, looks like we could use Common Crypto which is available back to OS X 10.5 and iOS 5.0. Conveniently we only profess compatibility with OS X 10.7 and iOS 5.0 and later.

[mikeabdullah]

Yes, if it meets your needs, you'd likely be mad not to use CommonCrypto for a project like this.

[danielgindi]

@mikeabdullah Yes, when I get to it, I'll look into CommonCrypto first. Do you have a need for AES right now?

@pixelglow I'm pretty satisfied with the result right now. Do you want to merge into your master branch?

[mikeabdullah]

Nope, I'm not even using zipzap; just an interested party

[pixelglow]

@danielgindi -- we're almost there, but you would need to address:

• When I meant that the input streams should be composable, I meant that the init of a stream class should take another stream instance.
• ZZDecrypter hierarchy should not use C++ virtual member function for polymorphism. See comments above.
• ZZOldArchiveEntry should not expose file headers.

But if you're happy with it now, I can massage the code a little to address the above while in your branch and before merging it. I'll leave it up to you whether you want to continue working on it.

[danielgindi]

Well, go ahead :-)
I'll try and find some time next week to maybe work a little on AES decryption.
I think for now we can put encryption aside, as most people probably do more decompressing in the app rather than compressing.

[pixelglow]

OK, folks, I've done some minor surgery before merging:

• Incorporated the new composable input streams. Now ZZInflateInputStream and ZZDecryptInputStream are totally separate and can be put together in any order.
• Rebased to 5 atomic commits: all can be built and tested successfully, and each incorporate the relevant bug fixes, refactorings etc.
• ZZOldArchiveEntry no longer exposes file headers, you can use encrypted property on the entry instead of isEncrypted on the archive.

I'll work a little more on the ZZDecrypter hierarchy afterwards, but I'm happy to merge it down. @danielgindi, thanks for your hard work and patience!