{"action":"created","issue":{"url":"https://api.github.com/repos/globalsign/certlint/issues/13","repository_url":"https://api.github.com/repos/globalsign/certlint","labels_url":"https://api.github.com/repos/globalsign/certlint/issues/13/labels{/name}","comments_url":"https://api.github.com/repos/globalsign/certlint/issues/13/comments","events_url":"https://api.github.com/repos/globalsign/certlint/issues/13/events","html_url":"https://github.com/globalsign/certlint/pull/13","id":308135082,"number":13,"title":"extensions checks: Add OCSP must staple check.","user":{"login":"cpu","id":292650,"avatar_url":"https://avatars1.githubusercontent.com/u/292650?v=4","gravatar_id":"","url":"https://api.github.com/users/cpu","html_url":"https://github.com/cpu","followers_url":"https://api.github.com/users/cpu/followers","following_url":"https://api.github.com/users/cpu/following{/other_user}","gists_url":"https://api.github.com/users/cpu/gists{/gist_id}","starred_url":"https://api.github.com/users/cpu/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cpu/subscriptions","organizations_url":"https://api.github.com/users/cpu/orgs","repos_url":"https://api.github.com/users/cpu/repos","events_url":"https://api.github.com/users/cpu/events{/privacy}","received_events_url":"https://api.github.com/users/cpu/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2018-03-23T18:25:09Z","updated_at":"2018-03-26T07:29:18Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/globalsign/certlint/pulls/13","html_url":"https://github.com/globalsign/certlint/pull/13","diff_url":"https://github.com/globalsign/certlint/pull/13.diff","patch_url":"https://github.com/globalsign/certlint/pull/13.patch"},"body":"This commit adds a check for the OCSP Must Staple x509v3 feature\r\nextension defined in RFC 7633. Prior to this commit running `certlint`\r\non a certificate with this extension produced output like:\r\n\r\n```\r\nBad Certificates Detected: {\r\n  'xxxx': {\r\n    'valid': False,\r\n    'problems': [\r\n      'Certificate contains unknown extension (1.3.6.1.5.5.7.1.24)'\r\n    ]\r\n  }\r\n}\r\n```\r\n\r\nThe OCSP Must Staple check presently enforces that:\r\n1) The `certdata.Data.Type` is not OCSP. The RFC only defines this\r\n   extension for end entity certificates, CSRs, and issuing\r\n   certificates, not OCSP signing certificates.\r\n2) The `ex.Critical` flag is set to false. The RFC forbids setting this\r\n   extension to critical.\r\n\r\nAfter applying this commit the above Bad Certificate Detected problem is\r\nresolved.\r\n\r\n_*Note to reviewers:* I did not find any unit tests for the existing \r\n`certlint/extensions/` checks so I haven't tried to write one for this \r\naddition. I did verify using known good certificates both with/without \r\nthe extension. If there is an example unit test to refer to I can attempt\r\nto add one for the OCSP Must Staple check._"},"comment":{"url":"https://api.github.com/repos/globalsign/certlint/issues/comments/376071559","html_url":"https://github.com/globalsign/certlint/pull/13#issuecomment-376071559","issue_url":"https://api.github.com/repos/globalsign/certlint/issues/13","id":376071559,"user":{"login":"vanbroup","id":722070,"avatar_url":"https://avatars1.githubusercontent.com/u/722070?v=4","gravatar_id":"","url":"https://api.github.com/users/vanbroup","html_url":"https://github.com/vanbroup","followers_url":"https://api.github.com/users/vanbroup/followers","following_url":"https://api.github.com/users/vanbroup/following{/other_user}","gists_url":"https://api.github.com/users/vanbroup/gists{/gist_id}","starred_url":"https://api.github.com/users/vanbroup/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vanbroup/subscriptions","organizations_url":"https://api.github.com/users/vanbroup/orgs","repos_url":"https://api.github.com/users/vanbroup/repos","events_url":"https://api.github.com/users/vanbroup/events{/privacy}","received_events_url":"https://api.github.com/users/vanbroup/received_events","type":"User","site_admin":false},"created_at":"2018-03-26T07:29:18Z","updated_at":"2018-03-26T07:29:18Z","author_association":"OWNER","body":"> For reference I believe this check & the level of scrutiny it applies is fairly consistent with the existing codebase. I based this off of ocspnocheck.go which seems like it would need the same review feedback you gave this work. It does not check the IsCa field of the cert, or its key usage:\r\n\r\nAs added to the review comment, OCSPNoCheck applies only in OCSP signing certificates and not to any end-entity certificates. The type OCSP is based on the extended key usage `x509.ExtKeyUsageOCSPSigning`. If you just check for `IsCA: false` than you directly cover all end-entity certificates.\r\n\r\n> Similarly, though the extension is not supported by Go there is no explicit check of the ASN.1. Many other checks appear to apply the same surface level evaluation I did in this PR (e.g. adobetimestamp.go, pdfrevocation.go, smimecapabilities.go). It would be helpful to external contributors if there were more guidelines available for writing checks & consistency throughout the existing checks.\r\n\r\nI agree that is not completely fair, I think most recent changes have all come with test coverage as I think it's important to work towards a 100% coverage for this package. The test case required for this extension is significantly simple that I feel it should not be much work to add."}}
{"actor":{"display_login":"vanbroup"}}