xDS Security: Use new way to fetch certificate provider plugin instance config

[yashykt]

The new method is documented at https://github.com/grpc/proposal/blob/master/A29-xds-tls-security.md and based on envoyproxy/envoy#17201

Note that the older fields are still used as a fallback mechanism since our current infrastructure still depends on those fields.

All existing tests have been modified to use the newer fields and some basic tests have been added to test the old fields.

Also performing a quick check on all the cases mentioned in the gRFC for NACKing behavior. Cases NACKed-

1. Unrecognized certificate provider instance names
2. Bad StringMatcher for match_subject_alt_names
3. CertificateValidationContext:verify_certificate_spki (unsupported)
4. CertificateValidationContext:verify_certificate_hash (unsupported)
5. CertificateValidationContext:require_signed_certificate_timestamp (unsupported)
6. CertificateValidationContext:crl (unsupported)
7. CertificateValidationContext:custom_validator_config (unsupported)
8. CommonTlsContext:validation_context_sds_secret_config (unsupported)
9. CommonTlsContext:tls_certificates (Only NACKed if TLS certificate provider not provided in the supported fields)
10. CommonTlsContext:tls_certificate_sds_secret_configs (Only NACKed if TLS certificate provider not provided in the supported fields)
11. CommonTlsContext:tls_params (unsupported)
12. CommonTlsContext:custom_handshaker (unsupported)
13. DownstreamTlsContext/UpstreamTlsContext:Unrecognized transport socket
14. DownstreamTlsContext:require_sni (unsupported)
15. DownstreamTlsContext:unsupported ocsp_staple_policy (only LENIENT_STAPLING supported)
16. DownstreamTlsContext:TLS configuration without configuration to fetch TLS certs
17. DownstreamTlsContext:Tls configuration requires client certs but no validation certs configuration provided.
18. DownstreamTlsContext:match_subject_alt_names (unsupported on servers)
19. UpstreamTlsContext:TLS configuration provided without validation certs configuration

---

This change is 

[markdroth]

This looks pretty good! My comments are mostly minor, except for the one about updating the tests. I think it's important that our tests cover the fields actually described in the gRFC.

Please let me know if you have any questions. Thanks!

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @donnadionne and @yashykt)

---

src/core/ext/xds/xds_api.cc, line 1912 at r1 (raw file):

}

// CertificateProviderInstance is deprecated but we are still supporting it for

Please add a TODO to remove this as soon as TD is updated to populate the new fields.

---

src/core/ext/xds/xds_api.cc, line 2083 at r1 (raw file):

      envoy_extensions_transport_sockets_tls_v3_CommonTlsContext_combined_validation_context(
          common_tls_context_proto);
  // The validation context is derived from the oneof in

Please move this comment up above the preceding statement.

---

src/core/ext/xds/xds_api.cc, line 2101 at r1 (raw file):

    // 'validation_context_certificate_provider_instance' inside
    // 'combined_validation_context'. Note that this way of fetching root
    // certificates is deprecated and might be removed in the future.

s/might/will/. Please make this a TODO.

---

src/core/ext/xds/xds_api.cc, line 2140 at r1 (raw file):

  } else {
    // Fall back onto 'tls_certificate_certificate_provider_instance'. Note that
    // this way of fetching identity certificates is deprecated and might be

s/might/will/. Please make this a TODO.

---

src/core/ext/xds/xds_api.cc, line 2408 at r1 (raw file):

        "provider instance specified for validation."));
  }
  return GRPC_ERROR_CREATE_FROM_VECTOR("Error parsing DownstreamTlsContext",

If we're not going to support match_subject_alt_names on the server side, then we need an additional check here to add an error if that field is set.

Please thoroughly check the gRFC to make sure that there are no other edge cases we're missing here.

---

src/proto/grpc/testing/xds/v3/tls.proto, line 156 at r1 (raw file):

}

message SdsSecretConfig {}

Doesn't look like this is needed here, since you've added it inside of CommonTlsContext below.

---

test/cpp/end2end/xds_end2end_test.cc, line 8138 at r1 (raw file):

      transport_socket->set_name("envoy.transport_sockets.tls");
      UpstreamTlsContext upstream_tls_context;
      // TODO(yashykt): Modify this to use the new security fields once the

I think we should go ahead and do this now, and just add a separate test or two that use the old fields. That way, when we remove the legacy code, we can just remove those separate tests and everything else will already be correct.

---

test/cpp/end2end/xds_end2end_test.cc, line 9200 at r1 (raw file):

    filter_chain->add_filters()->mutable_typed_config()->PackFrom(
        HttpConnectionManager());
    // TODO(yashykt): Modify this to use the new security fields once the actual

Same here.

[yashykt]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @donnadionne, @markdroth, and @yashykt)

---

src/core/ext/xds/xds_api.cc, line 2408 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

If we're not going to support match_subject_alt_names on the server side, then we need an additional check here to add an error if that field is set.

Please thoroughly check the gRFC to make sure that there are no other edge cases we're missing here.

On the server side an implementation may support this field and its semantics, or NACK the update it if it doesn't. Ah, I missed this earlier

[yashykt]

Reviewable status: 3 of 6 files reviewed, 8 unresolved discussions (waiting on @donnadionne and @markdroth)

---

src/core/ext/xds/xds_api.cc, line 1912 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please add a TODO to remove this as soon as TD is updated to populate the new fields.

Done.

---

src/core/ext/xds/xds_api.cc, line 2083 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please move this comment up above the preceding statement.

Done.

---

src/core/ext/xds/xds_api.cc, line 2101 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

s/might/will/. Please make this a TODO.

Done.

---

src/core/ext/xds/xds_api.cc, line 2140 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

s/might/will/. Please make this a TODO.

Done.

---

src/proto/grpc/testing/xds/v3/tls.proto, line 156 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Doesn't look like this is needed here, since you've added it inside of CommonTlsContext below.

Good eye!

---

test/cpp/end2end/xds_end2end_test.cc, line 8138 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I think we should go ahead and do this now, and just add a separate test or two that use the old fields. That way, when we remove the legacy code, we can just remove those separate tests and everything else will already be correct.

Done.

---

test/cpp/end2end/xds_end2end_test.cc, line 9200 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Same here.

Done.

[yashykt]

Still need to add NACKing for tls_params and custom_handshaker

[yashykt]

Added NACKing for tls_params and custom_handshaker. @markdroth PTALA

[markdroth]

Overall, this looks really good. There's just one problem with the tests to address.

Please let me know if you have any questions. Thanks!

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @donnadionne and @yashykt)

---

test/cpp/end2end/xds_end2end_test.cc, line 9646 at r4 (raw file):

}

TEST_P(XdsServerSecurityTest, NacksFieldTlsCertificates) {

These two tests seem wrong.

On the server side, the TLS cert provider is required, so if it's not present, we should be getting an error that says that the cert provider is not present, and it does not matter whether the tls_certificates or tls_certificate_sds_secret_configs fields are specified. (Technically, we don't even need to check these fields on the server side. It's okay if we do, and it's okay if that error gets added to the NACK message, but what we really care about on the server side is that we get an error message that says that the TLS cert provider is not specified, not whether it also includes errors about these two irrelevant fields.)

Conversely, I think we do need these tests on the client side, because there the TLS cert provider is optional, so the checks for these two fields are relevant.

[yashykt]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @donnadionne and @markdroth)

---

test/cpp/end2end/xds_end2end_test.cc, line 9646 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

These two tests seem wrong.

On the server side, the TLS cert provider is required, so if it's not present, we should be getting an error that says that the cert provider is not present, and it does not matter whether the tls_certificates or tls_certificate_sds_secret_configs fields are specified. (Technically, we don't even need to check these fields on the server side. It's okay if we do, and it's okay if that error gets added to the NACK message, but what we really care about on the server side is that we get an error message that says that the TLS cert provider is not specified, not whether it also includes errors about these two irrelevant fields.)

Conversely, I think we do need these tests on the client side, because there the TLS cert provider is optional, so the checks for these two fields are relevant.

Moved to the client side

[markdroth]

Looks great!

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @donnadionne)

[yashykt]

Thanks for reviewing!