diff --git a/xds/internal/xdsclient/cds_test.go b/xds/internal/xdsclient/cds_test.go index dc29cffac3b5..177665bd615d 100644 --- a/xds/internal/xdsclient/cds_test.go +++ b/xds/internal/xdsclient/cds_test.go @@ -20,6 +20,7 @@ package xdsclient import ( "regexp" + "strings" "testing" v2xdspb "github.com/envoyproxy/go-control-plane/envoy/api/v2" @@ -480,6 +481,176 @@ func (s) TestValidateClusterWithSecurityConfig_EnvVarOff(t *testing.T) { } } +func (s) TestSecurityConfigFromCommonTLSContextUsingNewFields_ErrorCases(t *testing.T) { + tests := []struct { + name string + common *v3tlspb.CommonTlsContext + server bool + wantErr string + }{ + { + name: "unsupported-tls_certificates-field-for-identity-certs", + common: &v3tlspb.CommonTlsContext{ + TlsCertificates: []*v3tlspb.TlsCertificate{ + {CertificateChain: &v3corepb.DataSource{}}, + }, + }, + wantErr: "unsupported field tls_certificates is set in CommonTlsContext message", + }, + { + name: "unsupported-tls_certificates_sds_secret_configs-field-for-identity-certs", + common: &v3tlspb.CommonTlsContext{ + TlsCertificateSdsSecretConfigs: []*v3tlspb.SdsSecretConfig{ + {Name: "sds-secrets-config"}, + }, + }, + wantErr: "unsupported field tls_certificate_sds_secret_configs is set in CommonTlsContext message", + }, + { + name: "unsupported-sds-validation-context", + common: &v3tlspb.CommonTlsContext{ + ValidationContextType: &v3tlspb.CommonTlsContext_ValidationContextSdsSecretConfig{ + ValidationContextSdsSecretConfig: &v3tlspb.SdsSecretConfig{ + Name: "foo-sds-secret", + }, + }, + }, + wantErr: "validation context contains unexpected type", + }, + { + name: "missing-ca_certificate_provider_instance-in-validation-context", + common: &v3tlspb.CommonTlsContext{ + ValidationContextType: &v3tlspb.CommonTlsContext_ValidationContext{ + ValidationContext: &v3tlspb.CertificateValidationContext{}, + }, + }, + wantErr: "expected field ca_certificate_provider_instance is missing in CommonTlsContext message", + }, + { + name: "unsupported-field-verify_certificate_spki-in-validation-context", + common: &v3tlspb.CommonTlsContext{ + ValidationContextType: &v3tlspb.CommonTlsContext_ValidationContext{ + ValidationContext: &v3tlspb.CertificateValidationContext{ + CaCertificateProviderInstance: &v3tlspb.CertificateProviderPluginInstance{ + InstanceName: "rootPluginInstance", + CertificateName: "rootCertName", + }, + VerifyCertificateSpki: []string{"spki"}, + }, + }, + }, + wantErr: "unsupported verify_certificate_spki field in CommonTlsContext message", + }, + { + name: "unsupported-field-verify_certificate_hash-in-validation-context", + common: &v3tlspb.CommonTlsContext{ + ValidationContextType: &v3tlspb.CommonTlsContext_ValidationContext{ + ValidationContext: &v3tlspb.CertificateValidationContext{ + CaCertificateProviderInstance: &v3tlspb.CertificateProviderPluginInstance{ + InstanceName: "rootPluginInstance", + CertificateName: "rootCertName", + }, + VerifyCertificateHash: []string{"hash"}, + }, + }, + }, + wantErr: "unsupported verify_certificate_hash field in CommonTlsContext message", + }, + { + name: "unsupported-field-require_signed_certificate_timestamp-in-validation-context", + common: &v3tlspb.CommonTlsContext{ + ValidationContextType: &v3tlspb.CommonTlsContext_ValidationContext{ + ValidationContext: &v3tlspb.CertificateValidationContext{ + CaCertificateProviderInstance: &v3tlspb.CertificateProviderPluginInstance{ + InstanceName: "rootPluginInstance", + CertificateName: "rootCertName", + }, + RequireSignedCertificateTimestamp: &wrapperspb.BoolValue{Value: true}, + }, + }, + }, + wantErr: "unsupported require_sugned_ceritificate_timestamp field in CommonTlsContext message", + }, + { + name: "unsupported-field-crl-in-validation-context", + common: &v3tlspb.CommonTlsContext{ + ValidationContextType: &v3tlspb.CommonTlsContext_ValidationContext{ + ValidationContext: &v3tlspb.CertificateValidationContext{ + CaCertificateProviderInstance: &v3tlspb.CertificateProviderPluginInstance{ + InstanceName: "rootPluginInstance", + CertificateName: "rootCertName", + }, + Crl: &v3corepb.DataSource{}, + }, + }, + }, + wantErr: "unsupported crl field in CommonTlsContext message", + }, + { + name: "unsupported-field-custom_validator_config-in-validation-context", + common: &v3tlspb.CommonTlsContext{ + ValidationContextType: &v3tlspb.CommonTlsContext_ValidationContext{ + ValidationContext: &v3tlspb.CertificateValidationContext{ + CaCertificateProviderInstance: &v3tlspb.CertificateProviderPluginInstance{ + InstanceName: "rootPluginInstance", + CertificateName: "rootCertName", + }, + CustomValidatorConfig: &v3corepb.TypedExtensionConfig{}, + }, + }, + }, + wantErr: "unsupported custom_validator_config field in CommonTlsContext message", + }, + { + name: "invalid-match_subject_alt_names-field-in-validation-context", + common: &v3tlspb.CommonTlsContext{ + ValidationContextType: &v3tlspb.CommonTlsContext_ValidationContext{ + ValidationContext: &v3tlspb.CertificateValidationContext{ + CaCertificateProviderInstance: &v3tlspb.CertificateProviderPluginInstance{ + InstanceName: "rootPluginInstance", + CertificateName: "rootCertName", + }, + MatchSubjectAltNames: []*v3matcherpb.StringMatcher{ + {MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: ""}}, + }, + }, + }, + }, + wantErr: "empty prefix is not allowed in StringMatcher", + }, + { + name: "unsupported-field-matching-subject-alt-names-in-validation-context-of-server", + common: &v3tlspb.CommonTlsContext{ + ValidationContextType: &v3tlspb.CommonTlsContext_ValidationContext{ + ValidationContext: &v3tlspb.CertificateValidationContext{ + CaCertificateProviderInstance: &v3tlspb.CertificateProviderPluginInstance{ + InstanceName: "rootPluginInstance", + CertificateName: "rootCertName", + }, + MatchSubjectAltNames: []*v3matcherpb.StringMatcher{ + {MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: "sanPrefix"}}, + }, + }, + }, + }, + server: true, + wantErr: "match_subject_alt_names field in validation context is not supported on the server", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + _, err := securityConfigFromCommonTLSContextUsingNewFields(test.common, test.server) + if err == nil { + t.Fatal("securityConfigFromCommonTLSContextUsingNewFields() succeeded when expected to fail") + } + if !strings.Contains(err.Error(), test.wantErr) { + t.Fatalf("securityConfigFromCommonTLSContextUsingNewFields() returned err: %v, wantErr: %v", err, test.wantErr) + } + }) + } +} + func (s) TestValidateClusterWithSecurityConfig(t *testing.T) { const ( identityPluginInstance = "identityPluginInstance" @@ -503,6 +674,25 @@ func (s) TestValidateClusterWithSecurityConfig(t *testing.T) { wantUpdate ClusterUpdate wantErr bool }{ + { + name: "transport-socket-matches", + cluster: &v3clusterpb.Cluster{ + ClusterDiscoveryType: &v3clusterpb.Cluster_Type{Type: v3clusterpb.Cluster_EDS}, + EdsClusterConfig: &v3clusterpb.Cluster_EdsClusterConfig{ + EdsConfig: &v3corepb.ConfigSource{ + ConfigSourceSpecifier: &v3corepb.ConfigSource_Ads{ + Ads: &v3corepb.AggregatedConfigSource{}, + }, + }, + ServiceName: serviceName, + }, + LbPolicy: v3clusterpb.Cluster_ROUND_ROBIN, + TransportSocketMatches: []*v3clusterpb.Cluster_TransportSocketMatch{ + {Name: "transport-socket-match-1"}, + }, + }, + wantErr: true, + }, { name: "transport-socket-unsupported-name", cluster: &v3clusterpb.Cluster{ @@ -574,6 +764,56 @@ func (s) TestValidateClusterWithSecurityConfig(t *testing.T) { }, wantErr: true, }, + { + name: "transport-socket-unsupported-tls-params-field", + cluster: &v3clusterpb.Cluster{ + ClusterDiscoveryType: &v3clusterpb.Cluster_Type{Type: v3clusterpb.Cluster_EDS}, + EdsClusterConfig: &v3clusterpb.Cluster_EdsClusterConfig{ + EdsConfig: &v3corepb.ConfigSource{ + ConfigSourceSpecifier: &v3corepb.ConfigSource_Ads{ + Ads: &v3corepb.AggregatedConfigSource{}, + }, + }, + ServiceName: serviceName, + }, + LbPolicy: v3clusterpb.Cluster_ROUND_ROBIN, + TransportSocket: &v3corepb.TransportSocket{ + ConfigType: &v3corepb.TransportSocket_TypedConfig{ + TypedConfig: testutils.MarshalAny(&v3tlspb.UpstreamTlsContext{ + CommonTlsContext: &v3tlspb.CommonTlsContext{ + TlsParams: &v3tlspb.TlsParameters{}, + }, + }), + }, + }, + }, + wantErr: true, + }, + { + name: "transport-socket-unsupported-custom-handshaker-field", + cluster: &v3clusterpb.Cluster{ + ClusterDiscoveryType: &v3clusterpb.Cluster_Type{Type: v3clusterpb.Cluster_EDS}, + EdsClusterConfig: &v3clusterpb.Cluster_EdsClusterConfig{ + EdsConfig: &v3corepb.ConfigSource{ + ConfigSourceSpecifier: &v3corepb.ConfigSource_Ads{ + Ads: &v3corepb.AggregatedConfigSource{}, + }, + }, + ServiceName: serviceName, + }, + LbPolicy: v3clusterpb.Cluster_ROUND_ROBIN, + TransportSocket: &v3corepb.TransportSocket{ + ConfigType: &v3corepb.TransportSocket_TypedConfig{ + TypedConfig: testutils.MarshalAny(&v3tlspb.UpstreamTlsContext{ + CommonTlsContext: &v3tlspb.CommonTlsContext{ + CustomHandshaker: &v3corepb.TypedExtensionConfig{}, + }, + }), + }, + }, + }, + wantErr: true, + }, { name: "transport-socket-unsupported-validation-context", cluster: &v3clusterpb.Cluster{ diff --git a/xds/internal/xdsclient/filter_chain.go b/xds/internal/xdsclient/filter_chain.go index 5b2fd79973ad..87aea9e46e1d 100644 --- a/xds/internal/xdsclient/filter_chain.go +++ b/xds/internal/xdsclient/filter_chain.go @@ -519,6 +519,17 @@ func (fci *FilterChainManager) filterChainFromProto(fc *v3listenerpb.FilterChain if err := proto.Unmarshal(any.GetValue(), downstreamCtx); err != nil { return nil, fmt.Errorf("failed to unmarshal DownstreamTlsContext in LDS response: %v", err) } + if downstreamCtx.GetRequireSni().GetValue() { + return nil, fmt.Errorf("require_sni field set to true in DownstreamTlsContext message: %v", downstreamCtx) + } + if downstreamCtx.GetOcspStaplePolicy() != v3tlspb.DownstreamTlsContext_LENIENT_STAPLING { + return nil, fmt.Errorf("ocsp_staple_policy field set to unsupported value in DownstreamTlsContext message: %v", downstreamCtx) + } + // The following fields from `DownstreamTlsContext` are ignore: + // - disable_stateless_session_resumption + // - session_ticket_keys + // - session_ticket_keys_sds_secret_config + // - session_timeout if downstreamCtx.GetCommonTlsContext() == nil { return nil, errors.New("DownstreamTlsContext in LDS response does not contain a CommonTlsContext") } diff --git a/xds/internal/xdsclient/filter_chain_test.go b/xds/internal/xdsclient/filter_chain_test.go index fd6b3c224276..5219e34cb105 100644 --- a/xds/internal/xdsclient/filter_chain_test.go +++ b/xds/internal/xdsclient/filter_chain_test.go @@ -372,6 +372,44 @@ func TestNewFilterChainImpl_Failure_BadSecurityConfig(t *testing.T) { }, wantErr: "DownstreamTlsContext in LDS response does not contain a CommonTlsContext", }, + { + desc: "require_sni-set-to-true-in-downstreamTlsContext", + lis: &v3listenerpb.Listener{ + FilterChains: []*v3listenerpb.FilterChain{ + { + TransportSocket: &v3corepb.TransportSocket{ + Name: "envoy.transport_sockets.tls", + ConfigType: &v3corepb.TransportSocket_TypedConfig{ + TypedConfig: testutils.MarshalAny(&v3tlspb.DownstreamTlsContext{ + RequireSni: &wrapperspb.BoolValue{Value: true}, + }), + }, + }, + Filters: emptyValidNetworkFilters, + }, + }, + }, + wantErr: "require_sni field set to true in DownstreamTlsContext message", + }, + { + desc: "unsupported-ocsp_staple_policy-in-downstreamTlsContext", + lis: &v3listenerpb.Listener{ + FilterChains: []*v3listenerpb.FilterChain{ + { + TransportSocket: &v3corepb.TransportSocket{ + Name: "envoy.transport_sockets.tls", + ConfigType: &v3corepb.TransportSocket_TypedConfig{ + TypedConfig: testutils.MarshalAny(&v3tlspb.DownstreamTlsContext{ + OcspStaplePolicy: v3tlspb.DownstreamTlsContext_STRICT_STAPLING, + }), + }, + }, + Filters: emptyValidNetworkFilters, + }, + }, + }, + wantErr: "ocsp_staple_policy field set to unsupported value in DownstreamTlsContext message", + }, { desc: "unsupported validation context in transport socket", lis: &v3listenerpb.Listener{ @@ -397,6 +435,31 @@ func TestNewFilterChainImpl_Failure_BadSecurityConfig(t *testing.T) { }, wantErr: "validation context contains unexpected type", }, + { + desc: "unsupported match_subject_alt_names field in transport socket", + lis: &v3listenerpb.Listener{ + FilterChains: []*v3listenerpb.FilterChain{ + { + TransportSocket: &v3corepb.TransportSocket{ + Name: "envoy.transport_sockets.tls", + ConfigType: &v3corepb.TransportSocket_TypedConfig{ + TypedConfig: testutils.MarshalAny(&v3tlspb.DownstreamTlsContext{ + CommonTlsContext: &v3tlspb.CommonTlsContext{ + ValidationContextType: &v3tlspb.CommonTlsContext_ValidationContextSdsSecretConfig{ + ValidationContextSdsSecretConfig: &v3tlspb.SdsSecretConfig{ + Name: "foo-sds-secret", + }, + }, + }, + }), + }, + }, + Filters: emptyValidNetworkFilters, + }, + }, + }, + wantErr: "validation context contains unexpected type", + }, { desc: "no root certificate provider with require_client_cert", lis: &v3listenerpb.Listener{ diff --git a/xds/internal/xdsclient/xds.go b/xds/internal/xdsclient/xds.go index c2d627c4f25f..686c52a350ff 100644 --- a/xds/internal/xdsclient/xds.go +++ b/xds/internal/xdsclient/xds.go @@ -794,6 +794,9 @@ func dnsHostNameFromCluster(cluster *v3clusterpb.Cluster) (string, error) { // securityConfigFromCluster extracts the relevant security configuration from // the received Cluster resource. func securityConfigFromCluster(cluster *v3clusterpb.Cluster) (*SecurityConfig, error) { + if tsm := cluster.GetTransportSocketMatches(); len(tsm) != 0 { + return nil, fmt.Errorf("unsupport transport_socket_matches field is non-empty: %+v", tsm) + } // The Cluster resource contains a `transport_socket` field, which contains // a oneof `typed_config` field of type `protobuf.Any`. The any proto // contains a marshaled representation of an `UpstreamTlsContext` message. @@ -812,6 +815,10 @@ func securityConfigFromCluster(cluster *v3clusterpb.Cluster) (*SecurityConfig, e if err := proto.Unmarshal(any.GetValue(), upstreamCtx); err != nil { return nil, fmt.Errorf("failed to unmarshal UpstreamTlsContext in CDS response: %v", err) } + // The following fields from `UpstreamTlsContext` are ignored: + // - sni + // - allow_renegotiation + // - max_session_keys if upstreamCtx.GetCommonTlsContext() == nil { return nil, errors.New("UpstreamTlsContext in CDS response does not contain a CommonTlsContext") } @@ -820,16 +827,22 @@ func securityConfigFromCluster(cluster *v3clusterpb.Cluster) (*SecurityConfig, e } // common is expected to be not nil. +// The `alpn_protocols` field is ignored. func securityConfigFromCommonTLSContext(common *v3tlspb.CommonTlsContext, server bool) (*SecurityConfig, error) { - sc, err := securityConfigFromCommonTLSContextUsingNewFields(common) - if err != nil { - return nil, err + if common.GetTlsParams() != nil { + return nil, fmt.Errorf("unsupported tls_params field in CommonTlsContext message: %+v", common) + } + if common.GetCustomHandshaker() != nil { + return nil, fmt.Errorf("unsupported custom_handshaker field in CommonTlsContext message: %+v", common) } + + // For now, if we can't get a valid security config from the new fields, we + // fallback to the old deprecated fields. + // TODO: Drop support for deprecated fields. NACK if err != nil here. + sc, _ := securityConfigFromCommonTLSContextUsingNewFields(common, server) if sc == nil || sc.Equal(&SecurityConfig{}) { - // If we can't get a valid security config from the new fields, we - // fallback to the old deprecated fields. - // TODO(easwars): Remove this once TD starts populating the new fields. - sc, err = securityConfigFromCommonTLSContextWithDeprecatedFields(common) + var err error + sc, err = securityConfigFromCommonTLSContextWithDeprecatedFields(common, server) if err != nil { return nil, err } @@ -850,7 +863,7 @@ func securityConfigFromCommonTLSContext(common *v3tlspb.CommonTlsContext, server return sc, nil } -func securityConfigFromCommonTLSContextWithDeprecatedFields(common *v3tlspb.CommonTlsContext) (*SecurityConfig, error) { +func securityConfigFromCommonTLSContextWithDeprecatedFields(common *v3tlspb.CommonTlsContext, server bool) (*SecurityConfig, error) { // The `CommonTlsContext` contains a // `tls_certificate_certificate_provider_instance` field of type // `CertificateProviderInstance`, which contains the provider instance name @@ -883,6 +896,9 @@ func securityConfigFromCommonTLSContextWithDeprecatedFields(common *v3tlspb.Comm matchers = append(matchers, matcher) } } + if server && len(matchers) != 0 { + return nil, fmt.Errorf("match_subject_alt_names field in validation context is not supported on the server: %v", common) + } sc.SubjectAltNameMatchers = matchers if pi := combined.GetValidationContextCertificateProviderInstance(); pi != nil { sc.RootInstanceName = pi.GetInstanceName() @@ -909,15 +925,20 @@ func securityConfigFromCommonTLSContextWithDeprecatedFields(common *v3tlspb.Comm // // This helper function attempts to fetch security configuration from the // `CertificateProviderPluginInstance` message, given a CommonTlsContext. -func securityConfigFromCommonTLSContextUsingNewFields(common *v3tlspb.CommonTlsContext) (*SecurityConfig, error) { +func securityConfigFromCommonTLSContextUsingNewFields(common *v3tlspb.CommonTlsContext, server bool) (*SecurityConfig, error) { // The `tls_certificate_provider_instance` field of type // `CertificateProviderPluginInstance` is used to fetch the identity // certificate provider. sc := &SecurityConfig{} - if identity := common.GetTlsCertificateProviderInstance(); identity != nil { - sc.IdentityInstanceName = identity.GetInstanceName() - sc.IdentityCertName = identity.GetCertificateName() + identity := common.GetTlsCertificateProviderInstance() + if identity == nil && len(common.GetTlsCertificates()) != 0 { + return nil, fmt.Errorf("expected field tls_certificate_provider_instance is not set, while unsupported field tls_certificates is set in CommonTlsContext message: %+v", common) + } + if identity == nil && common.GetTlsCertificateSdsSecretConfigs() != nil { + return nil, fmt.Errorf("expected field tls_certificate_provider_instance is not set, while unsupported field tls_certificate_sds_secret_configs is set in CommonTlsContext message: %+v", common) } + sc.IdentityInstanceName = identity.GetInstanceName() + sc.IdentityCertName = identity.GetCertificateName() // The `CommonTlsContext` contains a oneof field `validation_context_type`, // which contains the `CertificateValidationContext` message in one of the @@ -943,7 +964,7 @@ func securityConfigFromCommonTLSContextUsingNewFields(common *v3tlspb.CommonTlsC // - certificate_name // - this is an opaque name passed to the certificate provider var validationCtx *v3tlspb.CertificateValidationContext - switch common.GetValidationContextType().(type) { + switch typ := common.GetValidationContextType().(type) { case *v3tlspb.CommonTlsContext_ValidationContext: validationCtx = common.GetValidationContext() case *v3tlspb.CommonTlsContext_CombinedValidationContext: @@ -951,11 +972,33 @@ func securityConfigFromCommonTLSContextUsingNewFields(common *v3tlspb.CommonTlsC case nil: // It is valid for the validation context to be nil on the server side. return sc, nil - } - if validationCtx == nil || validationCtx.GetCaCertificateProviderInstance() == nil { - // Bail out if the `CertificateProviderPluginInstance` message is not - // found through one of the way detailed above. - return nil, nil + default: + return nil, fmt.Errorf("validation context contains unexpected type: %T", typ) + } + // If we get here, it means that the `CertificateValidationContext` message + // was found through one of the supported ways. It is an error if the + // validation context is specified, but it does not contain the + // ca_certificate_provider_instance field which contains information about + // the certificate provider to be used for the root certificates. + if validationCtx.GetCaCertificateProviderInstance() == nil { + return nil, fmt.Errorf("expected field ca_certificate_provider_instance is missing in CommonTlsContext message: %+v", common) + } + // The following fields are ignored: + // - trusted_ca + // - watched_directory + // - allow_expired_certificate + // - trust_chain_verification + switch { + case len(validationCtx.GetVerifyCertificateSpki()) != 0: + return nil, fmt.Errorf("unsupported verify_certificate_spki field in CommonTlsContext message: %+v", common) + case len(validationCtx.GetVerifyCertificateHash()) != 0: + return nil, fmt.Errorf("unsupported verify_certificate_hash field in CommonTlsContext message: %+v", common) + case validationCtx.GetRequireSignedCertificateTimestamp().GetValue(): + return nil, fmt.Errorf("unsupported require_sugned_ceritificate_timestamp field in CommonTlsContext message: %+v", common) + case validationCtx.GetCrl() != nil: + return nil, fmt.Errorf("unsupported crl field in CommonTlsContext message: %+v", common) + case validationCtx.GetCustomValidatorConfig() != nil: + return nil, fmt.Errorf("unsupported custom_validator_config field in CommonTlsContext message: %+v", common) } if rootProvider := validationCtx.GetCaCertificateProviderInstance(); rootProvider != nil { @@ -970,6 +1013,9 @@ func securityConfigFromCommonTLSContextUsingNewFields(common *v3tlspb.CommonTlsC } matchers = append(matchers, matcher) } + if server && len(matchers) != 0 { + return nil, fmt.Errorf("match_subject_alt_names field in validation context is not supported on the server: %v", common) + } sc.SubjectAltNameMatchers = matchers return sc, nil }