)]}'
{"swift/common/crypto_utils.py":[{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"7d06d48e19282d748e5b54c2a0601e1940cb93c4","unresolved":false,"context_lines":[{"line_number":25,"context_line":"        # Get the key(s) from the keymaster"},{"line_number":26,"context_line":"        try:"},{"line_number":27,"context_line":"            if env.get(\u0027swift.crypto.override\u0027, \u0027False\u0027) is True:"},{"line_number":28,"context_line":"                keys \u003d {}"},{"line_number":29,"context_line":"            else:"},{"line_number":30,"context_line":"                fetch_crypto_keys \u003d env[\u0027swift.crypto.fetch_crypto_keys\u0027]"},{"line_number":31,"context_line":"                keys \u003d fetch_crypto_keys()"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba15a1d1_89c4de92","line":28,"updated":"2015-09-14 01:42:29.000000000","message":"Good catch that the handle_get in the decrypterContContext did not check the override flag!  I would like to fix this in a slightly different way, though.  And, I would like to fix it in the review that introduces the problem, which is the \"decrypting container listing\" instead of here.  I am working on a new patch for that other review.  when you rebase, you can remove this and the additional check in the decrypter on the return, and see if it resolves all the issues.  Sound ok?","commit_id":"f6865f6607ca9b518a60f5ca34ca421330384327"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"6ee0f7df01d2b02d427e9bf13e2f277fa7c1b3f9","unresolved":false,"context_lines":[{"line_number":24,"context_line":"    def get_keys(env):"},{"line_number":25,"context_line":"        # Get the key(s) from the keymaster"},{"line_number":26,"context_line":"        try:"},{"line_number":27,"context_line":"            if env.get(\u0027swift.crypto.override\u0027, \u0027False\u0027) is True:"},{"line_number":28,"context_line":"                keys \u003d {}"},{"line_number":29,"context_line":"            else:"},{"line_number":30,"context_line":"                fetch_crypto_keys \u003d env[\u0027swift.crypto.fetch_crypto_keys\u0027]"}],"source_content_type":"text/x-python","patch_set":9,"id":"7a5de9d1_d0a16d47","line":27,"range":{"start_line":27,"start_character":64,"end_line":27,"end_character":65},"updated":"2016-01-30 20:08:41.000000000","message":"The changes to this file are no longer needed.  If this override is set, then \"get_keys\" won\u0027t be called.  I think this was a bug previously that now is fixed.  see the override flag is now checked here: https://github.com/openstack/swift/blob/feature/crypto/swift/common/middleware/decrypter.py#L140","commit_id":"db06b613762c101d1f179a872e4fe65472706522"}],"swift/common/middleware/decrypter.py":[{"author":{"_account_id":17156,"name":"Hamdi Roumani","email":"roumani@ca.ibm.com","username":"hroumani"},"change_message_id":"e572d7afaca867a50c3f6a0f23023ad9b2a04fcb","unresolved":false,"context_lines":[{"line_number":302,"context_line":""},{"line_number":303,"context_line":"        keys \u003d CryptoUtils.get_keys(req.environ)"},{"line_number":304,"context_line":""},{"line_number":305,"context_line":"        # if override is true and keys are empty, return and no decryption"},{"line_number":306,"context_line":"        if keys \u003d\u003d {}:"},{"line_number":307,"context_line":"            start_response(self._response_status, self._response_headers,"},{"line_number":308,"context_line":"                           self._response_exc_info)"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba15a1d1_ae1c4814","line":305,"updated":"2015-09-19 16:16:45.000000000","message":"This comment mentions \u0027override\u0027 being true, from the context of this function it\u0027s really not at all clear what that refers to.\n\nNow I know CryptoUtils.get_keys checks just this ; )  But I\u0027d simple remove any mention over \u0027override\u0027 here, you\u0027ve encapsulated the handling of that (which is great).  Maybe just phrase is as:  \"If get_keys doesn\u0027t return anything it implies no decryption is required.","commit_id":"f6865f6607ca9b518a60f5ca34ca421330384327"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"7d06d48e19282d748e5b54c2a0601e1940cb93c4","unresolved":false,"context_lines":[{"line_number":307,"context_line":"            start_response(self._response_status, self._response_headers,"},{"line_number":308,"context_line":"                           self._response_exc_info)"},{"line_number":309,"context_line":"            return app_resp"},{"line_number":310,"context_line":""},{"line_number":311,"context_line":"        out_content_type \u003d get_listing_content_type(req)"},{"line_number":312,"context_line":""},{"line_number":313,"context_line":"        # Pre-process the response body since the content-length will change"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba15a1d1_e910faaa","line":310,"updated":"2015-09-14 01:42:29.000000000","message":"this is the part I referred to in the previous comment.  I think that this will change, but I will change it in the other review.  Then you can rebase.","commit_id":"f6865f6607ca9b518a60f5ca34ca421330384327"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"6ee0f7df01d2b02d427e9bf13e2f277fa7c1b3f9","unresolved":false,"context_lines":[{"line_number":280,"context_line":""},{"line_number":281,"context_line":"        keys \u003d CryptoUtils.get_keys(req.environ)"},{"line_number":282,"context_line":""},{"line_number":283,"context_line":"        # if override is true and keys are empty, return and no decryption"},{"line_number":284,"context_line":"        if keys \u003d\u003d {}:"},{"line_number":285,"context_line":"            start_response(self._response_status, self._response_headers,"},{"line_number":286,"context_line":"                           self._response_exc_info)"}],"source_content_type":"text/x-python","patch_set":9,"id":"7a5de9d1_d06f2dfa","line":283,"updated":"2016-01-30 20:08:41.000000000","message":"After you rebase onto the latest, you will no longer need to include the check for the override flag in the CryptoUtils.  The only reference to that flag in decrypter should be: https://github.com/openstack/swift/blob/feature/crypto/swift/common/middleware/decrypter.py#L140","commit_id":"db06b613762c101d1f179a872e4fe65472706522"}],"swift/common/middleware/keymaster.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"16214f5e2754a745cd7fe88c5aa461ffa94f7e24","unresolved":false,"context_lines":[{"line_number":76,"context_line":""},{"line_number":77,"context_line":"    def PUT(self, req, start_response):"},{"line_number":78,"context_line":"        if req.headers.get(\u0027X-Encrypt-Me\u0027, \u0027True\u0027) \u003d\u003d \u0027False\u0027:"},{"line_number":79,"context_line":"            req.environ[\u0027swift.crypto.override\u0027] \u003d True"},{"line_number":80,"context_line":"        else:"},{"line_number":81,"context_line":"            if self.obj_path:"},{"line_number":82,"context_line":"                # TODO: re-examine need for this special handling once COPY has"}],"source_content_type":"text/x-python","patch_set":10,"id":"5a18252c_caafb392","line":79,"updated":"2016-04-13 14:27:17.000000000","message":"suggest x-crypto-override as header name","commit_id":"370443e7c66867733f7b93e3622570f8287dd6fa"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7919896a8abaaf3089e5d501095fe6d1f09491df","unresolved":false,"context_lines":[{"line_number":75,"context_line":"        return resp"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"    def PUT(self, req, start_response):"},{"line_number":78,"context_line":"        if req.headers.get(\u0027X-Crypto-Override\u0027, \u0027False\u0027) \u003d\u003d \u0027True\u0027:"},{"line_number":79,"context_line":"            req.environ[\u0027swift.crypto.override\u0027] \u003d True"},{"line_number":80,"context_line":"        else:"},{"line_number":81,"context_line":"            if self.obj_path:"}],"source_content_type":"text/x-python","patch_set":12,"id":"1a122d0e_3b6ad2ee","line":78,"updated":"2016-04-19 15:07:27.000000000","message":"we need this for POST too I think?\n\nis it true that if overide is set in the write path then when we read an object there will be no crypto meta and so no attempt is made to decrypt - I think that should eb the case which means the override header would not be needed for GET/HEAD.","commit_id":"1bfe64725b99f964db087b48d4e020c82279210a"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"361c7c5cb0dc5cca7dfa889843d495db66a228d8","unresolved":false,"context_lines":[{"line_number":75,"context_line":"        return resp"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"    def PUT(self, req, start_response):"},{"line_number":78,"context_line":"        if req.headers.get(\u0027X-Crypto-Override\u0027, \u0027False\u0027) \u003d\u003d \u0027True\u0027:"},{"line_number":79,"context_line":"            req.environ[\u0027swift.crypto.override\u0027] \u003d True"},{"line_number":80,"context_line":"        else:"},{"line_number":81,"context_line":"            if self.obj_path:"}],"source_content_type":"text/x-python","patch_set":12,"id":"1a122d0e_958e332c","line":78,"in_reply_to":"1a122d0e_3b6ad2ee","updated":"2016-04-20 07:11:29.000000000","message":"We don\u0027t have any failing tests due to POST as of now, so i didn\u0027t update it. Maybe in future when we do have those cases we could take care?\n\nYou got it right about why the header is not needed in GET/HEAD.","commit_id":"1bfe64725b99f964db087b48d4e020c82279210a"}],"swift/common/middleware/trivial_keymaster.py":[{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"7d06d48e19282d748e5b54c2a0601e1940cb93c4","unresolved":false,"context_lines":[{"line_number":68,"context_line":""},{"line_number":69,"context_line":"    def PUT(self, req, start_response):"},{"line_number":70,"context_line":"        if req.headers.get(\u0027X-Encrypt-Me\u0027, \u0027True\u0027) \u003d\u003d \u0027False\u0027:"},{"line_number":71,"context_line":"            req.environ[\u0027swift.crypto.override\u0027] \u003d True"},{"line_number":72,"context_line":"        else:"},{"line_number":73,"context_line":"            if self.obj_path:"},{"line_number":74,"context_line":"                # TODO: re-examine need for this special handling once COPY has"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba15a1d1_4949e6cf","line":71,"updated":"2015-09-14 01:42:29.000000000","message":"a couple things here.  1) Call the \"mark_no_encrypt\" method instead of setting the env variable directly.  2) I would rather make this check once in the __call__ method.  That way, you don\u0027t have to do it twice, and the context object would not even need to be invoked.","commit_id":"f6865f6607ca9b518a60f5ca34ca421330384327"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"70457e33d59c304b5ccccf46599fb3df07280be2","unresolved":false,"context_lines":[{"line_number":68,"context_line":""},{"line_number":69,"context_line":"    def PUT(self, req, start_response):"},{"line_number":70,"context_line":"        if req.headers.get(\u0027X-Encrypt-Me\u0027, \u0027True\u0027) \u003d\u003d \u0027False\u0027:"},{"line_number":71,"context_line":"            req.environ[\u0027swift.crypto.override\u0027] \u003d True"},{"line_number":72,"context_line":"        else:"},{"line_number":73,"context_line":"            if self.obj_path:"},{"line_number":74,"context_line":"                # TODO: re-examine need for this special handling once COPY has"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a1a9d01_e0afb392","line":71,"in_reply_to":"9a1a9d01_1d581356","updated":"2015-09-24 16:16:20.000000000","message":"If that method was removed, then no need to change this.","commit_id":"f6865f6607ca9b518a60f5ca34ca421330384327"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"6950adfdd5a3f80dea93044029a1e92ac4ca6351","unresolved":false,"context_lines":[{"line_number":68,"context_line":""},{"line_number":69,"context_line":"    def PUT(self, req, start_response):"},{"line_number":70,"context_line":"        if req.headers.get(\u0027X-Encrypt-Me\u0027, \u0027True\u0027) \u003d\u003d \u0027False\u0027:"},{"line_number":71,"context_line":"            req.environ[\u0027swift.crypto.override\u0027] \u003d True"},{"line_number":72,"context_line":"        else:"},{"line_number":73,"context_line":"            if self.obj_path:"},{"line_number":74,"context_line":"                # TODO: re-examine need for this special handling once COPY has"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a1a9d01_1d581356","line":71,"in_reply_to":"ba15a1d1_4949e6cf","updated":"2015-09-24 14:30:22.000000000","message":"I will have to see how many times I will need a \"mark_no_encrypt\", since it is now not around in this file, right?","commit_id":"f6865f6607ca9b518a60f5ca34ca421330384327"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"7d06d48e19282d748e5b54c2a0601e1940cb93c4","unresolved":false,"context_lines":[{"line_number":80,"context_line":"                id_name \u003d \"%scrypto-id\" % get_obj_persisted_sysmeta_prefix()"},{"line_number":81,"context_line":"                req.headers[id_name] \u003d \\"},{"line_number":82,"context_line":"                    base64.b64encode(self.obj_path)"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":"            if \u0027X-client-obj-key\u0027 in req.headers:"},{"line_number":85,"context_line":"                self.keys[\u0027object\u0027] \u003d req.headers[\u0027X-client-obj-key\u0027]"},{"line_number":86,"context_line":"            if \u0027X-client-cont-key\u0027 in req.headers:"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba15a1d1_89029e3d","line":83,"updated":"2015-09-14 01:42:29.000000000","message":"you could make a method called apply_client_keys or something that would encapsulate the logic to use the client keys if they are present.  That way, if there are changes to that logic, it only will need to be modified in one place instead of 3 places.","commit_id":"f6865f6607ca9b518a60f5ca34ca421330384327"},{"author":{"_account_id":17156,"name":"Hamdi Roumani","email":"roumani@ca.ibm.com","username":"hroumani"},"change_message_id":"e572d7afaca867a50c3f6a0f23023ad9b2a04fcb","unresolved":false,"context_lines":[{"line_number":80,"context_line":"                id_name \u003d \"%scrypto-id\" % get_obj_persisted_sysmeta_prefix()"},{"line_number":81,"context_line":"                req.headers[id_name] \u003d \\"},{"line_number":82,"context_line":"                    base64.b64encode(self.obj_path)"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":"            if \u0027X-client-obj-key\u0027 in req.headers:"},{"line_number":85,"context_line":"                self.keys[\u0027object\u0027] \u003d req.headers[\u0027X-client-obj-key\u0027]"},{"line_number":86,"context_line":"            if \u0027X-client-cont-key\u0027 in req.headers:"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba15a1d1_6e1630f3","line":83,"in_reply_to":"ba15a1d1_89029e3d","updated":"2015-09-19 16:16:45.000000000","message":"Yea I like this idea, if just to simplify the PUT codepath, also for someone readng this I don\u0027t think its very clear what X-client-* would be referring too so add a comment explaining that as well.","commit_id":"f6865f6607ca9b518a60f5ca34ca421330384327"},{"author":{"_account_id":17156,"name":"Hamdi Roumani","email":"roumani@ca.ibm.com","username":"hroumani"},"change_message_id":"e572d7afaca867a50c3f6a0f23023ad9b2a04fcb","unresolved":false,"context_lines":[{"line_number":93,"context_line":"                       self._response_exc_info)"},{"line_number":94,"context_line":"        return resp"},{"line_number":95,"context_line":""},{"line_number":96,"context_line":"    def POST(self, req, start_response):"},{"line_number":97,"context_line":"        if \u0027X-client-obj-key\u0027 in req.headers:"},{"line_number":98,"context_line":"            self.keys[\u0027object\u0027] \u003d req.headers[\u0027X-client-obj-key\u0027]"},{"line_number":99,"context_line":"        if \u0027X-client-cont-key\u0027 in req.headers:"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba15a1d1_2ed7d824","line":96,"updated":"2015-09-19 16:16:45.000000000","message":"Same comment as above, hopefully we can make one function and call it again in all the cases.","commit_id":"f6865f6607ca9b518a60f5ca34ca421330384327"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"7d06d48e19282d748e5b54c2a0601e1940cb93c4","unresolved":false,"context_lines":[{"line_number":105,"context_line":"                       self._response_exc_info)"},{"line_number":106,"context_line":"        return resp"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    def GET(self, req, start_response):"},{"line_number":109,"context_line":"        client_key \u003d False"},{"line_number":110,"context_line":"        if \u0027X-client-obj-key\u0027 in req.headers:"},{"line_number":111,"context_line":"            self.keys[\u0027object\u0027] \u003d req.headers[\u0027X-client-obj-key\u0027]"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba15a1d1_0979aefc","line":108,"updated":"2015-09-14 01:42:29.000000000","message":"all of the following new logic should be in the handle_get_or_head method, since it applies to both.","commit_id":"f6865f6607ca9b518a60f5ca34ca421330384327"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"7d06d48e19282d748e5b54c2a0601e1940cb93c4","unresolved":false,"context_lines":[{"line_number":116,"context_line":"            client_key \u003d True"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"        if client_key:"},{"line_number":119,"context_line":"            if not req.environ.get(\u0027swift.crypto.override\u0027):"},{"line_number":120,"context_line":"                req.environ[\u0027swift.crypto.fetch_crypto_keys\u0027] \u003d \\"},{"line_number":121,"context_line":"                    self.fetch_crypto_keys"},{"line_number":122,"context_line":"            resp \u003d self._app_call(req.environ)"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba15a1d1_694b8ab1","line":119,"updated":"2015-09-14 01:42:29.000000000","message":"if the override value needs to be checked, then it needs to be checked regardless of whether the client provided the key or not. \n\nGood work finding a bug in the previous keymaster logic!  This is similar to the decrypter bug you found where it did not check the override flag in all cases.  I might propose this fix at the same time as the decrypter fix.  then you could rebase.  Then this review would only be addressing the new use of client provided keys and the encrypt-me flag.","commit_id":"f6865f6607ca9b518a60f5ca34ca421330384327"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"70457e33d59c304b5ccccf46599fb3df07280be2","unresolved":false,"context_lines":[{"line_number":116,"context_line":"            client_key \u003d True"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"        if client_key:"},{"line_number":119,"context_line":"            if not req.environ.get(\u0027swift.crypto.override\u0027):"},{"line_number":120,"context_line":"                req.environ[\u0027swift.crypto.fetch_crypto_keys\u0027] \u003d \\"},{"line_number":121,"context_line":"                    self.fetch_crypto_keys"},{"line_number":122,"context_line":"            resp \u003d self._app_call(req.environ)"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a1a9d01_43c8e974","line":119,"in_reply_to":"9a1a9d01_bdec8790","updated":"2015-09-24 16:16:20.000000000","message":"after rebasing with the latest proposal to https://review.openstack.org/#/c/214438/\n(see patchset 8 for these changes), you might not need the checks for the override flags that were added in this review.  It depends on why you added the checks.  If the decrypter was having a problem decrypting in this case, then the patchset 8 will fix that.  However, if there were some assertions added to tests that the keymaster wont derive a key when the override flag is on, then you would need the checks.","commit_id":"f6865f6607ca9b518a60f5ca34ca421330384327"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"6950adfdd5a3f80dea93044029a1e92ac4ca6351","unresolved":false,"context_lines":[{"line_number":116,"context_line":"            client_key \u003d True"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"        if client_key:"},{"line_number":119,"context_line":"            if not req.environ.get(\u0027swift.crypto.override\u0027):"},{"line_number":120,"context_line":"                req.environ[\u0027swift.crypto.fetch_crypto_keys\u0027] \u003d \\"},{"line_number":121,"context_line":"                    self.fetch_crypto_keys"},{"line_number":122,"context_line":"            resp \u003d self._app_call(req.environ)"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a1a9d01_bdec8790","line":119,"in_reply_to":"ba15a1d1_694b8ab1","updated":"2015-09-24 14:30:22.000000000","message":"That\u0027s right, this needs to be checked irrespective of client_key. I could push this in along with the client-key changes, now that we have trivial_keymaster merged in, sounds ok?","commit_id":"f6865f6607ca9b518a60f5ca34ca421330384327"}]}