Support MSC4040 matrix-fed service name lookup
#142
Conversation
For matrix-org/matrix-federation-tester#142 See matrix-org/matrix-spec#1624 Co-authored-by: devonh <[email protected]>
| // isValidCertificate is sourced from an old version of gomatrixserverlib | ||
| func isValidCertificate(serverName spec.ServerName, c *x509.Certificate, intermediates *x509.CertPool) (valid bool, err error) { | ||
| host, _, isValid := spec.ParseAndValidateServerName(serverName) | ||
| if !isValid { | ||
| err = fmt.Errorf("%q is not a valid serverName", serverName) | ||
| return false, err | ||
| } | ||
|
|
||
| // Check certificate chain validity | ||
| verificationOpts := x509.VerifyOptions{ | ||
| // Certificate.Verify appears to handle IP addresses optionally surrounded by square brackets. | ||
| DNSName: host, | ||
| Intermediates: intermediates, | ||
| } | ||
| roots, err := c.Verify(verificationOpts) | ||
|
|
||
| return len(roots) > 0, err | ||
| } |
There was a problem hiding this comment.
This makes me a bit nervous, it seems it was removed in matrix-org/gomatrixserverlib#364 and added in matrix-org/gomatrixserverlib@5a698c3 and never used anywhere (except here?)
tl;dr Is there a standard go way to validate the certificate instead of custom code like this?
There was a problem hiding this comment.
fwiw, here is how I do it in MMR (another Go project).
The block of code added in the PR here was mostly out of fear: it felt particularly magic, and I wasn't super interested in debugging too much of the federation tester 😅
There was a problem hiding this comment.
Given we're just doing this for informational purposes, I think this is OK. I mean, it also looks sane (but looks can be deceiving).
I can't see how you'd really knock any of this off — you need to be able to specify the DNS name and any intermediate certs. From recent experience working with X.509 I'd say it looks decent. But even if I was wrong, the worst we'll do is show the wrong result in the tester? I don't feel a grand need to worry too much.
There was a problem hiding this comment.
Most of these changes are just from bumping the gomatrixserverlib used, I think?
There was a problem hiding this comment.
yes - should be its own commit
Co-authored-by: Sandro <[email protected]>
| // Append the deprecated ones too | ||
| cname, records, err2 := net.LookupSRV("matrix", "tcp", string(serverName)) | ||
| if result.SRVCName == "" { | ||
| result.SRVCName = cname | ||
| } | ||
| if records != nil { | ||
| result.SRVRecords = append(result.SRVRecords, records...) | ||
| } |
There was a problem hiding this comment.
Does this really make sense? I suspect it would be better to return it in a different field and update the UI to show reasonable information about the legacy vs. new ones.
There was a problem hiding this comment.
Returning a new field felt sufficiently complicated, for reasons I don't remember. I agree that a new field would be best.
There was a problem hiding this comment.
Will the UI tool be able to distinguish the two different types of SRV records based on this info?
I think a new field could possibly be best too, but either way I would like to ensure the information is there.
(Maybe you kind of want the result to be a warning of some sort if only the legacy option is set...)
| // isValidCertificate is sourced from an old version of gomatrixserverlib | ||
| func isValidCertificate(serverName spec.ServerName, c *x509.Certificate, intermediates *x509.CertPool) (valid bool, err error) { | ||
| host, _, isValid := spec.ParseAndValidateServerName(serverName) | ||
| if !isValid { | ||
| err = fmt.Errorf("%q is not a valid serverName", serverName) | ||
| return false, err | ||
| } | ||
|
|
||
| // Check certificate chain validity | ||
| verificationOpts := x509.VerifyOptions{ | ||
| // Certificate.Verify appears to handle IP addresses optionally surrounded by square brackets. | ||
| DNSName: host, | ||
| Intermediates: intermediates, | ||
| } | ||
| roots, err := c.Verify(verificationOpts) | ||
|
|
||
| return len(roots) > 0, err | ||
| } |
There was a problem hiding this comment.
Given we're just doing this for informational purposes, I think this is OK. I mean, it also looks sane (but looks can be deceiving).
I can't see how you'd really knock any of this off — you need to be able to specify the DNS name and any intermediate certs. From recent experience working with X.509 I'd say it looks decent. But even if I was wrong, the worst we'll do is show the wrong result in the tester? I don't feel a grand need to worry too much.
| // Append the deprecated ones too | ||
| cname, records, err2 := net.LookupSRV("matrix", "tcp", string(serverName)) | ||
| if result.SRVCName == "" { | ||
| result.SRVCName = cname | ||
| } | ||
| if records != nil { | ||
| result.SRVRecords = append(result.SRVRecords, records...) | ||
| } |
There was a problem hiding this comment.
Will the UI tool be able to distinguish the two different types of SRV records based on this info?
I think a new field could possibly be best too, but either way I would like to ensure the information is there.
(Maybe you kind of want the result to be a warning of some sort if only the legacy option is set...)
|
This PR may need taking over from someone on the maintenance side, sorry. This PR is becoming fairly critical to deploy, but I'm running out of spare time to push it forward. |
See matrix-org/matrix-spec#1624
Requires matrix-org/gomatrixserverlib#411
Domain tests:
3c.s.resolvematrix.dev-_matrixonly,.well-known4.s.resolvematrix.dev-_matrixonly3c.msc4040.s.resolvematrix.dev-_matrix-fedonly,.well-known4.msc4040.s.resolvematrix.dev-_matrix-fedonlyt2l.io-_matrix-fedand_matrixfallback(TODO: Update resolvematrix.dev to support this case)