ChainingTrustEngine resets SOAP/TLS-based null peer entity name, forces TrustEngine name matching

http://svn.shibboleth.net/view/cpp-opensaml?rev=720&view=rev

This looks like a regression created in 2.4 that mistakenly resets the peer name property, even though the base class never actually clears it during reset anyway.

HTTP over TLS has its own rules that have to be followed, and we believe it's counterproductive to try and impose a different set of rules for TLS than people ordinarily expect. While it would be somewhat attractive to do the more generic cert matching we do for signing, no other implementation will ever support that. At least with signing, there are no rules for what to do anyway, but we don't think deployers would appreciate being led to rely on TLS name matching that would only work with Shibboleth.

Cloning is probably the appropriate solution here, but I have to maintain backward API compatibility. I can add new methods, but adding virtual methods that a subclass would have to override isn't possible, and there's no other way to clone. So I'll have to come up with something else for now.

Yes, you've nailed it... and that seems to be the intent of the CURLSOAPTransport->verify_callback, based on inline comments: to skip TrustEngine name validation, deferring it to CURL by explicitly setting/leaving the PeerName to null. As you've surmised, changing the metadata wouldn't be a preferred approach.

To be honest, the TrustEngine documentation suggests that all of this name validation occurs within the PKIX TrustEngine and that hostname is added to the list of trusted names for TLS, in addition to the entityId. Implementation doesn't quite follow that, since it would ONLY support one or the other based on sequence of configuration, etc... unless the entityId is fed into the libCurl name validation as an alternate trusted name (not even sure if that's possible).

Thinking out loud... would it be worth "cloning" the input m_criteria for input into each TrustEngine in the chaining to eliminate any cross-contamination? Obviously, from an code/encapsulation perspective, I wouldn't expect input criteria to be changed by a TrustEngine, only read... but I'm sure that's a whole other discussion.

Trying to rephrase what you're seeing: you want the libcurl layer to handle the name validation and skip that in the TrustEngine because your metadata contains neither the certificate itself nor even a KeyName for the host. Therefore, if the PKIX engine runs second, after the reset() call, the engine fails while verifying the name of the certificate because the only valid key name it's using as input to the check is the entityID of the peer. Is that about it?

I think you can work around this without changing the order of the engines by adding a KeyName to the metadata explicitly that contains the hostname that would be in the TLS cert. That probably isn't a better way since you'd have to change the metadata in that case, just like adding the certificate would require. But it would help me verify I understand the issue.

It's the reset override in the opensaml subclass that's resetting the peer name, but I don't understand the actual description of the behavior. Setting the peer name causes name checking to happen, it doesn't prevent it, so the bug should cause more checking than is intended. But your description says "unable to get TLS name validation to occur".

Are you saying you don't want it to check the name and it is anyway? Or you do want it to and it's not?