ServerTlsContext: allow disabling verify peer name

[Thomas-Gelf]

Motivation: servers accepting connections from trusted peers do not know the expected peer name in advance. Therefore, it must be possible to accept incoming connections (validating their client certificate) without being forced to specify an expected client name.

You do not need this when using amphp/socket to run your very own public web server, but it is a requirement when running every other kind of service based on trusted client certificates (with more than one client).

This patch tries to address this, while preserving compatibility with the current behaviour.

[Thomas-Gelf]

Hint: with the last force-push I applied a similar patch for ClientTlsContext too. Faced this issue there too, and keeping them similar makes sense anyway.

[kelunik]

How does your setup look like? How are the certificates validated?

[Thomas-Gelf]

Hi @kelunik,

custom CA / trust store directory. Verify_peer rejects every connection not signed by a trusted CA. Peer names are not known at connection setup time, especially not on the server side - verify_peer_name is therefore a problem. Once verify_peer allowed connection setup, we grant dedicated permissions based on certificate subject/name or custom certificate extensions.

Cheers,
Thomas

[Thomas-Gelf]

@kelunik: if you have related concerns/doubts or different opinions, please let me know

[kelunik]

@Thomas-Gelf Sorry for still not tagging a release, yet. Could you please have a look at 0169e97? If you think this is a good idea, I'll go ahead and tag a release.

[Thomas-Gelf]

Hi @kelunik,

what worries me is that with those changes the following lines would unexpectedly have a different outcome:

$ctx->withoutPeerNameVerification()->withPeerVerification();
$ctx->withPeerVerification()->withoutPeerNameVerification();

Your change might be related to my "magic" when asking for peer name verification state. My motivation was that there cannot be a name verification w/o peer verification for a Server. For clients this would also make no sense from a cryptographic point of view, but someone might argue that he is only interested in a pure string comparison to make sure, to not connect to the wrong server, while not knowing the related CA. And NO, personally I wouldn't do this.

If you want to streamline Server and Client please feel free to remove some magic, but please do not introduce the change resulting in the behaviour shown above.

In a completely "transparent" implementation (just wrapping context options), there isn't much to discuss, but as soon as we try to provide specific default behaviours and "good choices" on behalf of developers / users of this library, things get complicated quickly ;-) IMHO, in this context treating Client and Server differently is absolutely valid, if not required.

• for a Client, verify_peer without verify_peer_name is never secure, but might be used for various reasons
• for a server, using verify_peer without verify_peer_name is useful wherever you have your very own CA, and want to make sure, that only trusted clients with signed certificates are allowed to connect. Those clients might be thousands, so how do you set peer_name in a way satisfying verify_peer_name? In some scenarios using a wildcard in the peer name might help, but that doesn't fit most environments I guess.
• for both Server and Client using verify_peer_name without verify_peer makes no sense at all, BUT you might face developers who want to configure this on the client

Hope this helps!

Cheers,
Thomas

[kelunik]

Thank you for the very detailed and fast response!

for both Server and Client using verify_peer_name without verify_peer makes no sense at all, BUT you might face developers who want to configure this on the client

Indeed, that's why we didn't allow to change only verify peer name to begin with.

$ctx->withoutPeerNameVerification()->withPeerVerification();
$ctx->withPeerVerification()->withoutPeerNameVerification();

Regarding those two, what's your expectation vs. actual result? When I read those, the behavior I introduced seems the expected result IMO, but it has of course the downside of first having to decide on overall peer verification and then about the name.

We could probably even make the choice based on whether a peer name is provided or not for clients?

In the end, we also use mTLS at work with a custom CA and don't care about the peer names there.

[Thomas-Gelf]

for both Server and Client using verify_peer_name without verify_peer makes no sense at all, BUT you might face developers who want to configure this on the client

Indeed, that's why we didn't allow to change only verify peer name to begin with.

Sure, but the other way round, using verify_peer w/o verify_peer_name is an absolutely useful option in a Server context ;-)

$ctx->withoutPeerNameVerification()->withPeerVerification();
$ctx->withPeerVerification()->withoutPeerNameVerification();

Regarding those two, what's your expectation vs. actual result?

I would expect both to result in verify_peer: true, verify_peer_name: false, but instead with 0169e97 the first one would result in verify_peer: true, verify_peer_name: true. This would not only "violate" POLS, but also result in code, where someone "alphabetically sorting those 'without-lines' of his colleagues" would actually unintentionally change behaviour.

We could probably even make the choice based on whether a peer name is provided or not for clients?

For Clients this would be a valid choice, yes. Someone out there might disagree, because of... reasons. However, this combination wasn't possible until now, and no one complained - so it would be fine I guess.

In the end, we also use mTLS at work with a custom CA and don't care about the peer names there.

So that's probably why you discovered this PR again I guess 😅

[kelunik]

This would not only "violate" POLS, but also result in code, where someone "alphabetically sorting those 'without-lines' of his colleagues" would actually unintentionally change behaviour.

If you sort these alphabetically there might always be unexpected changes. Some method might set multiple settings at once and then a later one change things. That's for example the case here, but I can imagine other cases, security level is an implicit one with similar effect.

The good thing is, even if someone does it wrong, it will be secure and not leave a security hole. It just won't work.

If I read these literally without having PHPs native options in mind, the result seems expected to me.

So that's probably why you discovered this PR again I guess 😅

It's actually a completely different stack (Java), but yeah. I rediscovered it now due to a chat message. 😅