Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix eku check for BRs #171

Merged
merged 3 commits into from
Sep 6, 2017
Merged

Fix eku check for BRs #171

merged 3 commits into from
Sep 6, 2017

Conversation

kumarde
Copy link

@kumarde kumarde commented Sep 6, 2017

Addresses #167

Copy link
Member

@dadrian dadrian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % nits

util/eku.go Outdated
@@ -0,0 +1,17 @@
package util
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May as well just make this part of ca.go. https://github.com/zmap/zlint/blob/master/util/ca.go

@@ -97,6 +98,9 @@ func (l *Lint) CheckEffective(c *x509.Certificate) bool {
// CheckEffective()
// Execute()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the first paragraph of the above comment to describe the additional checks you just added, e.g. lints that are checking for BR compliance are only ran against certs that fall under the purview of the BR's.

@zakird
Copy link
Member

zakird commented Sep 6, 2017

Does this work for CA certificates?

@zakird zakird requested a review from titanous September 6, 2017 17:55
@zakird
Copy link
Member

zakird commented Sep 6, 2017

@titanous can you take a look at this too?

util/ca.go Outdated
@@ -29,3 +29,15 @@ func IsSelfSigned(c *x509.Certificate) bool {
func IsSubscriberCert(c *x509.Certificate) bool {
return !IsCACert(c) && !IsSelfSigned(c)
}

func IsTestableBRCertificate(cert *x509.Certificate) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this should be renamed? It checks if a certificate can be used for ServerAuth, so IsServerAuthCert or similar?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that.

@kumarde
Copy link
Author

kumarde commented Sep 6, 2017

@titanous i've updated the name.

@kumarde kumarde merged commit f4040da into master Sep 6, 2017
@kumarde kumarde deleted the kumarde/check_eku_for_brs branch September 6, 2017 19:25
aaomidi pushed a commit to aaomidi/zlint that referenced this pull request Nov 29, 2022
aaomidi pushed a commit to aaomidi/zlint that referenced this pull request Nov 29, 2022
aaomidi pushed a commit to aaomidi/zlint that referenced this pull request Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants