MAISTRA-2687 Fix spiffe certificate verification

[luksa]

Previously, when copying the X509 store context, the code erroneously copied the output certificate chain instead of the input (untrusted) chain to the new context. As the output certificate chain is set only after you run X509_verify_cert(), this chain was always empty. Thus, the code only worked properly when the leaf certificate was signed by one of the certificates in the trust bundle (e.g. when the leaf certificate was signed directly by the root certificate). In cases involving intermediate certificates, the leaf certificate was only trusted if all the intermediate certificates were in the trust bundle. Intermediate certificates sent by the client were ignored.

When copying the X509 store context, the input intermediate certificates should be retrieved by calling X509_STORE_CTX_get0_untrusted(), not X509_STORE_CTX_get0_chain().

[oschaaf]

Nice catch! I wonder would it be hard to add a test in test/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator_test.cc?

[dgn]

/retest

[dgn]

/retest

[dgn]

/test envoy-unit

[dgn]

really good catch. I wonder if our copying/re-init of a fresh x509 store is actually necessary?

[twghu]

Checking the validity of this change in relation to previous upstream commit.

[dgn]

/retest

[twghu]

There appears to be a problem with the spiffe_san_signed_by_intermediate_cert_chain.pem cert, the ca cert included does not match the ca cert using as an anchor in the envoy configuration in the test.
This fails:

openssl verify -verbose -CAfile ca_cert.pem -untrusted  intermediate_ca_cert.pem spiffe_san_signed_by_intermediate_cert.pem
C = US, ST = California, L = San Francisco, O = Lyft, OU = Lyft Engineering, CN = Test Server
error 20 at 0 depth lookup: unable to get local issuer certificate
error spiffe_san_signed_by_intermediate_cert.pem: verification failed

If we run the openssl/demos/certs/mkcerts.sh script (openssl source) and then run

openssl verify -CAfile root.pem -untrusted intca.pem server.pem
server.pem: OK

[dgn]

/retest

[luksa]

There appears to be a problem with the spiffe_san_signed_by_intermediate_cert_chain.pem cert, the ca cert included does not match the ca cert using as an anchor in the envoy configuration in the test. This fails:

Thanks. Yeah, I built the certs by running certs.sh. I did verify the new cert with openssl and it worked. But running certs.sh replaces the ca and intermediate cert, so I reverted them before committing. Obviously, the new cert was no longer valid, because the ca cert was now different.

[oschaaf]

/test unit

[oschaaf]

This looks good to me.
Deferring to @twghu for final review.

[twghu]

Looks good to me. Builds and tests pass.

[jwendell]

Dumb question: Does this affects only our fork? Or upstream too?

[oschaaf]

@jwendell good call, so after comparing the upstream code, I think you are right: it looks like this affects upstream as well.

[luksa]

@jwendell @oschaaf As far as I can tell, this issue in only in our code, because we clone store_ctx (I'm not sure why). Upstream executes X509_verify_cert(store_ctx), whereas we invoke X509_verify_cert(verify_ctx). Given that my fix copies the intermediate certs from store_ctx to verify_ctx, it's clear that the store_ctx contains the certificates. Therefore, the verification should work properly.

[twghu]

It would make sense to add the BoringSSL version of this chain validation test to upstream.

[dmitri-d]

As far as I can tell, this issue in only in our code, because we clone store_ctx (I'm not sure why).

A fly-by comment: BoringSSL exposes store_ctx/context_ctx data structures and makes all their fields directly accessible. OpenSSL hides details of those and only allows access and modification for some of the fields (and only via api), hence all these hoops... Sorry about the bug!

[luksa]

@twghu Here's the upstream PR: envoyproxy/envoy#18914

@dmitri-d Thanks for the explanation. No worries about the bug.

[luksa]

/test unit