pilot-agent should do a basic validity check on the root cert bundle prior to sending to envoy

mikegrass commented 7 months ago

Is this the right place to submit this?

[X] This is not a security vulnerability or a crashing bug
[X] This is not a question about how to use Istio

Bug Description

We've noticed some cases in our environment where pilot-agent pushes an empty root cert bundles to envoy, which then gets rejected.

This can happen if the entity updating the trust bundle does not do an atomic write (write to a temp file in the same dir, then rename).

Example log sequence:

2024-02-08T07:36:45.548547Z info    cache   event for file certificate /path/to/cacerts.pem : WRITE, pushing to proxy
2024-02-08T07:36:45.548626Z info    ads XDS: Incremental Pushing ConnectedEndpoints:8 Version:
2024-02-08T07:36:45.548737Z info    cache   read certificate from file  resource=file-root:/path/to/cacerts.pem
2024-02-08T07:36:45.549032Z warning envoy config external/envoy/source/extensions/config_subscription/grpc/grpc_subscription_impl.cc:128    gRPC config for type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.Secret rejected: SAN-based verification of peer certificates without trusted CA is insecure and not allowed   thread=116

pilot-agent can be improved to do a basic validity check of the trust bundle before pushing the update event to the proxy.

Note that #37722 added validity checks for private keys/certs. This request is to add a similar check for root cert bundles.

Version

$ istioctl version
client version: 1.19-alpha.a15cd06012e0f0a8e6604960da48abfeedb042bc
control plane version: 1.19-alpha.a15cd06012e0f0a8e6604960da48abfeedb042bc
data plane version: 1.19-alpha.a15cd06012e0f0a8e6604960da48abfeedb042bc (6 proxies), 1.20-alpha.e8e2bf61d24477a0dda67d9de8da9cd8f925fb65 (586 proxies)

$ kubectl version
Client Version: v1.24.15
Kustomize Version: v4.5.4
Server Version: v1.26.12-eks-5e0fdde

Additional Information

No response

mikegrass commented 7 months ago

cc: @ramaraochavali

hzxuzhonghu commented 7 months ago

we have done similar check in istiod iirc

ramaraochavali commented 7 months ago

Yes. We did. We need the same check in agent.

leosarra commented 7 months ago

@ramaraochavali do you mind if I take a look at this to build some experience on the secrets handling?

ramaraochavali commented 7 months ago

I have just pushed the change some time back. Sorry. you should have told me earlier. Next time you can do

leosarra commented 7 months ago

np :+1:

istio / istio