Broker

<a href='#crh-start'></a><a href='#crh-data-%7B%22processed%22%3A%20%5B%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21695061%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21695485%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21695588%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21695766%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21696255%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21698483%22%5D%2C%20%22comments%22%3A%20%7B%22Pull%20b1af80b6a90386ea9a85f16634df8220643b54fe%20ADALiOS/ADALiOSTests/ADAuthenticationContextTests.m%2061%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21698483%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22When%20you%20call%20dispatch_release%20for%20this%20semaphore%2C%20signal%20and%20wait%20needs%20to%20be%20balanced.%20Reusing%20in%20multiple%20calls%20can%20give%20you%20weird%20error%2C%20but%20each%20call%20looks%20like%20in%20sync.%20I%20guess%20putitng%20the%20dispatch_release%20to%20the%20end%20will%20be%20ok.%20%20%22%2C%20%22created_at%22%3A%20%222014-12-11T18%3A53%3A03Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/466805%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/omercs%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20ADALiOS/ADALiOSTests/ADAuthenticationContextTests.m%3AL1146-1161%22%7D%2C%20%22Pull%20b1af80b6a90386ea9a85f16634df8220643b54fe%20ADALiOS/ADALiOS/ADBrokerKeyHelper.h%2015%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21695588%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Is%20this%20key%20created%20from%20app%3F%22%2C%20%22created_at%22%3A%20%222014-12-11T18%3A16%3A16Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/466805%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/omercs%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20ADALiOS/ADALiOS/ADBrokerKeyHelper.h%3AL37-44%22%7D%2C%20%22Pull%20b1af80b6a90386ea9a85f16634df8220643b54fe%20ADALiOS/ADALiOS/ADAuthenticationContext.m%2051%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21695061%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22No%20action%20but%20to%20think%20about%3A%20This%20is%20a%20digest%20hash%20check.%20Caller%20could%20replace%20hash%20and%20modify%20message.%20Adal%20will%20say%20hashes%20are%20ok%20and%20move%20to%20decrypt%20step.%20Caller%20can%20make%20up%20multiple%20fake%20calls%20to%20get%20more%20infor%20about%20app%27s%20key.%20hmac%20gives%20more%20security%20since%20it%20will%20fail%20to%20validate%20if%20message%20changed.%22%2C%20%22created_at%22%3A%20%222014-12-11T18%3A07%3A59Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/466805%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/omercs%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20ADALiOS/ADALiOS/ADAuthenticationContext.m%3AL192-245%22%7D%2C%20%22Pull%20b1af80b6a90386ea9a85f16634df8220643b54fe%20ADALiOS/ADALiOSTests/XCTestCase%252BTestHelperMethods.m%20188%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21696255%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22I%20think%20you%20need%20%5Cr%5Cndispatch_release%28sem%29%5Cr%5Cnafter%20wait%20is%20done.%22%2C%20%22created_at%22%3A%20%222014-12-11T18%3A24%3A59Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/466805%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/omercs%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20ADALiOS/ADALiOSTests/XCTestCase%2BTestHelperMethods.m%3AL532-551%22%7D%2C%20%22Pull%20b1af80b6a90386ea9a85f16634df8220643b54fe%20ADALiOS/ADALiOS/ADBrokerKeyHelper.m%2082%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21695766%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22i%20suggest%20to%20check%20if%20memory%20allocated%20or%20not%3F%22%2C%20%22created_at%22%3A%20%222014-12-11T18%3A18%3A54Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/466805%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/omercs%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20ADALiOS/ADALiOS/ADBrokerKeyHelper.m%3AL161-204%22%7D%2C%20%22Pull%20b1af80b6a90386ea9a85f16634df8220643b54fe%20ADALiOS/ADALiOS/ADAuthenticationResult%252BInternal.m%2024%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21695485%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22wrong%20key%22%2C%20%22created_at%22%3A%20%222014-12-11T18%3A14%3A56Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/466805%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/omercs%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20ADALiOS/ADALiOS/ADAuthenticationResult%2BInternal.m%3AL102-142%22%7D%7D%7D'></a>
<a href='https://www.codereviewhub.com/'><img src='http://www.codereviewhub.com/site/github-bar.png' height=40></a>
- [ ] <a href='#crh-comment-Pull b1af80b6a90386ea9a85f16634df8220643b54fe ADALiOS/ADALiOS/ADAuthenticationContext.m 51'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232#discussion_r21695061'>File: ADALiOS/ADALiOS/ADAuthenticationContext.m:L192-245</a></b>
- <a href='https://github.com/omercs'><img border=0 src='https://avatars.githubusercontent.com/u/466805?v=3' height=16 width=16'></a> No action but to think about: This is a digest hash check. Caller could replace hash and modify message. Adal will say hashes are ok and move to decrypt step. Caller can make up multiple fake calls to get more infor about app's key. hmac gives more security since it will fail to validate if message changed.
- [x] <a href='#crh-comment-Pull b1af80b6a90386ea9a85f16634df8220643b54fe ADALiOS/ADALiOS/ADAuthenticationResult%2BInternal.m 24'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232#discussion_r21695485'>File: ADALiOS/ADALiOS/ADAuthenticationResult+Internal.m:L102-142</a></b>
- <a href='https://github.com/omercs'><img border=0 src='https://avatars.githubusercontent.com/u/466805?v=3' height=16 width=16'></a> wrong key
- [ ] <a href='#crh-comment-Pull b1af80b6a90386ea9a85f16634df8220643b54fe ADALiOS/ADALiOS/ADBrokerKeyHelper.h 15'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232#discussion_r21695588'>File: ADALiOS/ADALiOS/ADBrokerKeyHelper.h:L37-44</a></b>
- <a href='https://github.com/omercs'><img border=0 src='https://avatars.githubusercontent.com/u/466805?v=3' height=16 width=16'></a> Is this key created from app?
- [ ] <a href='#crh-comment-Pull b1af80b6a90386ea9a85f16634df8220643b54fe ADALiOS/ADALiOS/ADBrokerKeyHelper.m 82'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232#discussion_r21695766'>File: ADALiOS/ADALiOS/ADBrokerKeyHelper.m:L161-204</a></b>
- <a href='https://github.com/omercs'><img border=0 src='https://avatars.githubusercontent.com/u/466805?v=3' height=16 width=16'></a> i suggest to check if memory allocated or not?
- [ ] <a href='#crh-comment-Pull b1af80b6a90386ea9a85f16634df8220643b54fe ADALiOS/ADALiOSTests/XCTestCase%2BTestHelperMethods.m 188'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232#discussion_r21696255'>File: ADALiOS/ADALiOSTests/XCTestCase+TestHelperMethods.m:L532-551</a></b>
- <a href='https://github.com/omercs'><img border=0 src='https://avatars.githubusercontent.com/u/466805?v=3' height=16 width=16'></a> I think you need
dispatch_release(sem)
after wait is done.
- [ ] <a href='#crh-comment-Pull b1af80b6a90386ea9a85f16634df8220643b54fe ADALiOS/ADALiOSTests/ADAuthenticationContextTests.m 61'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232#discussion_r21698483'>File: ADALiOS/ADALiOSTests/ADAuthenticationContextTests.m:L1146-1161</a></b>
- <a href='https://github.com/omercs'><img border=0 src='https://avatars.githubusercontent.com/u/466805?v=3' height=16 width=16'></a> When you call dispatch_release for this semaphore, signal and wait needs to be balanced. Reusing in multiple calls can give you weird error, but each call looks like in sync. I guess putitng the dispatch_release to the end will be ok.


<a href='https://www.codereviewhub.com/AzureAD/azure-activedirectory-library-for-objc/pull/232?mark_as_completed=1'><img src='http://www.codereviewhub.com/site/github-mark-as-completed.png' height=26></a>&nbsp;<a href='https://www.codereviewhub.com/AzureAD/azure-activedirectory-library-for-objc/pull/232?approve=1'><img src='http://www.codereviewhub.com/site/github-approve.png' height=26></a>&nbsp;<a href='https://github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232'><img src='http://www.codereviewhub.com/site/github-refresh.png' height=26></a>
<a href='#crh-end'></a>
:+1: