lints: ballot 144/201 .onion linting. #263
Conversation
Adds a lint (`lint_san_dns_name_onion_not_ev_cert.go`) that validates that any subscriber certificates containing a `.onion` subject that were issued after May 1st, 2015 are EV certificates. Any non-EV certs issued after this date that contain a `.onion` subject should receive an `Error` lint result.
Adds a lint (`lint_ext_tor_service_descriptor.go`) that validates that `.onion` subjects have the correct `cabf-TorServiceDescriptor` extension with a well formed `TorServiceDescriptorHash` object for each `.onion` subject.
Adds a lint (`lint_onion_subject_validity_time_too_large.go`) that validates certificates with one or more `.onion` subjects do not have a validity period larger than 15 months.
cpu
changed the title
I think for this one lint the effective date should be the 8th of July 2017 instead of May 1st, 2015 based on it being a clarification from Ballot 201. I'll try to clarify and update shortly. |
cpu
changed the title
|
Resolves #14 |
| if !util.IsSubscriberCert(c) { | ||
| return false | ||
| } | ||
| names := append(c.DNSNames, c.Subject.CommonName) |
There was a problem hiding this comment.
We might want to move this into a util helper since it's already in multiple lints and could plausibly be reused in the future.
| // and parses a TorServiceDescriptorHash using the data contained in the | ||
| // sequence. The TorServiceDescriptorHash object and the remaining data are | ||
| // returned if no error occurs. | ||
| func parseTorServiceDescriptorHash(data []byte) (*TorServiceDescriptorHash, []byte, error) { |
There was a problem hiding this comment.
I suspect we want this parsing to live in ZCrypto (https://github.com/zmap/zcrypto) and be an exposed part of a certificate rather than parsed out here as part of the lint. That way when we parse certificates in other places, we have this data available. Thoughts?
There was a problem hiding this comment.
That makes sense to me 👍 I'll take a crack at pulling out a separate zcrypto PR.
There was a problem hiding this comment.
I haven't had a chance to start on dissecting this PR but I think my rough plan of attack will be:
- Closing this PR.
- Opening a new PR against
zlintthat addslint_onion_subject_validity_time_too_large.goandlint_san_dns_name_onion_not_ev_cert.gosince these can land right away. - Opening a PR against
zcryptoto add theTorServiceDescriptorHashparsing code - Working through review of that branch until it lands.
- Opening a final PR against
zlintthat adds an updatedlint_ext_tor_service_descriptor_hash_invalid.gothat lints without any ASN.1 parsing logic.
@zakird Is it fair to assume that since zlint doesn't pin/vendor its dependencies that writing a lint that only builds against zcrypto master is copacetic?
| Description: fmt.Sprintf( | ||
| "certificates with .onion names can not be valid for more than %d months", | ||
| maxOnionValidityMonths), | ||
| // TODO(@cpu): Cite section of BRs instead of ballot? |
There was a problem hiding this comment.
Agree, we should cite the finalized BR not the ballot.
|
Yes I think that's fine. Long term we should probably pin.
…On Tue, Mar 5, 2019 at 11:19 AM Daniel McCarney ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lints/lint_ext_tor_service_descriptor_hash_invalid.go
<#263 (comment)>:
> + return fmt.Errorf("invalid TorServiceDescriptorHash subjectPublicKeyHash, "+
+ "alg is SHA384 but bit length is %d not %d",
+ t.HashBits, 384)
+ } else if t.Alg.Algorithm.Equal(util.SHA512OID) && t.HashBits != 512 {
+ return fmt.Errorf("invalid TorServiceDescriptorHash subjectPublicKeyHash, "+
+ "alg is SHA512 but bit length is %d not %d",
+ t.HashBits, 512)
+ }
+ return nil
+}
+
+// parseTorServiceDescriptorHash unmarshals a SEQUENCE from the provided data
+// and parses a TorServiceDescriptorHash using the data contained in the
+// sequence. The TorServiceDescriptorHash object and the remaining data are
+// returned if no error occurs.
+func parseTorServiceDescriptorHash(data []byte) (*TorServiceDescriptorHash, []byte, error) {
I haven't had a chance to start on dissecting this PR but I think my rough
plan of attack will be:
1. Closing this PR.
2. Opening a new PR against zlint that adds
lint_onion_subject_validity_time_too_large.go and
lint_san_dns_name_onion_not_ev_cert.go since these can land right away.
3. Opening a PR against zcrypto to add the TorServiceDescriptorHash
parsing code
4. Working through review of that branch until it lands.
5. Opening a final PR against zlint that adds an updated
lint_ext_tor_service_descriptor_hash_invalid.go that lints without any
ASN.1 parsing logic.
@zakird <https://github.com/zakird> Is it fair to assume that since zlint
doesn't pin/vendor its dependencies that writing a lint that only builds
against zcrypto master is copacetic?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#263 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAMSUA4bCYdvLJi9ic4ggcYBRQmAJn1oks5vTsM-gaJpZM4baoiF>
.
|
👍 #264 |
|
Closing this PR based on my plan to split it up. |
This branch adds three new lints based on CABF Ballot 144 and CABF Ballot 201 specific to the allowance of
.onionsubject names under several additional requirements. Presently all of the new lints are made to be effective as of May 1st, 2015 but that may need to be tweaked.lint_san_dns_name_onion_not_ev_cert.govalidates that any subscriber certificates containing a.onionsubject are EV certificates.lint_ext_tor_service_descriptor.govalidates that certificates with one or more.onionsubjects have the correctcabf-TorServiceDescriptorextension, and a well formedTorServiceDescriptorHashobject for each.onionsubject.lint_onion_subject_validity_time_too_large.govalidates certificates with one or more.onionsubjects do not have a validity period larger than 15 months.These linters will hopefully help catch miss-issuance events like this one by DigiCert in March 2018.
I'd appreciate some extra 👀 on my parsing of the
TorServiceDescriptorHashASN.1. As far as I know there aren't any other open source projects that have implemented this so I wasn't able to spot check against someone else's code.AWSLabs/Certlinthas not yet implemented this linting at the time of writing.Resolves #14