Support for certificate with raw IP SAN (RFC8738)

[orangepizza]

A tiny edit on a few functions that adapt the type of identifier/ SAN type if input 'domain' can be parsed as IP address.
Not bothered to change the name of variable []domains to some better name: kindly don't want to follow all those functions calls to pass the full identifier

test on pebble that this is enough for ipv4 and ipv6 in -d.

Fixes #1649

---

have to create a new PR because the old PR was from master of the fork. (why GitHub does not allow creating new source branch for PR 👎 )

[orangepizza]

why Mac OS ignore RFC3330 and only has 127.0.0.1/32, not 127.0.0.1/8 like everyone else?

[orangepizza]

could you trigger ci run again? it errored on a dns api extension this doesn't touch.

[dmke]

Overall this looks good! I've left some initial thoughts/comments.

[dmke]

RFC8738, section 6 requires the SNI extension to contain the .in-addr.arpa or .ip6.arpa reverse mapping of the IP address.

Can you add a test for this? (miekg/dns).ReverseAddr will be of help here.

[orangepizza]

don't think we need that here, SNI extension is something ACME server's VA should sent, and our TLS-ALPN solver don't care about incomming traffics SNI and just give same certificate

[dmke]

Hm. My thinking is: if the server MUST send the IP address in the SNI extension, the client should validate it is correct. The http-01 challenge solver does something similar with the Host/Forwarded-* headers (this is mostly to prevent DNS rebind attacks though).

@ldez: Do you have an opinion on this?

[orangepizza]

Hm. My thinking is: if the server MUST send the IP address in the SNI extension, the client should validate it is correct. The http-01 challenge solver does something similar with the Host/Forwarded-* headers (this is mostly to prevent DNS rebind attacks though).

as It current TLS-ALPN answerer didn't care or domain test send SNI by moke VA before this commit, and as this is acme client we would be attacker side of such attack, I don't think we need to verify ACME server do the right thithes, that'd be ACME server's duty to cornfirm they are strict enough.

[dmke]

we would be attacker side of such attack

This is not entirely true. With challenges like tls-alpn-01, ACME clients need to make the client host available to the outside world, by means of starting a web server. IIRC, the certificate order (be it a new certificate or renewal of an existing one) is publicly available, even before a certificate is issued. (Yes, the order URL is not easily guessable; no, there's no access control.)

This could allow potential 3rd-party attackers to meddle with the running Lego instance.

There's also random chance (e.g. after port scan or by sheer luck), that someone tries to access the TLS port.

Measures like checking the SNI field raise the bar for anyone accessing the running Lego server; any deviation from the standard should be rejected immediately.

[dmke]

The reverseaddr implementation is redundant. The DNS library we're using already has such a function.

[orangepizza]

1. even if someone else visit our alpn server, connection will closed right after tls handshake anyway (effectively we reject everything), and if go TLS is hopelessly broken such that handshake alone enough to break it, we'd have much bigger problem.
2. if attacker saw to certificate we tried to issue they already know the right name to send.
3. order url require POST-as-GET request signed by JWT signed by account key, so attacker can't read.
4. we didn't do that on domain name, doing that on this PR feels it will blur the scope of PR.

[dmke]

You raise some valid points there, I'm glad you're on top of things.

Regarding (4), I agree. I was under the assumption that we're already checking the SNI field.

I'm currently a bit busy to finish the review in detail, but glancing over it, I don't see any deficits. If @ldez doesn't take over from here, you may need to wait until the weekend before I find some time to do it myself (sorry for that).

[ldez]

LGTM