server: fix the advisory certificate check #39166
Conversation
|
This change is |
pkg/base/addr_validation.go
Outdated
| notifyFunc := func(msg, host string) { | ||
| if buf.Len() > 0 { | ||
| buf.WriteString(" and ") | ||
| host, _, _ := net.SplitHostPort(cfg.AdvertiseAddr) |
There was a problem hiding this comment.
Is there a reason you're ignoring the returned error? Have there been valid addresses we failed on?
There was a problem hiding this comment.
I'm assuming here ValidateAddrs() has been called already. If the syntax was not correct that would have failed before. Do you think I should still check here with a panic("call ValidateAddrs first")?
There was a problem hiding this comment.
I'm fine leaving it out, mostly curious.
(context: `cockroach start` performs some advisory checks on the certificates prior to starting up, to inform the user upfront if the cert configuration is suspicious. These checks are complementary to the actual security validation performed in the go std lib.) Prior to this patch, the advisory cert validation was performing checks on both the addres given to `--listen` and that given to `--advertise` (either explicitly, or implicitly when derived from `--listen`). This was problematic because the listen address is really irrelevant to security validation. What matters for node cert validation is that the advertised address is present. So this patch removes the check on the listen addr (recommended by @mberhault). Additionally this patch also enhances the check on the advertise addr: if a host name is given to `--advertise-addr` (either explicitly, or implicitly as derived from `--listen-addr`), and the name is not present in the cert, the validation is considered to pass if the resolved address for the given name is present int he cert. Release note (cli change): The advisory/informative check performed by `cockroach start` on the validity of addresses contained in the node certificate is relaxed to focus on the advertised node address, and to tolerate cases when the cert contains an IP address but a hostname is specified as advertised address.
|
TFYR bors r+ |
39148: importccl: Modify row_id generation for IMPORT from CSV. r=adityamaru27 a=adityamaru27 This change was motivated by a bug in which consecutive IMPORT INTO queries into a table without an explicit PK, and from unique data sources would overwrite instead of appending data. This is because row_id generation was based on fileIndex and rowNum which created colliding PKs across query runs. To fix this we add the timestamp at which IMPORT INTO is run at a 10-microsecond granularity to the rowNum. 39166: server: fix the advisory certificate check r=knz a=knz Fixes #32539. (context: `cockroach start` performs some advisory checks on the certificates prior to starting up, to inform the user upfront if the cert configuration is suspicious. These checks are complementary to the actual security validation performed in the go std lib.) Prior to this patch, the advisory cert validation was performing checks on both the addres given to `--listen` and that given to `--advertise` (either explicitly, or implicitly when derived from `--listen`). This was problematic because the listen address is really irrelevant to security validation. What matters for node cert validation is that the advertised address is present. So this patch removes the check on the listen addr (recommended by @mberhault). Additionally this patch also enhances the check on the advertise addr: if a host name is given to `--advertise-addr` (either explicitly, or implicitly as derived from `--listen-addr`), and the name is not present in the cert, the validation is considered to pass if the resolved address for the given name is present int he cert. Release note (cli change): The advisory/informative check performed by `cockroach start` on the validity of addresses contained in the node certificate is relaxed to focus on the advertised node address, and to tolerate cases when the cert contains an IP address but a hostname is specified as advertised address. Co-authored-by: Aditya Maru <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]>
Build succeeded |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @knz and @mberhault)
pkg/base/addr_validation.go, line 287 at r2 (raw file):
} if err := cert.VerifyHostname(host); err != nil { // The hostname itself is not in the cert. Is the numeric address in the cert?
Does it work to advertise a name but only have an IP address in the cert? I don't think it does. If it does work, that would be a security problem since DNS is insecure (it's not actually a security hole in this method since it's just advisory and the real check is in crypto/tls, but it's also not doing anything useful to manually resolve the IP address and compare it to the cert as far as I can tell.
|
Ben do you have any idea where to look in the go stdlib to check what you say? |
|
Grep for But it may be more straightforward to just write a test and see what happens: advertise |
I looked into this further: yes it works. The situation(problem?) is that it's possible for a node A to advertise some hostname HA, but use A's numeric address when connecting from B (e.g. start B with In that case, B will check A's identity based on Because of that situation, I think it's OK to (advisorily) verify that A's numeric address is in the cert if the hostname isn't. We could (but currently don't) validate inside the crdb protocol that the address given on B's What do you think? |
|
This is why it's sometimes useful to have un-advertised IPs/hostnames in the cert. But that doesn't make it OK to omit the advertised hostname from the cert. In some cases the advertised address will be used, and the connection will fail unless that address is in the cert. It's possible that the cluster will still work (if there are enough nodes using the correct join address that everything can become fully connected), but I think we should still disallow advertising addresses that are not present in the cert. |
|
Fixin in #39179 |
39179: server: simplify the advisory cert check r=knz a=knz Tweak to the previous commit (#39166) around this code, as requested by @bdarnell, see discussion here: #39166 (review) Co-authored-by: Raphael 'kena' Poss <[email protected]>
Fixes #32539.
(context:
cockroach startperforms some advisory checks on thecertificates prior to starting up, to inform the user upfront if the
cert configuration is suspicious. These checks are complementary to
the actual security validation performed in the go std lib.)
Prior to this patch, the advisory cert validation was performing
checks on both the addres given to
--listenand that given to--advertise(either explicitly, or implicitly when derived from--listen).This was problematic because the listen address is really irrelevant
to security validation. What matters for node cert validation is that
the advertised address is present. So this patch removes the check on
the listen addr (recommended by @mberhault).
Additionally this patch also enhances the check on the advertise addr:
if a host name is given to
--advertise-addr(either explicitly, orimplicitly as derived from
--listen-addr), and the name is notpresent in the cert, the validation is considered to pass if the
resolved address for the given name is present int he cert.
Release note (cli change): The advisory/informative check performed by
cockroach starton the validity of addresses contained in the nodecertificate is relaxed to focus on the advertised node address, and to
tolerate cases when the cert contains an IP address but a hostname is
specified as advertised address.