Fix: new cert issuing is incorrectly delayed #998
Conversation
xiaokangwang
added
the
Extensive Review Required
common/protocol/tls/cert/cert.go
Outdated
| @@ -142,7 +142,7 @@ func Generate(parent *Certificate, opts ...Option) (*Certificate, error) { | |||
| template := &x509.Certificate{ | |||
| SerialNumber: serialNumber, | |||
| NotBefore: time.Now().Add(time.Hour * -1), | |||
| NotAfter: time.Now().Add(time.Hour), | |||
| NotAfter: time.Now().Add(time.Hour + time.Minute*2), | |||
There was a problem hiding this comment.
这里签发的证书可以被攻击者探测到，或者当客户端的TLS版本为1.2（以及之前）时可以直接被抓取到。
62 分钟有效期太可丁可卯了，感觉会导致被攻击者检测出来服务器的类型。
There was a problem hiding this comment.
增加有效期的理由在1楼说了，在大量连接的情况下，62分钟的有效期并提前2分钟生成新证书，证书生成间隔为约整1小时，便于从日志中找出相关问题。这也跟实际使用的证书的情况类似（实际证书有效期一般会比目标有效期长一点）。
至于增加特征，的确有可能，主要原因还是60分钟的证书就很罕见，导致62分钟的就更特殊了。
如果这个改动您觉得不妥，我可以撤销。但为了从日志中找出相关问题，可否在证书过期和生成新证书时输出一些日志信息，您觉得如何？
| @@ -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)) | |||
There was a problem hiding this comment.
这里感觉没有问题，修复了之前的一个bug。
我就是趁着 |
这个功能本来是用来在客户端进行MITM的，这种动态签发在互联网上传送证书的用法并不是重点受到支持的算法。62分钟的部分我想了想还是觉得保持原有的60分钟比较好，这样也是个整数的时间，62一看就是V2Ray了 这种自建CA的检测还是挺明显的，因为攻击者可以自己连接，如果直接就签发了对应的证书，然后再看连接上之后的行为，基本上就可以确定是V2了，总感觉不是推荐的用法。 这个功能是用于MITM的，其他的用法的话感觉要慎重。 |
|
好的，稍后commit将恢复60分钟有效期，并增加相应日志输出。 |
|
嗯，有效期这个需要再多考虑考虑。提前1分钟签发的bug可以直接改掉。 |
| @@ -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() | |||
There was a problem hiding this comment.
Revoke 在CA语境下有一个特别的含义。在截至日期前失去信任才是 Revoke，现在这个到截止日期了才删除，我合并后换一个单词吧。 https://en.wikipedia.org/wiki/Certificate_revocation_list
There was a problem hiding this comment.
现在正是在证书Expire之前2分钟就将证书废止，所以我才用了Revoke一词。其实真正需要更改的是isCertificateExpired()这个函数名，它的代码逻辑是判断“证书是否需要更换”而非“证书是否过期”。如果您觉得需要对用词进行修正，我也可以把它改一下。
There was a problem hiding this comment.
我仔细考虑了一下，由于CRL的缺失，这个废止仅在服务端生效，客户端在证书到期前其实依旧能够继续使用，因此revoke一词确实不够准确，或许换成discard更合适。
* fix new cert issuing is incorrectly delayed * apply lint * revert cert duration & write cert issue/revoke info into log * apply lint Co-authored-by: Bhoppi Chaw <bhoppi#outlook,com>
Fix #997
P.S. 因为VMess协议允许Server/Client的时间误差为90s，因此为了保证在该时间误差下生成的证书依旧可用，我将生成证书的提前时间从1分钟增加为2分钟。同时将证书的有效期也延长为1小时2分钟，这样在大量连接的情况下，证书生成间隔为约整1小时，便于从日志中找出相关问题。