)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"00624581b2abfa507a8b613bbd49c6acf02e04c2","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This change upgrades the certificate verification functionality"},{"line_number":10,"context_line":"provided in signature_utils, allowing certificates to be verified"},{"line_number":11,"context_line":"against a certificate trust store."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Change-Id: Ifa6582af5f3f7d1f650d226598b486ec403708f1"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"1aa78d24_a24f1fbd","line":11,"updated":"2016-07-14 13:25:26.000000000","message":"This is adding a feature without mentioning any corresponding blueprint / spec","commit_id":"f546c9902740e3745d840b31ebb6b436ae10ddb6"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"2e847f2a0e20b4ba99a00ae863f1083156244fc4","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This change upgrades the certificate verification functionality"},{"line_number":10,"context_line":"provided in signature_utils, allowing certificates to be verified"},{"line_number":11,"context_line":"against a certificate trust store."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Change-Id: Ifa6582af5f3f7d1f650d226598b486ec403708f1"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"1aa78d24_823943f5","line":11,"in_reply_to":"1aa78d24_a24f1fbd","updated":"2016-07-14 13:29:42.000000000","message":"So there is a bug for this, but thats stretching it really, it is a feature.\n\nIt certainly needs a reno note.","commit_id":"f546c9902740e3745d840b31ebb6b436ae10ddb6"}],"nova/conf/glance.py":[{"author":{"_account_id":7012,"name":"Brianna Poulos","email":"Brianna.Poulos@jhuapl.edu","username":"brianna-poulos"},"change_message_id":"91ae43cd5e1c653dd4cb17f12ac4085d24f81c01","unresolved":false,"context_lines":[{"line_number":72,"context_line":"    cfg.StrOpt(\u0027certificate_trust_store\u0027,"},{"line_number":73,"context_line":"               default\u003d\u0027/etc/ssl/certs\u0027,"},{"line_number":74,"context_line":"               help\u003d\u0027The path to the certificate trust store on the compute \u0027"},{"line_number":75,"context_line":"                    \u0027node.\u0027),"},{"line_number":76,"context_line":"    ]"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_efd719bd","line":75,"updated":"2016-06-21 15:03:23.000000000","message":"Note that the help text of these config options is being fleshed out in the patch: https://review.openstack.org/#/c/324007/\n\nSo you might want to add more details here accordingly.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"b4abc80b7f2cad62bf3a318812547026b5a516c1","unresolved":false,"context_lines":[{"line_number":72,"context_line":"    cfg.StrOpt(\u0027certificate_trust_store\u0027,"},{"line_number":73,"context_line":"               default\u003d\u0027/etc/ssl/certs\u0027,"},{"line_number":74,"context_line":"               help\u003d\u0027The path to the certificate trust store on the compute \u0027"},{"line_number":75,"context_line":"                    \u0027node.\u0027),"},{"line_number":76,"context_line":"    ]"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_7a365ee8","line":75,"in_reply_to":"5a9d85d2_efd719bd","updated":"2016-06-22 18:38:56.000000000","message":"Thanks for the reference! I\u0027ll check it out.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"2becf1ba446821815c3dfaa374fed9ff24b978af","unresolved":false,"context_lines":[{"line_number":75,"context_line":"This option specifies the path to the directory containing the certificate"},{"line_number":76,"context_line":"trust store for the compute service. The certificates in the trust store are"},{"line_number":77,"context_line":"loaded and used to validate all certificates retrieved during image signature"},{"line_number":78,"context_line":"verification."},{"line_number":79,"context_line":""},{"line_number":80,"context_line":"* Possible values:"},{"line_number":81,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3aaa91ec_8e62ad3c","line":78,"updated":"2016-06-23 18:51:23.000000000","message":"I think we should be explicit that the trust store is a local folder, not a URL.","commit_id":"3003772b31fadd31d34cd2f6947e9afe8a1bb0eb"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"38cc788cc4b76fa31dc1a957edd76dc7e3820cfa","unresolved":false,"context_lines":[{"line_number":75,"context_line":"This option specifies the path to the directory containing the certificate"},{"line_number":76,"context_line":"trust store for the compute service. The certificates in the trust store are"},{"line_number":77,"context_line":"loaded and used to validate all certificates retrieved during image signature"},{"line_number":78,"context_line":"verification."},{"line_number":79,"context_line":""},{"line_number":80,"context_line":"* Possible values:"},{"line_number":81,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3aaa91ec_718f602a","line":78,"in_reply_to":"3aaa91ec_51bd0401","updated":"2016-06-23 19:06:02.000000000","message":"Actually I like that option. A mounting a read-only folder would allow admins to easily configure a common trust store across multiple compute nodes.","commit_id":"3003772b31fadd31d34cd2f6947e9afe8a1bb0eb"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"dcb2da290fa55d15a3622c6b9effc3e5e990ce55","unresolved":false,"context_lines":[{"line_number":75,"context_line":"This option specifies the path to the directory containing the certificate"},{"line_number":76,"context_line":"trust store for the compute service. The certificates in the trust store are"},{"line_number":77,"context_line":"loaded and used to validate all certificates retrieved during image signature"},{"line_number":78,"context_line":"verification."},{"line_number":79,"context_line":""},{"line_number":80,"context_line":"* Possible values:"},{"line_number":81,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3aaa91ec_743e2e8d","line":78,"in_reply_to":"3aaa91ec_718f602a","updated":"2016-06-23 19:21:37.000000000","message":"Agreed. It makes sense from a risk-minimization perspective.","commit_id":"3003772b31fadd31d34cd2f6947e9afe8a1bb0eb"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"6c4c3915a3292dacc90f18c745bbd3ada3c8fe33","unresolved":false,"context_lines":[{"line_number":75,"context_line":"This option specifies the path to the directory containing the certificate"},{"line_number":76,"context_line":"trust store for the compute service. The certificates in the trust store are"},{"line_number":77,"context_line":"loaded and used to validate all certificates retrieved during image signature"},{"line_number":78,"context_line":"verification."},{"line_number":79,"context_line":""},{"line_number":80,"context_line":"* Possible values:"},{"line_number":81,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3aaa91ec_51bd0401","line":78,"in_reply_to":"3aaa91ec_8e62ad3c","updated":"2016-06-23 19:00:36.000000000","message":"Do we think that the trust store could be in remote storage with a local mount?","commit_id":"3003772b31fadd31d34cd2f6947e9afe8a1bb0eb"},{"author":{"_account_id":7012,"name":"Brianna Poulos","email":"Brianna.Poulos@jhuapl.edu","username":"brianna-poulos"},"change_message_id":"ab951e1e3cbc628bf986540e1be9796c5d6a3627","unresolved":false,"context_lines":[{"line_number":79,"context_line":""},{"line_number":80,"context_line":"* Possible values:"},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"    /etc/ssl/certs (default)"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":"* Related options:"},{"line_number":85,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3aaa91ec_4d2d6e94","line":82,"updated":"2016-06-23 21:53:03.000000000","message":"In addition to the default, it would also be good to explain that any directory path is a valid value.","commit_id":"3003772b31fadd31d34cd2f6947e9afe8a1bb0eb"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"2e847f2a0e20b4ba99a00ae863f1083156244fc4","unresolved":false,"context_lines":[{"line_number":77,"context_line":"                help\u003d\u0027Require Nova to perform signature verification on \u0027"},{"line_number":78,"context_line":"                     \u0027each image downloaded from Glance.\u0027),"},{"line_number":79,"context_line":"    cfg.StrOpt(\u0027certificate_trust_store\u0027,"},{"line_number":80,"context_line":"               default\u003d\u0027/etc/ssl/certs\u0027,"},{"line_number":81,"context_line":"               help\u003d\"\"\""},{"line_number":82,"context_line":"This option specifies the path to the directory containing the certificate"},{"line_number":83,"context_line":"trust store for the compute service. The certificates in the trust store are"}],"source_content_type":"text/x-python","patch_set":7,"id":"1aa78d24_e2a5a747","line":80,"updated":"2016-07-14 13:29:42.000000000","message":"Not sure if there is a good default here... tempting to have no default.","commit_id":"f546c9902740e3745d840b31ebb6b436ae10ddb6"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"2e847f2a0e20b4ba99a00ae863f1083156244fc4","unresolved":false,"context_lines":[{"line_number":90,"context_line":""},{"line_number":91,"context_line":"* Related options:"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"    None"},{"line_number":94,"context_line":"\"\"\",),"},{"line_number":95,"context_line":"    ]"},{"line_number":96,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"1aa78d24_22668f29","line":93,"updated":"2016-07-14 13:29:42.000000000","message":"Surely its related to verify_glance_signatures\u003dTrue, else its not used?","commit_id":"f546c9902740e3745d840b31ebb6b436ae10ddb6"}],"nova/signature_utils.py":[{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"d4665b61ae619931f2315508811e5db79dc59b66","unresolved":false,"context_lines":[{"line_number":82,"context_line":""},{"line_number":83,"context_line":""},{"line_number":84,"context_line":"class InvalidSigningCertificate(Exception):"},{"line_number":85,"context_line":"    pass"},{"line_number":86,"context_line":""},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"def load_certificate(path):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_10b72f87","line":85,"updated":"2016-06-21 14:26:46.000000000","message":"New exceptions should be defined in nova/exception.py. I think we can just stick with SignatureVerificationError for right now. If this code is ever used for any other purpose in OpenStack, I think the new exception types would be more necessary.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7012,"name":"Brianna Poulos","email":"Brianna.Poulos@jhuapl.edu","username":"brianna-poulos"},"change_message_id":"91ae43cd5e1c653dd4cb17f12ac4085d24f81c01","unresolved":false,"context_lines":[{"line_number":82,"context_line":""},{"line_number":83,"context_line":""},{"line_number":84,"context_line":"class InvalidSigningCertificate(Exception):"},{"line_number":85,"context_line":"    pass"},{"line_number":86,"context_line":""},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"def load_certificate(path):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_924e7e6d","line":85,"in_reply_to":"5a9d85d2_10b72f87","updated":"2016-06-21 15:03:23.000000000","message":"I like the idea of having more specific exceptions (it makes debugging a bit easier).  Maybe these can inherit from SignatureVerificationError?","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"b4abc80b7f2cad62bf3a318812547026b5a516c1","unresolved":false,"context_lines":[{"line_number":82,"context_line":""},{"line_number":83,"context_line":""},{"line_number":84,"context_line":"class InvalidSigningCertificate(Exception):"},{"line_number":85,"context_line":"    pass"},{"line_number":86,"context_line":""},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"def load_certificate(path):"}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_9a422a3d","line":85,"in_reply_to":"5a9d85d2_924e7e6d","updated":"2016-06-22 18:38:56.000000000","message":"That\u0027s fine by me. These were carried over from the pyca/cryptography patch so they\u0027re definitely placeholders.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"d4665b61ae619931f2315508811e5db79dc59b66","unresolved":false,"context_lines":[{"line_number":98,"context_line":""},{"line_number":99,"context_line":"    # Convert the raw certificate data into a certificate object, first as a"},{"line_number":100,"context_line":"    # PEM-encoded certificate and, if that fails, then as a DER-encoded"},{"line_number":101,"context_line":"    # certificate. If both fail, the certificate cannot be loaded."},{"line_number":102,"context_line":"    try:"},{"line_number":103,"context_line":"        return x509.load_pem_x509_certificate(data, default_backend())"},{"line_number":104,"context_line":"    except ValueError:"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_c6e96d1c","line":101,"updated":"2016-06-21 14:26:46.000000000","message":"Is there any data on whether PEM or DER is more common? We should be trying the more common format first. I would suggest checking the file extension but I guess .crt could be either PEM or DER.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"b4abc80b7f2cad62bf3a318812547026b5a516c1","unresolved":false,"context_lines":[{"line_number":98,"context_line":""},{"line_number":99,"context_line":"    # Convert the raw certificate data into a certificate object, first as a"},{"line_number":100,"context_line":"    # PEM-encoded certificate and, if that fails, then as a DER-encoded"},{"line_number":101,"context_line":"    # certificate. If both fail, the certificate cannot be loaded."},{"line_number":102,"context_line":"    try:"},{"line_number":103,"context_line":"        return x509.load_pem_x509_certificate(data, default_backend())"},{"line_number":104,"context_line":"    except ValueError:"}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_ba8c4680","line":101,"in_reply_to":"5a9d85d2_c6e96d1c","updated":"2016-06-22 18:38:56.000000000","message":"From looking at the default certificates in Ubuntu 14.04, PEM is the common format, which is why I try that first here. We fall back to DER if that fails. I figured the best way to determine type was to just try loading it one way first and failing over in case we guessed wrong.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"d4665b61ae619931f2315508811e5db79dc59b66","unresolved":false,"context_lines":[{"line_number":102,"context_line":"    try:"},{"line_number":103,"context_line":"        return x509.load_pem_x509_certificate(data, default_backend())"},{"line_number":104,"context_line":"    except ValueError:"},{"line_number":105,"context_line":"        # TODO (pah) Catch just ValueError or any error?"},{"line_number":106,"context_line":"        try:"},{"line_number":107,"context_line":"            return x509.load_der_x509_certificate(data, default_backend())"},{"line_number":108,"context_line":"        except ValueError:"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_eb3ef2d8","line":105,"updated":"2016-06-21 14:26:46.000000000","message":"Looking at the load_pem_x509_certificate and load_der_x509_certificate code in cryptography\u0027s openssl/backend.py, I see that these methods only raise a ValueError. However, looking at how their object hierarchy works, I can see that the exception type for this method is dependent on what the underlying backend throws. So I don\u0027t think we can depend on this only throwing a ValueError in perpetuity.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"b4abc80b7f2cad62bf3a318812547026b5a516c1","unresolved":false,"context_lines":[{"line_number":102,"context_line":"    try:"},{"line_number":103,"context_line":"        return x509.load_pem_x509_certificate(data, default_backend())"},{"line_number":104,"context_line":"    except ValueError:"},{"line_number":105,"context_line":"        # TODO (pah) Catch just ValueError or any error?"},{"line_number":106,"context_line":"        try:"},{"line_number":107,"context_line":"            return x509.load_der_x509_certificate(data, default_backend())"},{"line_number":108,"context_line":"        except ValueError:"}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_1ad11a58","line":105,"in_reply_to":"5a9d85d2_eb3ef2d8","updated":"2016-06-22 18:38:56.000000000","message":"Yes, I\u0027m switching these to a vanilla Exception for now. I figure any error is sufficient to indicate load failure.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"d4665b61ae619931f2315508811e5db79dc59b66","unresolved":false,"context_lines":[{"line_number":129,"context_line":"                cert \u003d load_certificate(file_path)"},{"line_number":130,"context_line":"            except InvalidCertificate:"},{"line_number":131,"context_line":"                # TODO (pah) Catch generic exception here?"},{"line_number":132,"context_line":"                # TODO (pah) Log or raise error?"},{"line_number":133,"context_line":"                LOG.error(\"Failed to load certificate: %s\" % file_path)"},{"line_number":134,"context_line":"                continue"},{"line_number":135,"context_line":"            certs.append(cert)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_a62ee1f7","line":132,"updated":"2016-06-21 14:26:46.000000000","message":"I think catching whatever we expect load_certificate to throw is fine. Logging is good here. If we have a valid, verifiable chain and an extraneous cert fails to load, we don\u0027t want to block validation.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"b4abc80b7f2cad62bf3a318812547026b5a516c1","unresolved":false,"context_lines":[{"line_number":129,"context_line":"                cert \u003d load_certificate(file_path)"},{"line_number":130,"context_line":"            except InvalidCertificate:"},{"line_number":131,"context_line":"                # TODO (pah) Catch generic exception here?"},{"line_number":132,"context_line":"                # TODO (pah) Log or raise error?"},{"line_number":133,"context_line":"                LOG.error(\"Failed to load certificate: %s\" % file_path)"},{"line_number":134,"context_line":"                continue"},{"line_number":135,"context_line":"            certs.append(cert)"}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_7af5bec9","line":132,"in_reply_to":"5a9d85d2_a62ee1f7","updated":"2016-06-22 18:38:56.000000000","message":"Yep, that was my thinking for a log warning instead of an exception. I\u0027ll update this as I fiddle with the exception types.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"d4665b61ae619931f2315508811e5db79dc59b66","unresolved":false,"context_lines":[{"line_number":167,"context_line":"    \"\"\""},{"line_number":168,"context_line":"    # TODO (pah) Check for the subjectAltName extension and its"},{"line_number":169,"context_line":"    # TODO (pah) criticality if the subject is empty and use it for the"},{"line_number":170,"context_line":"    # TODO (pah) comparison instead?"},{"line_number":171,"context_line":"    return issuing_certificate.subject \u003d\u003d issued_certificate.issuer"},{"line_number":172,"context_line":""},{"line_number":173,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_21781b2b","line":170,"updated":"2016-06-21 14:26:46.000000000","message":"Yeah I agree with your comments that this check can\u0027t be this simple. We need some sort of fallback check here.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"365aefb678ab64d3b56547a674ddec195a80c93b","unresolved":false,"context_lines":[{"line_number":167,"context_line":"    \"\"\""},{"line_number":168,"context_line":"    # TODO (pah) Check for the subjectAltName extension and its"},{"line_number":169,"context_line":"    # TODO (pah) criticality if the subject is empty and use it for the"},{"line_number":170,"context_line":"    # TODO (pah) comparison instead?"},{"line_number":171,"context_line":"    return issuing_certificate.subject \u003d\u003d issued_certificate.issuer"},{"line_number":172,"context_line":""},{"line_number":173,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_3c014106","line":170,"in_reply_to":"3aaa91ec_5ab1e2e0","updated":"2016-06-23 15:39:28.000000000","message":"Reading the fine print of RFC 5280 actually reveals that a CA certificate *must* have the subject name filled in:\nhttps://tools.ietf.org/html/rfc5280#section-4.1.2.6\n\nIf it\u0027s not filled in, the certificate cannot be a signing certificate. I\u0027m starting to think that the basic check below is sufficient. What am I missing?","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"b4abc80b7f2cad62bf3a318812547026b5a516c1","unresolved":false,"context_lines":[{"line_number":167,"context_line":"    \"\"\""},{"line_number":168,"context_line":"    # TODO (pah) Check for the subjectAltName extension and its"},{"line_number":169,"context_line":"    # TODO (pah) criticality if the subject is empty and use it for the"},{"line_number":170,"context_line":"    # TODO (pah) comparison instead?"},{"line_number":171,"context_line":"    return issuing_certificate.subject \u003d\u003d issued_certificate.issuer"},{"line_number":172,"context_line":""},{"line_number":173,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_5ab1e2e0","line":170,"in_reply_to":"5a9d85d2_21781b2b","updated":"2016-06-22 18:38:56.000000000","message":"Yes, RFC 5280 defines how to match certificate names:\nhttps://tools.ietf.org/html/rfc5280#section-7.1\n\nIt basically boils down to conducting a strict comparison of the number, value, type, and order of the name attributes contained in each distinguished name structure. pyca/cryptography does a direct attribute list comparison for each name structure. See line 66 here:\nhttps://github.com/pyca/cryptography/blob/master/src/cryptography/x509/name.py","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"d4665b61ae619931f2315508811e5db79dc59b66","unresolved":false,"context_lines":[{"line_number":204,"context_line":"    for name_attribute in certificate.subject:"},{"line_number":205,"context_line":"        if name_attribute._oid \u003d\u003d x509.oid.NameOID.COMMON_NAME:"},{"line_number":206,"context_line":"            common_name \u003d u\u0027\u0027.join(name_attribute._value).encode(\u0027utf-8\u0027)"},{"line_number":207,"context_line":"            break"},{"line_number":208,"context_line":""},{"line_number":209,"context_line":"    return common_name"},{"line_number":210,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_09cc2569","line":207,"updated":"2016-06-21 14:26:46.000000000","message":"I think this can be cleaned up a bit. Can\u0027t we just do a find operation instead of the loop/break way it\u0027s written here?","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"b4abc80b7f2cad62bf3a318812547026b5a516c1","unresolved":false,"context_lines":[{"line_number":204,"context_line":"    for name_attribute in certificate.subject:"},{"line_number":205,"context_line":"        if name_attribute._oid \u003d\u003d x509.oid.NameOID.COMMON_NAME:"},{"line_number":206,"context_line":"            common_name \u003d u\u0027\u0027.join(name_attribute._value).encode(\u0027utf-8\u0027)"},{"line_number":207,"context_line":"            break"},{"line_number":208,"context_line":""},{"line_number":209,"context_line":"    return common_name"},{"line_number":210,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_5aff2267","line":207,"in_reply_to":"5a9d85d2_09cc2569","updated":"2016-06-22 18:38:56.000000000","message":"Yeah, I\u0027m not happy with the for/break approach. I\u0027ll fix it. The common name should always be present according to RFC 5280. Should we raise an error here if we somehow can\u0027t find it?","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"d4665b61ae619931f2315508811e5db79dc59b66","unresolved":false,"context_lines":[{"line_number":234,"context_line":"            signature_bytes, signature_hash_algorithm"},{"line_number":235,"context_line":"        )"},{"line_number":236,"context_line":""},{"line_number":237,"context_line":"    verifier.update(certificate.tbs_certificate_bytes)"},{"line_number":238,"context_line":"    verifier.verify()"},{"line_number":239,"context_line":""},{"line_number":240,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_e93b9936","line":237,"updated":"2016-06-21 14:26:46.000000000","message":"I think this should be in a loop. We don\u0027t have an explicit guarantee that the certificate size is limited to the chunk size of the underlying digest operations here.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"b4abc80b7f2cad62bf3a318812547026b5a516c1","unresolved":false,"context_lines":[{"line_number":234,"context_line":"            signature_bytes, signature_hash_algorithm"},{"line_number":235,"context_line":"        )"},{"line_number":236,"context_line":""},{"line_number":237,"context_line":"    verifier.update(certificate.tbs_certificate_bytes)"},{"line_number":238,"context_line":"    verifier.verify()"},{"line_number":239,"context_line":""},{"line_number":240,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_b5b57732","line":237,"in_reply_to":"5a9d85d2_d245a670","updated":"2016-06-22 18:38:56.000000000","message":"pyca/cryptography returns the entire byte buffer here containing the certificate bytes; it\u0027s not reading it in chunks. See line 144 here:\nhttps://github.com/pyca/cryptography/blob/master/src/cryptography/hazmat/backends/openssl/x509.py\n\nAlso, the verifier expects to receive the full byte blob for verification. See line 50 here (the method used for OpenSSL hashing):\nhttps://github.com/pyca/cryptography/blob/master/src/cryptography/hazmat/backends/openssl/hashes.py\n\nFrom these, I don\u0027t think a loop is needed. I never ran into problems with incomplete certificate byte data when testing this against pyca/cryptography.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7012,"name":"Brianna Poulos","email":"Brianna.Poulos@jhuapl.edu","username":"brianna-poulos"},"change_message_id":"91ae43cd5e1c653dd4cb17f12ac4085d24f81c01","unresolved":false,"context_lines":[{"line_number":234,"context_line":"            signature_bytes, signature_hash_algorithm"},{"line_number":235,"context_line":"        )"},{"line_number":236,"context_line":""},{"line_number":237,"context_line":"    verifier.update(certificate.tbs_certificate_bytes)"},{"line_number":238,"context_line":"    verifier.verify()"},{"line_number":239,"context_line":""},{"line_number":240,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_d245a670","line":237,"in_reply_to":"5a9d85d2_e93b9936","updated":"2016-06-21 15:03:23.000000000","message":"Agreed.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"d4665b61ae619931f2315508811e5db79dc59b66","unresolved":false,"context_lines":[{"line_number":248,"context_line":"        self._signing_certificates \u003d []"},{"line_number":249,"context_line":"        for certificate in certificates:"},{"line_number":250,"context_line":"            if not isinstance(certificate, x509.Certificate):"},{"line_number":251,"context_line":"                # TODO (pah) Raise an error here or log a warning for the cert?"},{"line_number":252,"context_line":"                raise InvalidCertificate("},{"line_number":253,"context_line":"                    \"A signing certificate must be an x509.Certificate \""},{"line_number":254,"context_line":"                    \"object.\""}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_6c03370e","line":251,"updated":"2016-06-21 14:26:46.000000000","message":"Raise an error. This class shouldn\u0027t be handling malformatted certs or other objects.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"b4abc80b7f2cad62bf3a318812547026b5a516c1","unresolved":false,"context_lines":[{"line_number":248,"context_line":"        self._signing_certificates \u003d []"},{"line_number":249,"context_line":"        for certificate in certificates:"},{"line_number":250,"context_line":"            if not isinstance(certificate, x509.Certificate):"},{"line_number":251,"context_line":"                # TODO (pah) Raise an error here or log a warning for the cert?"},{"line_number":252,"context_line":"                raise InvalidCertificate("},{"line_number":253,"context_line":"                    \"A signing certificate must be an x509.Certificate \""},{"line_number":254,"context_line":"                    \"object.\""}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_7a501e5f","line":251,"in_reply_to":"3aaa91ec_b1f804e7","updated":"2016-06-22 18:38:56.000000000","message":"I agree with consistency, otherwise it just gets confusing. I\u0027ll make sure they sync up.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"197f5fcefad138d491c0d0b67391a6e6302ddd89","unresolved":false,"context_lines":[{"line_number":248,"context_line":"        self._signing_certificates \u003d []"},{"line_number":249,"context_line":"        for certificate in certificates:"},{"line_number":250,"context_line":"            if not isinstance(certificate, x509.Certificate):"},{"line_number":251,"context_line":"                # TODO (pah) Raise an error here or log a warning for the cert?"},{"line_number":252,"context_line":"                raise InvalidCertificate("},{"line_number":253,"context_line":"                    \"A signing certificate must be an x509.Certificate \""},{"line_number":254,"context_line":"                    \"object.\""}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_b1f804e7","line":251,"in_reply_to":"5a9d85d2_5d226701","updated":"2016-06-22 11:51:10.000000000","message":"I think you have to just log the error, throw out the offending file, and continue with verification. If we\u0027re using a folder like /etc/ssl/certs as the default trust store, we should expect other processes to be adding and removing files, so it doesn\u0027t make sense to err out just because there\u0027s a non-x509 file there. @Brianna I agree about the consistency issue, my mistake.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7012,"name":"Brianna Poulos","email":"Brianna.Poulos@jhuapl.edu","username":"brianna-poulos"},"change_message_id":"91ae43cd5e1c653dd4cb17f12ac4085d24f81c01","unresolved":false,"context_lines":[{"line_number":248,"context_line":"        self._signing_certificates \u003d []"},{"line_number":249,"context_line":"        for certificate in certificates:"},{"line_number":250,"context_line":"            if not isinstance(certificate, x509.Certificate):"},{"line_number":251,"context_line":"                # TODO (pah) Raise an error here or log a warning for the cert?"},{"line_number":252,"context_line":"                raise InvalidCertificate("},{"line_number":253,"context_line":"                    \"A signing certificate must be an x509.Certificate \""},{"line_number":254,"context_line":"                    \"object.\""}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_5d226701","line":251,"in_reply_to":"5a9d85d2_6c03370e","updated":"2016-06-21 15:03:23.000000000","message":"I think we should be consistent between here and line 133: either we require that all certificates be formatted properly, or we allow malformed/irrelevant certificates as long as there is at least one that works properly.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"d4665b61ae619931f2315508811e5db79dc59b66","unresolved":false,"context_lines":[{"line_number":255,"context_line":"                )"},{"line_number":256,"context_line":""},{"line_number":257,"context_line":"            if not is_within_valid_dates(certificate):"},{"line_number":258,"context_line":"                # TODO (pah) Raise an error here or log a warning for the cert?"},{"line_number":259,"context_line":"                LOG.warning("},{"line_number":260,"context_line":"                    \"The \u0027%s\u0027 certificate is outside its valid date range \""},{"line_number":261,"context_line":"                    \"and cannot be used as a signing certificate.\" % certificate"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_2c480f5b","line":258,"updated":"2016-06-21 14:26:46.000000000","message":"Raise an error. I\u0027m always in favor of hard failure when there\u0027s a potential for fraud.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"365aefb678ab64d3b56547a674ddec195a80c93b","unresolved":false,"context_lines":[{"line_number":255,"context_line":"                )"},{"line_number":256,"context_line":""},{"line_number":257,"context_line":"            if not is_within_valid_dates(certificate):"},{"line_number":258,"context_line":"                # TODO (pah) Raise an error here or log a warning for the cert?"},{"line_number":259,"context_line":"                LOG.warning("},{"line_number":260,"context_line":"                    \"The \u0027%s\u0027 certificate is outside its valid date range \""},{"line_number":261,"context_line":"                    \"and cannot be used as a signing certificate.\" % certificate"}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_9f3947da","line":258,"in_reply_to":"3aaa91ec_1ac8fa21","updated":"2016-06-23 15:39:28.000000000","message":"Following from the above conversation, I\u0027m going to stick with logging warnings for now. However, certificates that fail to pass any of these checks will not be included in the persisted trust store list.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"b4abc80b7f2cad62bf3a318812547026b5a516c1","unresolved":false,"context_lines":[{"line_number":255,"context_line":"                )"},{"line_number":256,"context_line":""},{"line_number":257,"context_line":"            if not is_within_valid_dates(certificate):"},{"line_number":258,"context_line":"                # TODO (pah) Raise an error here or log a warning for the cert?"},{"line_number":259,"context_line":"                LOG.warning("},{"line_number":260,"context_line":"                    \"The \u0027%s\u0027 certificate is outside its valid date range \""},{"line_number":261,"context_line":"                    \"and cannot be used as a signing certificate.\" % certificate"}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_1ac8fa21","line":258,"in_reply_to":"5a9d85d2_2c480f5b","updated":"2016-06-22 18:38:56.000000000","message":"Agreed.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"d4665b61ae619931f2315508811e5db79dc59b66","unresolved":false,"context_lines":[{"line_number":266,"context_line":"                self._signing_certificates.append(certificate)"},{"line_number":267,"context_line":"            else:"},{"line_number":268,"context_line":"                # TODO (pah) Log the fact that the cert couldn\u0027t be used for"},{"line_number":269,"context_line":"                # TODO (pah) signing?"},{"line_number":270,"context_line":"                LOG.warning(\"A signing certificate must be configured to sign \")"},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"        self._signed_cert \u003d None"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_ec78c72d","line":269,"updated":"2016-06-21 14:26:46.000000000","message":"I think we can be a little looser here (i.e. not fail verification), I don\u0027t know how strict intermediate CAs are about configuring this extension.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"365aefb678ab64d3b56547a674ddec195a80c93b","unresolved":false,"context_lines":[{"line_number":266,"context_line":"                self._signing_certificates.append(certificate)"},{"line_number":267,"context_line":"            else:"},{"line_number":268,"context_line":"                # TODO (pah) Log the fact that the cert couldn\u0027t be used for"},{"line_number":269,"context_line":"                # TODO (pah) signing?"},{"line_number":270,"context_line":"                LOG.warning(\"A signing certificate must be configured to sign \")"},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"        self._signed_cert \u003d None"}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_ffb7ab37","line":269,"in_reply_to":"5a9d85d2_ec78c72d","updated":"2016-06-23 15:39:28.000000000","message":"For now I want to make sure we\u0027re following RFC 5280 best practice as close as possible. If it turns out that the CA markings/extensions are an issue, we can loosen the requirements.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"d4665b61ae619931f2315508811e5db79dc59b66","unresolved":false,"context_lines":[{"line_number":307,"context_line":"        signed_certificate \u003d self._signed_cert"},{"line_number":308,"context_line":"        certificate_chain \u003d [signed_certificate]"},{"line_number":309,"context_line":""},{"line_number":310,"context_line":"        while True:"},{"line_number":311,"context_line":"            signing_certificate \u003d None"},{"line_number":312,"context_line":""},{"line_number":313,"context_line":"            # Identify potential signing certificates by issuer matching."}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_4fe90d14","line":310,"updated":"2016-06-21 14:26:46.000000000","message":"Not a fan of while True loops in general. Isn\u0027t this equivalent to \n\nwhile (signed_certificate !\u003d signing_certificate):","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"b4abc80b7f2cad62bf3a318812547026b5a516c1","unresolved":false,"context_lines":[{"line_number":307,"context_line":"        signed_certificate \u003d self._signed_cert"},{"line_number":308,"context_line":"        certificate_chain \u003d [signed_certificate]"},{"line_number":309,"context_line":""},{"line_number":310,"context_line":"        while True:"},{"line_number":311,"context_line":"            signing_certificate \u003d None"},{"line_number":312,"context_line":""},{"line_number":313,"context_line":"            # Identify potential signing certificates by issuer matching."}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_1a647a20","line":310,"in_reply_to":"5a9d85d2_4fe90d14","updated":"2016-06-22 18:38:56.000000000","message":"Yes, but if we switch to not going back to the root (i.e., just a single link chain) we can\u0027t do this comparison.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7012,"name":"Brianna Poulos","email":"Brianna.Poulos@jhuapl.edu","username":"brianna-poulos"},"change_message_id":"91ae43cd5e1c653dd4cb17f12ac4085d24f81c01","unresolved":false,"context_lines":[{"line_number":607,"context_line":"    return certificate"},{"line_number":608,"context_line":""},{"line_number":609,"context_line":""},{"line_number":610,"context_line":"def verify_certificate(certificate):"},{"line_number":611,"context_line":"    \"\"\"Verify that the certificate is rooted in the trust store."},{"line_number":612,"context_line":""},{"line_number":613,"context_line":"    Load all certificates found in the trust store and store them in a"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_bd86eb10","line":610,"updated":"2016-06-21 15:03:23.000000000","message":"Would it make sense to group all of the methods related to certificate verification in one place?  It seems a little counter-intuitive to have the verify_certificates at the end of the file, but all of the helper methods at the beginning.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"b4abc80b7f2cad62bf3a318812547026b5a516c1","unresolved":false,"context_lines":[{"line_number":607,"context_line":"    return certificate"},{"line_number":608,"context_line":""},{"line_number":609,"context_line":""},{"line_number":610,"context_line":"def verify_certificate(certificate):"},{"line_number":611,"context_line":"    \"\"\"Verify that the certificate is rooted in the trust store."},{"line_number":612,"context_line":""},{"line_number":613,"context_line":"    Load all certificates found in the trust store and store them in a"}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_ba7d66b3","line":610,"in_reply_to":"5a9d85d2_bd86eb10","updated":"2016-06-22 18:38:56.000000000","message":"Makes sense. I\u0027ll move it.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7012,"name":"Brianna Poulos","email":"Brianna.Poulos@jhuapl.edu","username":"brianna-poulos"},"change_message_id":"91ae43cd5e1c653dd4cb17f12ac4085d24f81c01","unresolved":false,"context_lines":[{"line_number":620,"context_line":"    \"\"\""},{"line_number":621,"context_line":"    trusted_certificates \u003d load_certificates_from_trust_store("},{"line_number":622,"context_line":"        CONF.glance.certificate_trust_store"},{"line_number":623,"context_line":"    )"},{"line_number":624,"context_line":"    context \u003d CertificateVerificationContext(trusted_certificates)"},{"line_number":625,"context_line":""},{"line_number":626,"context_line":"    context.update(certificate)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a9d85d2_7d1c8377","line":623,"updated":"2016-06-21 15:03:23.000000000","message":"It\u0027s possible that trusted_certificates could be empty.  Should that be checked here so that it can be logged in a way that makes this possible error easier to debug?","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"b4abc80b7f2cad62bf3a318812547026b5a516c1","unresolved":false,"context_lines":[{"line_number":620,"context_line":"    \"\"\""},{"line_number":621,"context_line":"    trusted_certificates \u003d load_certificates_from_trust_store("},{"line_number":622,"context_line":"        CONF.glance.certificate_trust_store"},{"line_number":623,"context_line":"    )"},{"line_number":624,"context_line":"    context \u003d CertificateVerificationContext(trusted_certificates)"},{"line_number":625,"context_line":""},{"line_number":626,"context_line":"    context.update(certificate)"}],"source_content_type":"text/x-python","patch_set":1,"id":"3aaa91ec_9a5fca40","line":623,"in_reply_to":"5a9d85d2_7d1c8377","updated":"2016-06-22 18:38:56.000000000","message":"Yes! I\u0027ll do that too.","commit_id":"2fe1b7f46a076cc6de58804b42914b6de83520ec"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"38cc788cc4b76fa31dc1a957edd76dc7e3820cfa","unresolved":false,"context_lines":[{"line_number":254,"context_line":""},{"line_number":255,"context_line":""},{"line_number":256,"context_line":"class CertificateVerificationContext(object):"},{"line_number":257,"context_line":"    \"\"\""},{"line_number":258,"context_line":"    A collection of signing certificates that may be used to verify the"},{"line_number":259,"context_line":"    signatures of other certificates."},{"line_number":260,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":2,"id":"3aaa91ec_b1f6e8f6","line":257,"updated":"2016-06-23 19:06:02.000000000","message":"Pep8 error: no newline at the beginning of a docstring.","commit_id":"3003772b31fadd31d34cd2f6947e9afe8a1bb0eb"},{"author":{"_account_id":7012,"name":"Brianna Poulos","email":"Brianna.Poulos@jhuapl.edu","username":"brianna-poulos"},"change_message_id":"ab951e1e3cbc628bf986540e1be9796c5d6a3627","unresolved":false,"context_lines":[{"line_number":271,"context_line":"            if not is_within_valid_dates(certificate):"},{"line_number":272,"context_line":"                LOG.warning("},{"line_number":273,"context_line":"                    \"The \u0027%s\u0027 certificate is outside its valid date range \""},{"line_number":274,"context_line":"                    \"and cannot be used as a signing certificate.\" % name"},{"line_number":275,"context_line":"                )"},{"line_number":276,"context_line":"                continue"},{"line_number":277,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3aaa91ec_6d8aea1c","line":274,"updated":"2016-06-23 21:53:03.000000000","message":"It\u0027s possible that this name is \u0027\u0027 (or whatever default is decided).  I think using the path is better for logging warnings (possibly in addition to the name) since it makes the certificate easy to find.","commit_id":"3003772b31fadd31d34cd2f6947e9afe8a1bb0eb"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"38cc788cc4b76fa31dc1a957edd76dc7e3820cfa","unresolved":false,"context_lines":[{"line_number":271,"context_line":"            if not is_within_valid_dates(certificate):"},{"line_number":272,"context_line":"                LOG.warning("},{"line_number":273,"context_line":"                    \"The \u0027%s\u0027 certificate is outside its valid date range \""},{"line_number":274,"context_line":"                    \"and cannot be used as a signing certificate.\" % name"},{"line_number":275,"context_line":"                )"},{"line_number":276,"context_line":"                continue"},{"line_number":277,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3aaa91ec_713e408d","line":274,"updated":"2016-06-23 19:06:02.000000000","message":"This string needs a _LW formatting call.","commit_id":"3003772b31fadd31d34cd2f6947e9afe8a1bb0eb"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"38cc788cc4b76fa31dc1a957edd76dc7e3820cfa","unresolved":false,"context_lines":[{"line_number":280,"context_line":"            else:"},{"line_number":281,"context_line":"                LOG.warning(_LW("},{"line_number":282,"context_line":"                    \"The \u0027%s\u0027 certificate is not configured to act as a \""},{"line_number":283,"context_line":"                    \"signing certificate.\" % name"},{"line_number":284,"context_line":"                ))"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"        self._signed_certificate \u003d None"}],"source_content_type":"text/x-python","patch_set":2,"id":"3aaa91ec_d1b29405","line":283,"updated":"2016-06-23 19:06:02.000000000","message":"I think these calls go like:\n\n Log.warning(_LW(\"Some string: %s\"), some_str)\n\nor else pep8 gets mad.","commit_id":"3003772b31fadd31d34cd2f6947e9afe8a1bb0eb"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"6ef8770657621f0b1f33354fa2e82847d1d1ce77","unresolved":false,"context_lines":[{"line_number":37,"context_line":"from nova.i18n import _, _LE, _LW"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":40,"context_line":"CONF \u003d nova.conf.CONF"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":""},{"line_number":43,"context_line":"HASH_METHODS \u003d {"}],"source_content_type":"text/x-python","patch_set":6,"id":"1aa78d24_b5443774","line":40,"updated":"2016-07-11 12:58:40.000000000","message":"For the purposes of resolving https://bugs.launchpad.net/nova/+bug/1528349 more easily, we shouldn\u0027t be directly referencing the nova config at all within the signature_utils module.","commit_id":"0553be3e227393e9990d50af53721c3e80451bb0"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"6ef8770657621f0b1f33354fa2e82847d1d1ce77","unresolved":false,"context_lines":[{"line_number":229,"context_line":"    :raises: SignatureVerificationError if the certificate verification fails"},{"line_number":230,"context_line":"            for any reason."},{"line_number":231,"context_line":"    \"\"\""},{"line_number":232,"context_line":"    path \u003d CONF.glance.certificate_trust_store"},{"line_number":233,"context_line":"    if path is None or path \u003d\u003d \u0027\u0027:"},{"line_number":234,"context_line":"        trusted_certificates \u003d []"},{"line_number":235,"context_line":"    else:"}],"source_content_type":"text/x-python","patch_set":6,"id":"1aa78d24_b91b911b","line":232,"updated":"2016-07-11 12:58:40.000000000","message":"The trust store path should be a parameter to this function","commit_id":"0553be3e227393e9990d50af53721c3e80451bb0"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"2e847f2a0e20b4ba99a00ae863f1083156244fc4","unresolved":false,"context_lines":[{"line_number":218,"context_line":"    verifier.verify()"},{"line_number":219,"context_line":""},{"line_number":220,"context_line":""},{"line_number":221,"context_line":"def verify_certificate(certificate):"},{"line_number":222,"context_line":"    \"\"\"Verify that the certificate is rooted in the trust store."},{"line_number":223,"context_line":""},{"line_number":224,"context_line":"    Load all certificates found in the trust store and store them in a"}],"source_content_type":"text/x-python","patch_set":7,"id":"1aa78d24_a259df5d","line":221,"updated":"2016-07-14 13:29:42.000000000","message":"Can\u0027t we just wait till we have the new library to do all this work?","commit_id":"f546c9902740e3745d840b31ebb6b436ae10ddb6"}],"nova/tests/unit/test_signature_utils.py":[{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"869a2bc065c2622f0629a31aa1381a6adb8518fc","unresolved":false,"context_lines":[{"line_number":283,"context_line":"    @mock.patch(\u0027nova.signature_utils.verify_certificate\u0027)"},{"line_number":284,"context_line":"    def test_get_public_key_rsa(self, mock_verify_cert, mock_get_cert,"},{"line_number":285,"context_line":"                                mock_conf):"},{"line_number":286,"context_line":"        mock_conf.glance.certificate_trust_store \u003d self.temp_dir"},{"line_number":287,"context_line":"        fake_cert \u003d FakeCryptoCertificate()"},{"line_number":288,"context_line":"        mock_get_cert.return_value \u003d fake_cert"},{"line_number":289,"context_line":"        sig_key_type \u003d signature_utils.SignatureKeyType.lookup("}],"source_content_type":"text/x-python","patch_set":4,"id":"1aa78d24_62484d88","line":286,"updated":"2016-07-05 19:36:31.000000000","message":"Instead of setting the mock conf in each test, set the global conf value in TestCase constructor using self.flags() like so:\n\n    self.flags(certificate_trust_store\u003d\u0027fake_path\u0027, group\u003d\u0027glance\u0027)","commit_id":"a6f09001a3d3cd08447dd22882bf7a6c6435821a"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"11a8a5f134c5a1a4f211a918ccd2b90aa1a76bbf","unresolved":false,"context_lines":[{"line_number":283,"context_line":"    @mock.patch(\u0027nova.signature_utils.verify_certificate\u0027)"},{"line_number":284,"context_line":"    def test_get_public_key_rsa(self, mock_verify_cert, mock_get_cert,"},{"line_number":285,"context_line":"                                mock_conf):"},{"line_number":286,"context_line":"        mock_conf.glance.certificate_trust_store \u003d self.temp_dir"},{"line_number":287,"context_line":"        fake_cert \u003d FakeCryptoCertificate()"},{"line_number":288,"context_line":"        mock_get_cert.return_value \u003d fake_cert"},{"line_number":289,"context_line":"        sig_key_type \u003d signature_utils.SignatureKeyType.lookup("}],"source_content_type":"text/x-python","patch_set":4,"id":"1aa78d24_eb434811","line":286,"in_reply_to":"1aa78d24_62484d88","updated":"2016-07-06 17:10:53.000000000","message":"Neat, that\u0027s definitely a better approach. Thanks!","commit_id":"a6f09001a3d3cd08447dd22882bf7a6c6435821a"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"869a2bc065c2622f0629a31aa1381a6adb8518fc","unresolved":false,"context_lines":[{"line_number":389,"context_line":"        super(TestCertificateValidation, self).tearDown()"},{"line_number":390,"context_line":""},{"line_number":391,"context_line":"    def test_load_certificate(self):"},{"line_number":392,"context_line":"        # Test loading a PEM-encoded certificate"},{"line_number":393,"context_line":"        cert \u003d signature_utils.load_certificate("},{"line_number":394,"context_line":"            os.path.join(self.temp_dir, \u0027self_signed_cert.pem\u0027)"},{"line_number":395,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":4,"id":"1aa78d24_0557d3ce","line":392,"updated":"2016-07-05 19:36:31.000000000","message":"For these longer tests w/ multiple test cases, I\u0027d recommend breaking the test cases out into separate tests. Short tests w/ specific titles are very helpful for debugging Nova unit test failures and stylistically seems to be the standard here.","commit_id":"a6f09001a3d3cd08447dd22882bf7a6c6435821a"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"11a8a5f134c5a1a4f211a918ccd2b90aa1a76bbf","unresolved":false,"context_lines":[{"line_number":389,"context_line":"        super(TestCertificateValidation, self).tearDown()"},{"line_number":390,"context_line":""},{"line_number":391,"context_line":"    def test_load_certificate(self):"},{"line_number":392,"context_line":"        # Test loading a PEM-encoded certificate"},{"line_number":393,"context_line":"        cert \u003d signature_utils.load_certificate("},{"line_number":394,"context_line":"            os.path.join(self.temp_dir, \u0027self_signed_cert.pem\u0027)"},{"line_number":395,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":4,"id":"1aa78d24_4b741c67","line":392,"in_reply_to":"1aa78d24_0557d3ce","updated":"2016-07-06 17:10:53.000000000","message":"Agreed. I realized this yesterday when modifying the original TestSignatureUtils test suite. I\u0027ll break these up.","commit_id":"a6f09001a3d3cd08447dd22882bf7a6c6435821a"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"869a2bc065c2622f0629a31aa1381a6adb8518fc","unresolved":false,"context_lines":[{"line_number":399,"context_line":"        cert \u003d signature_utils.load_certificate("},{"line_number":400,"context_line":"            os.path.join(self.temp_dir, \u0027self_signed_cert.der\u0027)"},{"line_number":401,"context_line":"        )"},{"line_number":402,"context_line":"        self.assertIsInstance(cert, x509.Certificate)"},{"line_number":403,"context_line":""},{"line_number":404,"context_line":"        # Test loading a non-certificate file"},{"line_number":405,"context_line":"        path \u003d os.path.join(self.cert_path, \u0027not_a_cert.txt\u0027)"}],"source_content_type":"text/x-python","patch_set":4,"id":"1aa78d24_4500bb01","line":402,"updated":"2016-07-05 19:36:31.000000000","message":"Is there any way to check the encoding given the x509.Certificate object?","commit_id":"a6f09001a3d3cd08447dd22882bf7a6c6435821a"},{"author":{"_account_id":7764,"name":"Peter Hamilton","email":"peter.hamilton@jhuapl.edu","username":"Peter"},"change_message_id":"11a8a5f134c5a1a4f211a918ccd2b90aa1a76bbf","unresolved":false,"context_lines":[{"line_number":399,"context_line":"        cert \u003d signature_utils.load_certificate("},{"line_number":400,"context_line":"            os.path.join(self.temp_dir, \u0027self_signed_cert.der\u0027)"},{"line_number":401,"context_line":"        )"},{"line_number":402,"context_line":"        self.assertIsInstance(cert, x509.Certificate)"},{"line_number":403,"context_line":""},{"line_number":404,"context_line":"        # Test loading a non-certificate file"},{"line_number":405,"context_line":"        path \u003d os.path.join(self.cert_path, \u0027not_a_cert.txt\u0027)"}],"source_content_type":"text/x-python","patch_set":4,"id":"1aa78d24_0bfcd470","line":402,"in_reply_to":"1aa78d24_4500bb01","updated":"2016-07-06 17:10:53.000000000","message":"I don\u0027t think so. The object has a public_bytes method that you can use to get the DER or PEM encoding version of the certificate. I believe once it\u0027s loaded into an x509.Certificate, it\u0027s agnostic to the original encoding.","commit_id":"a6f09001a3d3cd08447dd22882bf7a6c6435821a"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"1c44326320a574e57312df6808027d9d5be74c9b","unresolved":false,"context_lines":[{"line_number":680,"context_line":"        context.update(cert)"},{"line_number":681,"context_line":"        self.assertEqual(cert, context._signed_certificate)"},{"line_number":682,"context_line":""},{"line_number":683,"context_line":"    @mock.patch(\u0027nova.signature_utils.timeutils.utcnow\u0027)"},{"line_number":684,"context_line":"    def test_context_update_with_date_invalid_certificate(self, mock_utcnow):"},{"line_number":685,"context_line":"        # Test updating the context with an out-of-date certificate"},{"line_number":686,"context_line":"        cert_tuples \u003d signature_utils.load_certificates_from_trust_store("}],"source_content_type":"text/x-python","patch_set":5,"id":"1aa78d24_fdc01c52","line":683,"updated":"2016-07-06 19:28:39.000000000","message":"Should timeutils be under the signature_utils module? Shouldn\u0027t we be mocking out oslo_utils.timeutils.utcnow? I think this may be the source of your py34 error.","commit_id":"9331b92aa941208d8a2aa67330e4cdf30b571549"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"6ef8770657621f0b1f33354fa2e82847d1d1ce77","unresolved":false,"context_lines":[{"line_number":452,"context_line":"            \u0027invalid-path\u0027"},{"line_number":453,"context_line":"        )"},{"line_number":454,"context_line":""},{"line_number":455,"context_line":"    @mock.patch(\u0027nova.signature_utils.timeutils.utcnow\u0027)"},{"line_number":456,"context_line":"    def test_is_within_valid_dates(self, mock_utcnow):"},{"line_number":457,"context_line":"        # Verify a certificate is valid at a time within its valid date range"},{"line_number":458,"context_line":"        cert \u003d signature_utils.load_certificate("}],"source_content_type":"text/x-python","patch_set":6,"id":"1aa78d24_b93c31c0","line":455,"updated":"2016-07-11 12:58:40.000000000","message":"Still don\u0027t understand why this mock is under the signature_utils module and not oslo_utils module, since that\u0027s where timeutils.utcnow is imported from.","commit_id":"0553be3e227393e9990d50af53721c3e80451bb0"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"6ef8770657621f0b1f33354fa2e82847d1d1ce77","unresolved":false,"context_lines":[{"line_number":589,"context_line":"    @mock.patch(\u0027nova.signature_utils.CONF\u0027)"},{"line_number":590,"context_line":"    def test_verify_valid_certificate_with_empty_trust_store(self, mock_conf):"},{"line_number":591,"context_line":"        # Test verifying a valid certificate against an empty trust store"},{"line_number":592,"context_line":"        mock_conf.glance.certificate_trust_store \u003d \u0027\u0027"},{"line_number":593,"context_line":"        cert \u003d signature_utils.load_certificate("},{"line_number":594,"context_line":"            os.path.join(self.cert_path, \u0027signed_cert.pem\u0027)"},{"line_number":595,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":6,"id":"1aa78d24_392e81fa","line":592,"updated":"2016-07-11 12:58:40.000000000","message":"Use self.flags to flip config options if you have to. However, as the other comments note, the signature_utils class shouldn\u0027t be referencing the nova conf directly anyway. The trust store path should be grabbed from the config by the GlanceClient during download and passed to the signature_utils module.","commit_id":"0553be3e227393e9990d50af53721c3e80451bb0"},{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"3aa3eec1d4c7f73f3747f2e4197f31a570d5fbea","unresolved":false,"context_lines":[{"line_number":589,"context_line":"    @mock.patch(\u0027nova.signature_utils.CONF\u0027)"},{"line_number":590,"context_line":"    def test_verify_valid_certificate_with_empty_trust_store(self, mock_conf):"},{"line_number":591,"context_line":"        # Test verifying a valid certificate against an empty trust store"},{"line_number":592,"context_line":"        mock_conf.glance.certificate_trust_store \u003d \u0027\u0027"},{"line_number":593,"context_line":"        cert \u003d signature_utils.load_certificate("},{"line_number":594,"context_line":"            os.path.join(self.cert_path, \u0027signed_cert.pem\u0027)"},{"line_number":595,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":7,"id":"1aa78d24_a5ebe9a2","line":592,"updated":"2016-07-13 13:16:59.000000000","message":"I think you\u0027d have to call self.flags both to set and unset the path so this is equally verbose. I\u0027m ok with it.","commit_id":"f546c9902740e3745d840b31ebb6b436ae10ddb6"}]}