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.

Sign up for GitHub

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

Jump to bottom

Fix: new cert issuing is incorrectly delayed #998

Merged
merged 4 commits into from
Jun 4, 2021
Merged

Fix: new cert issuing is incorrectly delayed #998

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3d36f5c
fix new cert issuing is incorrectly delayed
May 11, 2021
796e2ab
apply lint
May 11, 2021
1b6d6c4
revert cert duration & write cert issue/revoke info into log
May 12, 2021
7483569
apply lint
May 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion transport/internet/tls/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func isCertificateExpired(c *tls.Certificate) bool {
}

// If leaf is not there, the certificate is probably not used yet. We trust user to provide a valid certificate.
return c.Leaf != nil && c.Leaf.NotAfter.Before(time.Now().Add(-time.Minute))
return c.Leaf != nil && c.Leaf.NotAfter.Before(time.Now().Add(time.Minute*2))
Copy link
Contributor

Choose a reason for hiding this comment

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

这里感觉没有问题，修复了之前的一个bug。

}

func issueCertificate(rawCA *Certificate, domain string) (*tls.Certificate, error) {
Expand Down Expand Up @@ -122,6 +122,9 @@ func getGetCertificateFunc(c *tls.Config, ca []*Certificate) func(hello *tls.Cli
cert := certificate
if !isCertificateExpired(&cert) {
newCerts = append(newCerts, cert)
} else if cert.Leaf != nil {
expTime := cert.Leaf.NotAfter.Format(time.RFC3339)
newError("old certificate for ", domain, " (expire on ", expTime, ") revoked").AtInfo().WriteToLog()
Copy link
Contributor

Choose a reason for hiding this comment

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

Revoke 在CA语境下有一个特别的含义。在截至日期前失去信任才是 Revoke，现在这个到截止日期了才删除，我合并后换一个单词吧。 https://en.wikipedia.org/wiki/Certificate_revocation_list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

现在正是在证书Expire之前2分钟就将证书废止，所以我才用了Revoke一词。其实真正需要更改的是isCertificateExpired()这个函数名，它的代码逻辑是判断“证书是否需要更换”而非“证书是否过期”。如果您觉得需要对用词进行修正，我也可以把它改一下。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我仔细考虑了一下，由于CRL的缺失，这个废止仅在服务端生效，客户端在证书到期前其实依旧能够继续使用，因此revoke一词确实不够准确，或许换成discard更合适。

Copy link
Contributor

Choose a reason for hiding this comment

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

嗯，总之我合并的时候会改一下的。

}
}

Expand All @@ -139,6 +142,14 @@ func getGetCertificateFunc(c *tls.Config, ca []*Certificate) func(hello *tls.Cli
newError("failed to issue new certificate for ", domain).Base(err).WriteToLog()
continue
}
parsed, err := x509.ParseCertificate(newCert.Certificate[0])
if err == nil {
newCert.Leaf = parsed
expTime := parsed.NotAfter.Format(time.RFC3339)
newError("new certificate for ", domain, " (expire on ", expTime, ") issued").AtInfo().WriteToLog()
} else {
newError("failed to parse new certificate for ", domain).Base(err).WriteToLog()
}

access.Lock()
c.Certificates = append(c.Certificates, *newCert)
Expand Down