)]}'
{"swift/common/middleware/crypto/keymaster.py":[{"author":{"_account_id":9625,"name":"Thiago da Silva","email":"thiagodasilva@gmail.com","username":"thiago"},"change_message_id":"cc63dcaf522f660249c34fa3c901790759b8af79","unresolved":false,"context_lines":[{"line_number":122,"context_line":"        # The _get_root_secret() function is overridden by other keymasters"},{"line_number":123,"context_line":"        self.root_secret \u003d self._get_root_secret(conf)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"    def _load_keymaster_config_file(self, conf):"},{"line_number":126,"context_line":"        # Keymaster options specified in the filter section would be ignored if"},{"line_number":127,"context_line":"        # a separate keymaster config file is specified. To avoid confusion,"},{"line_number":128,"context_line":"        # prohibit them existing in the filter section."}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_a1bc35b6","line":125,"range":{"start_line":125,"start_character":8,"end_line":125,"end_character":35},"updated":"2018-07-30 15:04:07.000000000","message":"would it be an overkill to subclass KeyMaster with a \"read from config file\" class. That we could have an abstract _get_root_secret in KeyMaster. Seems cleaner to me, just an idea...","commit_id":"2f56072e3f77fd33b2f9b3890504b2b075ac00f2"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"34314810ea3c0fe59e03c1ab428383739a543e1b","unresolved":false,"context_lines":[{"line_number":122,"context_line":"        # The _get_root_secret() function is overridden by other keymasters"},{"line_number":123,"context_line":"        self.root_secret \u003d self._get_root_secret(conf)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"    def _load_keymaster_config_file(self, conf):"},{"line_number":126,"context_line":"        # Keymaster options specified in the filter section would be ignored if"},{"line_number":127,"context_line":"        # a separate keymaster config file is specified. To avoid confusion,"},{"line_number":128,"context_line":"        # prohibit them existing in the filter section."}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_d78fb21f","line":125,"range":{"start_line":125,"start_character":8,"end_line":125,"end_character":35},"in_reply_to":"5f7c97a3_a1bc35b6","updated":"2018-07-30 16:51:54.000000000","message":"I debated about that... but this all has me nervous about the idea of third-party keymasters subclassing KeyMaster (as we\u0027ve done for KmsKeyMaster and KmipKeyMaster).\n\nThis change already makes it tricky to do that in such a way as to be compatible with multiple versions of Swift -- if you originally wrote your keymaster against 2.18.0 (say), you had to include the conf loading and opt checking in your overridden _get_root_secret, but following this that checking becomes a bit of a liability. You don\u0027t want to come into _get_root_secret with you options already loaded from an external file, notice that you\u0027ve got an external file from which to load options, and declare all the options that have already been loaded for you bad. I was willing to put that aside because you should just need to gate your conf loading off\n\n if self.keymaster_config_path and \\\n         conf[\u0027__file__\u0027] !\u003d self.keymaster_config_path:\n\ninstead of\n\n if self.keymaster_config_path:\n\nAdding a true abstract base class, I\u0027d want names like BaseKeyMaster/KeyMaster... but then already-written libs would be defaulting to using the concrete keymaster *anyway*... so maybe we\u0027d want KeyMaster/TrivialKeyMaster? OTOH, I suppose different names in different versions isn\u0027t *so* horrible... you\u0027d just need something like\n\n try:\n     from swift.common.middleware.crypto.keymaster import BaseKeymaster\n except ImportError:\n     from swift.common.middleware.crypto.keymaster import KeyMaster as BaseKeymaster\n\nI\u0027m rambling... in part because I\u0027m not actually convinced that there *are* any third-party keymaster implementations that would need to do these contortions. Which makes some of the decisions made in https://review.openstack.org/#/c/577874/ seem strange, and which had knock-on effects in https://review.openstack.org/#/c/586902/. Maybe it\u0027s one more argument in favor of an abstract base class -- to highlight which interfaces should be considered stable.\n\nI think I might prefer it as a separate follow-up, though.","commit_id":"2f56072e3f77fd33b2f9b3890504b2b075ac00f2"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"0dee4b0041f18e5180c7968fe4f7cdaa1704c3dc","unresolved":false,"context_lines":[{"line_number":122,"context_line":"        # The _get_root_secret() function is overridden by other keymasters"},{"line_number":123,"context_line":"        self.root_secret \u003d self._get_root_secret(conf)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"    def _load_keymaster_config_file(self, conf):"},{"line_number":126,"context_line":"        # Keymaster options specified in the filter section would be ignored if"},{"line_number":127,"context_line":"        # a separate keymaster config file is specified. To avoid confusion,"},{"line_number":128,"context_line":"        # prohibit them existing in the filter section."}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_1bb866c6","line":125,"range":{"start_line":125,"start_character":8,"end_line":125,"end_character":35},"in_reply_to":"5f7c97a3_d78fb21f","updated":"2018-08-07 23:52:54.000000000","message":"Ok, I don\u0027t think my imagination is big enough to entertain upgrade paths for out of tree keymasters that inherit from this fuzzy definition of an interface as if it\u0027s public and supported.  Let\u0027s not eat a bunch of cruft imagining dragons while we are selves are still exploring/discovering the space in which various implementations can overlap and share code.","commit_id":"2f56072e3f77fd33b2f9b3890504b2b075ac00f2"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"0dee4b0041f18e5180c7968fe4f7cdaa1704c3dc","unresolved":false,"context_lines":[{"line_number":136,"context_line":"                                 \u0027are other config options specified: %s\u0027 %"},{"line_number":137,"context_line":"                                 \", \".join(list("},{"line_number":138,"context_line":"                                     set(keymaster_opts).intersection(conf))))"},{"line_number":139,"context_line":"            conf \u003d readconf(self.keymaster_config_path, \u0027keymaster\u0027)"},{"line_number":140,"context_line":"        b64_root_secret \u003d conf.get(\u0027encryption_root_secret\u0027)"},{"line_number":141,"context_line":"        try:"},{"line_number":142,"context_line":"            binary_root_secret \u003d strict_b64decode(b64_root_secret,"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_dbe66ee9","side":"PARENT","line":139,"updated":"2018-08-07 23:52:54.000000000","message":"pulling this out of here to __init__ seems like a big gigantic pile of FTW","commit_id":"d97673cf5431daf276a2395c449be48951cfd6f0"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"0dee4b0041f18e5180c7968fe4f7cdaa1704c3dc","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        self.app \u003d app"},{"line_number":115,"context_line":"        self.logger \u003d get_logger(conf, log_route\u003dself.log_route)"},{"line_number":116,"context_line":"        self.keymaster_config_path \u003d conf.get(\u0027keymaster_config_path\u0027)"},{"line_number":117,"context_line":"        if type(self) is KeyMaster:"},{"line_number":118,"context_line":"            self.keymaster_opts \u003d (\u0027encryption_root_secret\u0027, )"},{"line_number":119,"context_line":"        if self.keymaster_config_path:"},{"line_number":120,"context_line":"            conf \u003d self._load_keymaster_config_file(conf)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_3be202d7","line":117,"updated":"2018-08-07 23:52:54.000000000","message":"I\u0027m very not fond of the if type checking here - it\u0027s almost anti OO and made me WTF very hard.  Unittests pass with the more obvious pattern of defining the concrete implementation as a class attribute on the concrete class - what am I missing?\n\n\tdiff --git a/swift/common/middleware/crypto/keymaster.py b/swift/common/middleware/crypto/keymaster.py\n\tindex 9b7f64191..56b75bc1d 100644\n\t--- a/swift/common/middleware/crypto/keymaster.py\n\t+++ b/swift/common/middleware/crypto/keymaster.py\n\t@@ -135,16 +135,13 @@ class KeyMaster(object):\n\t     is likely to result in data loss.\n\t     \"\"\"\n\t     log_route \u003d \u0027keymaster\u0027\n\t-    keymaster_opts \u003d ()\n\t+    keymaster_opts \u003d (\u0027encryption_root_secret*\u0027, \u0027active_root_secret_id\u0027)\n\t     keymaster_conf_section \u003d \u0027keymaster\u0027\n\t \n\t     def __init__(self, app, conf):\n\t\t self.app \u003d app\n\t\t self.logger \u003d get_logger(conf, log_route\u003dself.log_route)\n\t\t self.keymaster_config_path \u003d conf.get(\u0027keymaster_config_path\u0027)\n\t-        if type(self) is KeyMaster:\n\t-            self.keymaster_opts \u003d (\u0027encryption_root_secret*\u0027,\n\t-                                   \u0027active_root_secret_id\u0027)\n\t\t if self.keymaster_config_path:\n\t\t     conf \u003d self._load_keymaster_config_file(conf)\n\t \n\n\nI assume if we ever needed an abstract base to inherit from we could define a BaseKeymaster shell with less concrete implementations - but filling in the concrete attributes at runtime seems bonkers to me.","commit_id":"3c92e3ce878d84d951c0acca26d8d9b76edf1bc1"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"40a7f598c9a21821ce3e8d2e200375be3fd17144","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        self.app \u003d app"},{"line_number":115,"context_line":"        self.logger \u003d get_logger(conf, log_route\u003dself.log_route)"},{"line_number":116,"context_line":"        self.keymaster_config_path \u003d conf.get(\u0027keymaster_config_path\u0027)"},{"line_number":117,"context_line":"        if type(self) is KeyMaster:"},{"line_number":118,"context_line":"            self.keymaster_opts \u003d (\u0027encryption_root_secret\u0027, )"},{"line_number":119,"context_line":"        if self.keymaster_config_path:"},{"line_number":120,"context_line":"            conf \u003d self._load_keymaster_config_file(conf)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_a2ffc2ff","line":117,"in_reply_to":"3f79a3b5_3be202d7","updated":"2018-08-08 00:08:16.000000000","message":"Yeah, this was part of my \"imagining dragons\" WRT potential 3rd party keymasters.","commit_id":"3c92e3ce878d84d951c0acca26d8d9b76edf1bc1"}],"swift/common/middleware/crypto/kmip_keymaster.py":[{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"1a62e6b231350c27e0a139622214d52bbd0b0acf","unresolved":false,"context_lines":[{"line_number":85,"context_line":"        if self.keymaster_config_path:"},{"line_number":86,"context_line":"            keymaster_opts \u003d [\u0027host\u0027, \u0027port\u0027, \u0027certfile\u0027, \u0027keyfile\u0027,"},{"line_number":87,"context_line":"                              \u0027ca_certs\u0027, \u0027username\u0027, \u0027password\u0027, \u0027key_id\u0027]"},{"line_number":88,"context_line":"            section \u003d \u0027kmip_keymaster\u0027"},{"line_number":89,"context_line":"            if any(opt in conf for opt in keymaster_opts):"},{"line_number":90,"context_line":"                raise ValueError(\u0027keymaster_config_path is set, but there \u0027"},{"line_number":91,"context_line":"                                 \u0027are other config options specified: %s\u0027 %"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_5dd98f52","side":"PARENT","line":88,"updated":"2018-07-30 07:05:59.000000000","message":"If we need to keep the section local variable, we should at least use the `keymaster_conf_section`:\n\n  section \u003d self.keymaster_conf_section","commit_id":"2f9bd85ae14c7d4a8c0646642e28899de92ec680"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"34314810ea3c0fe59e03c1ab428383739a543e1b","unresolved":false,"context_lines":[{"line_number":85,"context_line":"        if self.keymaster_config_path:"},{"line_number":86,"context_line":"            keymaster_opts \u003d [\u0027host\u0027, \u0027port\u0027, \u0027certfile\u0027, \u0027keyfile\u0027,"},{"line_number":87,"context_line":"                              \u0027ca_certs\u0027, \u0027username\u0027, \u0027password\u0027, \u0027key_id\u0027]"},{"line_number":88,"context_line":"            section \u003d \u0027kmip_keymaster\u0027"},{"line_number":89,"context_line":"            if any(opt in conf for opt in keymaster_opts):"},{"line_number":90,"context_line":"                raise ValueError(\u0027keymaster_config_path is set, but there \u0027"},{"line_number":91,"context_line":"                                 \u0027are other config options specified: %s\u0027 %"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_f7618fd5","side":"PARENT","line":88,"in_reply_to":"5f7c97a3_5dd98f52","updated":"2018-07-30 16:51:54.000000000","message":"Done","commit_id":"2f9bd85ae14c7d4a8c0646642e28899de92ec680"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"0dee4b0041f18e5180c7968fe4f7cdaa1704c3dc","unresolved":false,"context_lines":[{"line_number":94,"context_line":"            raise ValueError("},{"line_number":95,"context_line":"                \u0027KmipKeyMaster config cannot be read from conf dir %s. Use \u0027"},{"line_number":96,"context_line":"                \u0027keymaster_config_path option in the proxy server config to \u0027"},{"line_number":97,"context_line":"                \u0027specify a config file.\u0027)"},{"line_number":98,"context_line":""},{"line_number":99,"context_line":"        key_id \u003d conf.get(\u0027key_id\u0027)"},{"line_number":100,"context_line":"        if not key_id:"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_9f069f86","line":97,"updated":"2018-08-07 23:52:54.000000000","message":"I think a lot of these checks should be moved out of _get_root_secret for exactly all the reasons we did it in the other keymasters...","commit_id":"3c92e3ce878d84d951c0acca26d8d9b76edf1bc1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"0dee4b0041f18e5180c7968fe4f7cdaa1704c3dc","unresolved":false,"context_lines":[{"line_number":106,"context_line":""},{"line_number":107,"context_line":"        with ProxyKmipClient("},{"line_number":108,"context_line":"            config\u003dsection,"},{"line_number":109,"context_line":"            config_file\u003dconf[\u0027__file__\u0027]"},{"line_number":110,"context_line":"        ) as client:"},{"line_number":111,"context_line":"            secret \u003d client.get(key_id)"},{"line_number":112,"context_line":"        if (secret.cryptographic_algorithm.name,"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_7b45dab2","line":109,"updated":"2018-08-07 23:52:54.000000000","message":"so our kmip client really wants to do the config parsing it\u0027s own bad self...","commit_id":"3c92e3ce878d84d951c0acca26d8d9b76edf1bc1"}],"swift/common/middleware/crypto/kms_keymaster.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"40a7f598c9a21821ce3e8d2e200375be3fd17144","unresolved":false,"context_lines":[{"line_number":34,"context_line":"                      \u0027domain_id\u0027, \u0027domain_name\u0027, \u0027project_id\u0027,"},{"line_number":35,"context_line":"                      \u0027project_domain_id\u0027, \u0027reauthenticate\u0027,"},{"line_number":36,"context_line":"                      \u0027auth_endpoint\u0027, \u0027api_class\u0027, \u0027key_id\u0027)"},{"line_number":37,"context_line":"    keymmaster_conf_section \u003d \u0027kms_keymaster\u0027"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"    def _get_root_secret(self, conf):"},{"line_number":40,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_2213b25e","line":37,"range":{"start_line":37,"start_character":7,"end_line":37,"end_character":9},"updated":"2018-08-08 00:08:16.000000000","message":"Type mmuch, Tim?","commit_id":"3c92e3ce878d84d951c0acca26d8d9b76edf1bc1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"0dee4b0041f18e5180c7968fe4f7cdaa1704c3dc","unresolved":false,"context_lines":[{"line_number":34,"context_line":"                      \u0027domain_id\u0027, \u0027domain_name\u0027, \u0027project_id\u0027,"},{"line_number":35,"context_line":"                      \u0027project_domain_id\u0027, \u0027reauthenticate\u0027,"},{"line_number":36,"context_line":"                      \u0027auth_endpoint\u0027, \u0027api_class\u0027, \u0027key_id\u0027)"},{"line_number":37,"context_line":"    keymmaster_conf_section \u003d \u0027kms_keymaster\u0027"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"    def _get_root_secret(self, conf):"},{"line_number":40,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_1b062688","line":37,"updated":"2018-08-07 23:52:54.000000000","message":"pulling this up top is actually really helpful for documentation/visibility","commit_id":"3c92e3ce878d84d951c0acca26d8d9b76edf1bc1"}]}