Fix GH-9356: Incomplete validation of IPv6 Address fields in subjectAltNames

[lucasnetau]

Fixes GH-9356

IPv6 addresses are valid entries in subjectAltNames. Certificate Authorities may issue certificates including IPv6 addresses except if they fall within addresses in the RFC 4193 range. Google and CloudFlare provide IPv6 addresses in their DNS over HTTPS services.

Internal CAs do not have those restrictions and can issue Unique local addresses in certificates.

My C is a bit rusty and this is my first attempt at fixing this issue. I would appreciate any feedback, and assistance in checking I have enabled Windows support for inet_pton correctly, and what to do on systems without inet_pton support (any still around in the supported systems?)

[lucasnetau]

Hi @bukka Could you please review this PR and let me know if I need to add/change anything to add the missing IPv6 support?

[bukka]

I think the change makes sense but in my eyes it's more feature than bug as it was not done on purpose. I'm fine with changing that but just in master so please rebase it.

[lucasnetau]

Thank you for the feedback, I'll make the required code changes.

I was hoping this would be considered a bug per the initial issue #9356 post triage categorisation . The initial support for IP Address fields was implemented as a bug fix (https://bugs.php.net/bug.php?id=68879) and it made the incorrect statement that SAN IP Addresses were deprecated and skipped the implementation of IPv6. So the missing functionality is more a bug in implementing certificate validation specification. I hope you can please reconsider?

This is an issue we encounter when doing IPv6 only work and the various work around we have including peer cert signature verification does not always work as providers such as Google use different certificates on various edge points (even in the same location), having this at least land in the PHP8.2 branch would at least allow us to no longer rely on these workarounds.

[bukka]

Ok after thinking about we can treat this as a bug. Except one small NIT, the changes look fine to me. I just approved CI so we will see how it goes.

[lucasnetau]

Requested CS change made. I also added a test SKIPIF if IPv6 support isn't enabled at compile time or if it has been disabled in CI via the CI_NO_IPV6 flag

[lucasnetau]

LINUX_X32_DEBUG_ZTS doesn't appear to have IPv6 support enabled at the Docker layer and CI_NO_IPV6 is not set, this causes the test to fail due to the inability to create a IPv6 bound server. I've added an additional SKIPIF test taken from (/ext/sockets/tests/ipv6_skipif.inc) to check if hosts level support for IPv6 is enabled.

========DIFF========
001+ Warning: stream_socket_client(): Unable to connect to ssl://[::1]:64324 (Cannot assign requested address) in /__w/php-src/php-src/ext/openssl/tests/ServerClientTestCase.inc(130) : eval()'d code on line 10
002+ bool(false)
001- resource(%d) of type (stream)
     
003- Warning: stream_socket_client(): Unable to locate peer certificate CN in %s on line %d
004- 
005- Warning: stream_socket_client(): Failed to enable crypto in %s on line %d
006- 
007- Warning: stream_socket_client(): Unable to connect to ssl://[::1]:64324 (Unknown error) in %s on line %d
004+ Warning: stream_socket_client(): Unable to connect to ssl://[::1]:64324 (Cannot assign requested address) in /__w/php-src/php-src/ext/openssl/tests/ServerClientTestCase.inc(130) : eval()'d code on line 13
     bool(false)
========DONE========
FAIL IPv6 Peer verification matches SAN names [ext/openssl/tests/san_ipv6_peer_matching.phpt] 

[lucasnetau]

@bukka this should be complete now including ensuring tests can only run on IPv6 compiled php and host enabled CI

[lucasnetau]

@bukka Could you please approve the workflow to do the final tests?

[lucasnetau]

This is ready for merge, all tests have passed and requested changes in place

[lucasnetau]

Hi @bukka Is there any chance of this being merged soon, please?

[bukka]

Apology for slight delay (I usually rotate things to do in PHP every month to not switch context too much and increase my productivity).

The code was good and I also tested everything and all good as well.

It's all merged and also added a follow up minor CS fix in 3fc013b that I didn't want to bother you with.

Thanks for you contribution.