Skip to content

fix: reapply when domain list changed#340

Merged
usual2970 merged 6 commits intocertimate-go:mainfrom
LeoChen98:fix-reapply-when-domain-list-changed
Nov 18, 2024
Merged

fix: reapply when domain list changed#340
usual2970 merged 6 commits intocertimate-go:mainfrom
LeoChen98:fix-reapply-when-domain-list-changed

Conversation

@LeoChen98
Copy link
Copy Markdown
Contributor

@LeoChen98 LeoChen98 changed the title Fix reapply when domain list changed fix: reapply when domain list changed Nov 13, 2024
@fudiwei
Copy link
Copy Markdown
Member

fudiwei commented Nov 13, 2024

啊,只比较域名这一个参数啊,我以为是想任一参数发生改变都重新申请呢。

现在也挺好,如果能把签名算法的比较也加入进去就更完美了,这俩都是影响证书签发的重要因素 😀。其他的像 DNS Provider 之类的参数虽然对于中途更换了托管商的场景来说也很必要,但相对就比较小众了。

@fudiwei fudiwei added the enhancement New feature or request label Nov 13, 2024
@LeoChen98
Copy link
Copy Markdown
Contributor Author

LeoChen98 commented Nov 13, 2024

啊,只比较域名这一个参数啊,我以为是想任一参数发生改变都重新申请呢。

现在也挺好,如果能把签名算法的比较也加入进去就更完美了,这俩都是影响证书签发的重要因素 😀。其他的像 DNS Provider 之类的参数虽然对于中途更换了托管商的场景来说也很必要,但相对就比较小众了。

签名算法放进去倒也可以,DNS Provider这种的话等到期重新申请的时候应该自然地会跟着新的走?

@fudiwei
Copy link
Copy Markdown
Member

fudiwei commented Nov 13, 2024

签名算法放进去倒也可以,DNS Provider这种的话等到期重新申请的时候应该自然地会跟着新的走?

到期当然会,到期了所有参数都是用最新的了。这个 patch 不就是为了解决还没到期前却修改了参数、想重新触发一次申请么?

@LeoChen98
Copy link
Copy Markdown
Contributor Author

签名算法放进去倒也可以,DNS Provider这种的话等到期重新申请的时候应该自然地会跟着新的走?

到期当然会,到期了所有参数都是用最新的了。这个 patch 不就是为了解决还没到期前却修改了参数、想重新触发一次申请么?

主要是DNS Provider这种只影响申请过程不影响实际证书使用的似乎没有立即重新申请的必要?

Comment thread internal/domains/deploy.go Outdated
Comment on lines +146 to +161
// 遍历域名列表,检查是否都在证书中,找到第一个不存在证书中域名时提前返回false
for _, domain := range strings.Split(domains, ";") {
if !slices.Contains(cert.DNSNames, domain) && !slices.Contains(cert.DNSNames, "*."+removeLastSubdomain(domain)) {
return false
}
}

return true
}

func removeLastSubdomain(domain string) string {
parts := strings.Split(domain, ".")
if len(parts) > 1 {
return strings.Join(parts[1:], ".")
}
return domain
Copy link
Copy Markdown
Member

@fudiwei fudiwei Nov 13, 2024

Choose a reason for hiding this comment

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

这里能明白是判断如果已经签发过的是包含该域名的泛域名证书,也不再重新签发。

但我认为这里比较两个 slices 是否全等即可。理由如下:

  1. 泛域名跟非泛域名证书还是有区别的,用户之所以改了肯定是不想继续保留。
  2. 多域名证书中的多域名顺序也是一个隐式的参数,只有首个 SAN 才会是 CN,它会影响 TLS 握手速度和 Proxy 的缓存方式,证书的最佳实践中都指出过要把最常用的域名放在最前面。所以如果用户真的仅仅是调整了多域名的顺序,虽然域名完全一致,那也应该重新签发一张新的证书。

综上来看只要比较是否全等(内容+顺序均相等),可以用自带的 slices.Equal() 工具方法。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

判断泛域名是否包含主要是考虑重复申请的问题,如果申请入参同时包含同级泛域名和子域名的情况会导致acme报错

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

而且在实际作用中泛域名与单一子域名不冲突,不影响使用,我认为其实没必要更新

Copy link
Copy Markdown
Member

@fudiwei fudiwei Nov 13, 2024

Choose a reason for hiding this comment

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

判断泛域名是否包含主要是考虑重复申请的问题,如果申请入参同时包含同级泛域名和子域名的情况会导致acme报错

那这就应该作为异常抛给用户的,否则用户会以为自己填的参数还挺对。可以引导用户不要这么填,但不应该用户这么填了以后把这种错误隐式给吞掉了。

因为你这里的判断仅仅是“之前申请过的证书还没到期、又想重新触发申请”这个条件下的,可如果用户真的改成了这种“如果申请入参同时包含同级泛域名和子域名的情况会导致acme报错”的错误参数,那么下次到期时自动触发申请它一样还是会报错 —— 与其到那个时候才报错,不如提早就报出来能让用户发现。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

判断泛域名是否包含主要是考虑重复申请的问题,如果申请入参同时包含同级泛域名和子域名的情况会导致acme报错

那这就应该作为异常抛给用户的,否则用户会以为自己填的参数还挺对。可以引导用户不要这么填,但不应该用户这么填了以后把这种错误隐式给吞掉了。

因为你这里的判断仅仅是“之前申请过的证书还没到期、又想重新触发申请”这个条件下的,可如果用户真的改成了这种“如果申请入参同时包含同级泛域名和子域名的情况会导致acme报错”的错误参数,那么下次到期时自动触发申请它一样还是会报错 —— 与其到那个时候才报错,不如提早就报出来能让用户发现。

这种所谓的抛错误动作应该在前端实现而不是后端开始跑才抛,我这里主要考虑的是处理更改后证书还能用但域名在证书内的包含并不完全一致的情况。比如说从*.abc.com改成了a.abc.com;b.abc.com,这种改动就没有必要重新申请,因为证书仍然有效。

换言之,抛错误这个事情是UI/UX该干的,在输入验证的时候就该抛。后端代码这里要保证的是让还能用的证书充分利用以及顺带在前端验证还不完善的情况下将不算严重的入参错误修正让它正常工作。顺便,前端的验证逻辑我不会(躺)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

事实上如果用户明确只要单域名而不是泛域名证书,他应该重新新建实例,included这个变量本身就已经说明了,我的目的只是考虑原有证书是否包含现有列表的所有域名,即,能用。

至于你说的顺序有影响,还是那句话,能用吗?能用。如果你需要一张与原有证书完全不同的证书,也应该新建实例而不是在已有实例上改。

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

事实上如果用户明确只要单域名而不是泛域名证书,他应该重新新建实例,included这个变量本身就已经说明了,我的目的只是考虑原有证书是否包含现有列表的所有域名,即,能用。

至于你说的顺序有影响,还是那句话,能用吗?能用。如果你需要一张与原有证书完全不同的证书,也应该新建实例而不是在已有实例上改。

完全不认同。用户修改后点了保存,但证书真的“修改了吗?”,“如修”。

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

这么精细的判断挺难的,要判断:

  1. 当前的已申请的证书有没有包含新填写的域名
  2. 证书的算法是不是匹配

除了这些甚至还要考虑:

  1. 证书还剩一个月有效期,用户又添加了一个部署,域名没变,这个时候要不重新申请。
  2. 或者刚开始只是测试,捣鼓了一个星期,才有了一个正式的部署配置,这个时候用户应该是想要一个满血的证书的。但是域名没变,要不要重新申请。

所以这些选择后续交给用户。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

事实上如果用户明确只要单域名而不是泛域名证书,他应该重新新建实例,included这个变量本身就已经说明了,我的目的只是考虑原有证书是否包含现有列表的所有域名,即,能用。

至于你说的顺序有影响,还是那句话,能用吗?能用。如果你需要一张与原有证书完全不同的证书,也应该新建实例而不是在已有实例上改。

完全不认同。用户修改后点了保存,但证书真的“修改了吗?”,“如修”。

既然如此,说明我们的理念完全不一致,也没有必要纠结说服对方了,选择权交回给@usual2970 ,你看着办吧。

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

这么精细的判断挺难的,要判断:

  1. 当前的已申请的证书有没有包含新填写的域名
  2. 证书的算法是不是匹配

除了这些甚至还要考虑:

  1. 证书还剩一个月有效期,用户又添加了一个部署,域名没变,这个时候要不重新申请。
  2. 或者刚开始只是测试,捣鼓了一个星期,才有了一个正式的部署配置,这个时候用户应该是想要一个满血的证书的。但是域名没变,要不要重新申请。

所以这些选择后续交给用户。

同意。压根不需要精细判断,用户点了重新申请那就是重新申请。

@fudiwei
Copy link
Copy Markdown
Member

fudiwei commented Nov 13, 2024

主要是DNS Provider这种只影响申请过程不影响实际证书使用的似乎没有立即重新申请的必要?

啊,是我表达的不够清晰吗?😂 我说的就是这个意思

@LeoChen98
Copy link
Copy Markdown
Contributor Author

LeoChen98 commented Nov 13, 2024

主要是DNS Provider这种只影响申请过程不影响实际证书使用的似乎没有立即重新申请的必要?

啊,是我表达的不够清晰吗?😂 我说的就是这个意思

嘛,我这个patch的目的是在修改了域名列表的情况下立即重新申请证书以保证后续部署时不遇到证书与部署域名不匹配的情况,尤其是多个单一域名的变动。例如:

原来是:

a.abc.com
b.abc.com
c.def.com

变成了

a.abc.com
b.abc.com
c.def.com
d.ghi.com

就会出现部署d.ghi.com的时候报错域名与证书不匹配的情况,这是一定要立即生效的配置。

考虑到一些特殊使用场景,加密方式修改也可能会影响证书使用,加进去一起判断也合理。

但是如果修改后列表是修改前列表的完全子集我觉得没有必要立即生效,同级子域名和泛域名同时存在的情况甚至应该在前端进行限制,不然可能会导致报错。

@LeoChen98
Copy link
Copy Markdown
Contributor Author

LeoChen98 commented Nov 13, 2024

@fudiwei 对加密方式修改的判断目前看是做不了,X509解析证书能拿到的加密类型只有

const (
        UnknownPublicKeyAlgorithm PublicKeyAlgorithm = iota
        RSA
        DSA
        ECDSA)

而配置中保存的加密类型有

func parseKeyAlgorithm(algo string) certcrypto.KeyType {
	switch algo {
	case "RSA2048":
		return certcrypto.RSA2048
	case "RSA3072":
		return certcrypto.RSA3072
	case "RSA4096":
		return certcrypto.RSA4096
	case "RSA8192":
		return certcrypto.RSA8192
	case "EC256":
		return certcrypto.EC256
	case "EC384":
		return certcrypto.EC384
	default:
		return certcrypto.RSA2048
	}
}

两边不对称,无法完成对比。

@fudiwei
Copy link
Copy Markdown
Member

fudiwei commented Nov 14, 2024

@fudiwei 对加密方式修改的判断目前看是做不了,X509解析证书能拿到的加密类型只有

const (
        UnknownPublicKeyAlgorithm PublicKeyAlgorithm = iota
        RSA
        DSA
        ECDSA)

而配置中保存的加密类型有

func parseKeyAlgorithm(algo string) certcrypto.KeyType {
	switch algo {
	case "RSA2048":
		return certcrypto.RSA2048
	case "RSA3072":
		return certcrypto.RSA3072
	case "RSA4096":
		return certcrypto.RSA4096
	case "RSA8192":
		return certcrypto.RSA8192
	case "EC256":
		return certcrypto.EC256
	case "EC384":
		return certcrypto.EC384
	default:
		return certcrypto.RSA2048
	}
}

两边不对称,无法完成对比。

RSA、EC 是算法,2048、4906 这些是公钥的位长度,因此比较其 x509 对象的 PublicKey 属性即可。

伪代码:

switch pubkey := certX509.PublicKey.(type) {
case *rsa.PublicKey:
  bitSize := pubkey.N.BitLen()
  switch bitSize {
    case 2048:
      // RSA2048
    case 4096:
      // RSA4096
  }
case *ecdsa.PublicKey:
  bitSize := pubkey.Curve.Params().BitSize
  switch bitSize {
    case 256:
      // EC256
    case 384:
      // EC384
  }
}

Comment thread internal/domains/deploy.go Outdated
@fudiwei fudiwei added the help wanted Extra attention is needed label Nov 15, 2024
Copy link
Copy Markdown
Member

@usual2970 usual2970 left a comment

Choose a reason for hiding this comment

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

LGTM

@usual2970 usual2970 merged commit 9b52567 into certimate-go:main Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request help wanted Extra attention is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants