From e2d7591ddae2693534086690ac41c193d5a3c5ab Mon Sep 17 00:00:00 2001 From: Baiju Parameswaran Date: Tue, 13 Oct 2026 14:00:44 +0000 Subject: [PATCH 1/3] [luciusctl]: Verify remote TLS and restrict plaintext to loopback Apply one transport policy to diagnostic info and SAI commands, including completion. Support private CA trust without disabling certificate checks, retain local plaintext diagnostics, and test the connection boundaries. Signed-off-by: Baiju Parameswaran Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: db2e0f10-3729-4d5d-b4da-3654eb856d74 --- dataplane/docs/dataplanedebug.md | 8 + dataplane/luciusctl/BUILD | 19 +- dataplane/luciusctl/info/BUILD | 5 +- dataplane/luciusctl/info/info.go | 26 +- dataplane/luciusctl/internal/transport/BUILD | 22 ++ .../luciusctl/internal/transport/transport.go | 94 +++++ .../internal/transport/transport_test.go | 115 +++++++ dataplane/luciusctl/luciusctl.go | 5 +- dataplane/luciusctl/luciusctl_test.go | 324 ++++++++++++++++++ dataplane/luciusctl/sai/BUILD | 5 +- dataplane/luciusctl/sai/sai.go | 26 +- 11 files changed, 594 insertions(+), 55 deletions(-) create mode 100644 dataplane/luciusctl/internal/transport/BUILD create mode 100644 dataplane/luciusctl/internal/transport/transport.go create mode 100644 dataplane/luciusctl/internal/transport/transport_test.go create mode 100644 dataplane/luciusctl/luciusctl_test.go diff --git a/dataplane/docs/dataplanedebug.md b/dataplane/docs/dataplanedebug.md index 9168d7cef..b77c1a08d 100644 --- a/dataplane/docs/dataplanedebug.md +++ b/dataplane/docs/dataplanedebug.md @@ -9,6 +9,14 @@ The lemctl CLI contains useful tools for debugging Lemming and Lucius. +Live `lemctl lucius info` and `lemctl lucius sai` queries verify TLS certificates +by default. Use `--tls_ca_cert ca.pem` to add a private CA to system trust roots; +`--tls_skip_verify` is no longer supported. Plaintext `--insecure` requires a +literal loopback IP and port, such as `127.0.0.1:50000` or `[::1]:50000`, and +connects directly without a proxy. Hostnames, remote IPs, resolver targets, Unix +sockets and scoped IPv6 addresses are rejected for plaintext. `--tls_ca_cert` +cannot be combined with `--insecure`. + To enable packet traces, the lucius containers must be logging at level 3 or higher. Add the "-v=3" arg in the topology file for all lucius containers. diff --git a/dataplane/luciusctl/BUILD b/dataplane/luciusctl/BUILD index 502105d41..fed91ddf9 100644 --- a/dataplane/luciusctl/BUILD +++ b/dataplane/luciusctl/BUILD @@ -1,4 +1,4 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "luciusctl", @@ -12,3 +12,20 @@ go_library( "@com_github_spf13_viper//:viper", ], ) + +go_test( + name = "luciusctl_test", + size = "small", + srcs = ["luciusctl_test.go"], + embed = [":luciusctl"], + deps = [ + "//dataplane/proto/sai", + "//proto/forwarding", + "@com_github_spf13_cobra//:cobra", + "@com_github_spf13_viper//:viper", + "@org_golang_google_grpc//:grpc", + "@org_golang_google_grpc//credentials", + "@org_golang_google_grpc//peer", + "@org_golang_google_grpc//reflection", + ], +) diff --git a/dataplane/luciusctl/info/BUILD b/dataplane/luciusctl/info/BUILD index 8ccecc798..6d6e32a55 100644 --- a/dataplane/luciusctl/info/BUILD +++ b/dataplane/luciusctl/info/BUILD @@ -6,11 +6,8 @@ go_library( importpath = "github.com/openconfig/lemming/dataplane/luciusctl/info", visibility = ["//visibility:public"], deps = [ + "//dataplane/luciusctl/internal/transport", "//proto/forwarding", "@com_github_spf13_cobra//:cobra", - "@com_github_spf13_viper//:viper", - "@org_golang_google_grpc//:grpc", - "@org_golang_google_grpc//credentials", - "@org_golang_google_grpc//credentials/insecure", ], ) diff --git a/dataplane/luciusctl/info/info.go b/dataplane/luciusctl/info/info.go index 5efa9dc59..34ffc2342 100644 --- a/dataplane/luciusctl/info/info.go +++ b/dataplane/luciusctl/info/info.go @@ -16,15 +16,12 @@ package info import ( "context" - "crypto/tls" "encoding/hex" "fmt" "github.com/spf13/cobra" - "github.com/spf13/viper" - "google.golang.org/grpc" - "google.golang.org/grpc/credentials" - "google.golang.org/grpc/credentials/insecure" + + "github.com/openconfig/lemming/dataplane/luciusctl/internal/transport" fwdpb "github.com/openconfig/lemming/proto/forwarding" ) @@ -73,7 +70,7 @@ func New() *cobra.Command { } func listFn(cmd *cobra.Command, _ []string) error { - conn, err := dial() + conn, err := transport.Dial() if err != nil { return fmt.Errorf("failed to dial dataplane: %v", err) } @@ -92,7 +89,7 @@ func listFn(cmd *cobra.Command, _ []string) error { } func infoElement(ctx context.Context, name string, infoType fwdpb.InfoType, packet string) error { - conn, err := dial() + conn, err := transport.Dial() if err != nil { return fmt.Errorf("failed to dial dataplane: %v", err) } @@ -116,18 +113,3 @@ func infoElement(ctx context.Context, name string, infoType fwdpb.InfoType, pack fmt.Println(resp.GetContent()) return nil } - -func dial() (*grpc.ClientConn, error) { - insec, tlsSkipVerify := viper.GetBool("insecure"), viper.GetBool("tls_skip_verify") - if insec && tlsSkipVerify { - return nil, fmt.Errorf("both insecure and tls skip verify are set") - } - opts := grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{ - InsecureSkipVerify: tlsSkipVerify, // nolint:gosec - })) - if insec { - opts = grpc.WithTransportCredentials(insecure.NewCredentials()) - } - - return grpc.NewClient(viper.GetString("address"), opts) -} diff --git a/dataplane/luciusctl/internal/transport/BUILD b/dataplane/luciusctl/internal/transport/BUILD new file mode 100644 index 000000000..eb809b2df --- /dev/null +++ b/dataplane/luciusctl/internal/transport/BUILD @@ -0,0 +1,22 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "transport", + srcs = ["transport.go"], + importpath = "github.com/openconfig/lemming/dataplane/luciusctl/internal/transport", + visibility = ["//dataplane/luciusctl:__subpackages__"], + deps = [ + "@com_github_spf13_viper//:viper", + "@org_golang_google_grpc//:grpc", + "@org_golang_google_grpc//credentials", + "@org_golang_google_grpc//credentials/insecure", + ], +) + +go_test( + name = "transport_test", + size = "small", + srcs = ["transport_test.go"], + embed = [":transport"], + deps = ["@com_github_spf13_viper//:viper"], +) diff --git a/dataplane/luciusctl/internal/transport/transport.go b/dataplane/luciusctl/internal/transport/transport.go new file mode 100644 index 000000000..bcb3f1d91 --- /dev/null +++ b/dataplane/luciusctl/internal/transport/transport.go @@ -0,0 +1,94 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package transport provides verified TLS or local-only plaintext connections. +package transport + +import ( + "context" + "crypto/tls" + "crypto/x509" + "fmt" + "net" + "net/netip" + "os" + "strconv" + + "github.com/spf13/viper" + "google.golang.org/grpc" + "google.golang.org/grpc/credentials" + "google.golang.org/grpc/credentials/insecure" +) + +// Dial creates a channel using the configured diagnostic transport policy. +func Dial() (*grpc.ClientConn, error) { + if viper.GetBool("tls_skip_verify") { + return nil, fmt.Errorf("--tls_skip_verify is no longer supported; use --tls_ca_cert to trust the server's CA") + } + address := viper.GetString("address") + if address == "" { + return nil, fmt.Errorf("--address is required") + } + caFile := viper.GetString("tls_ca_cert") + if viper.GetBool("insecure") { + if caFile != "" { + return nil, fmt.Errorf("--tls_ca_cert cannot be used with --insecure") + } + address, err := loopbackAddress(address) + if err != nil { + return nil, err + } + // Pin every connection to the validated IP, bypassing resolvers and proxies. + dialer := &net.Dialer{} + return grpc.NewClient("passthrough:///"+address, + grpc.WithTransportCredentials(insecure.NewCredentials()), + grpc.WithContextDialer(func(ctx context.Context, _ string) (net.Conn, error) { + return dialer.DialContext(ctx, "tcp", address) + }), + ) + } + + config := &tls.Config{MinVersion: tls.VersionTLS12} + if caFile != "" { + certs, err := os.ReadFile(caFile) + if err != nil { + return nil, fmt.Errorf("read --tls_ca_cert: %w", err) + } + roots, err := x509.SystemCertPool() + if err != nil { + return nil, fmt.Errorf("load system certificate roots: %w", err) + } + if !roots.AppendCertsFromPEM(certs) { + return nil, fmt.Errorf("--tls_ca_cert contains no valid PEM certificates") + } + config.RootCAs = roots + } + return grpc.NewClient(address, grpc.WithTransportCredentials(credentials.NewTLS(config))) +} + +func loopbackAddress(address string) (string, error) { + host, port, err := net.SplitHostPort(address) + if err != nil { + return "", fmt.Errorf("--insecure requires a literal loopback IP and port: %w", err) + } + ip, err := netip.ParseAddr(host) + if err != nil || ip.Zone() != "" || !ip.Unmap().IsLoopback() { + return "", fmt.Errorf("--insecure requires a literal loopback IP; hostnames, remote IPs and resolver targets are not supported") + } + number, err := strconv.ParseUint(port, 10, 16) + if err != nil || number == 0 { + return "", fmt.Errorf("--insecure requires a port between 1 and 65535") + } + return net.JoinHostPort(ip.Unmap().String(), strconv.FormatUint(number, 10)), nil +} diff --git a/dataplane/luciusctl/internal/transport/transport_test.go b/dataplane/luciusctl/internal/transport/transport_test.go new file mode 100644 index 000000000..ec5c41356 --- /dev/null +++ b/dataplane/luciusctl/internal/transport/transport_test.go @@ -0,0 +1,115 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package transport + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spf13/viper" +) + +func TestLoopbackAddress(t *testing.T) { + tests := []struct { + address string + want string + }{ + {address: "127.0.0.1:50000", want: "127.0.0.1:50000"}, + {address: "127.255.255.254:1", want: "127.255.255.254:1"}, + {address: "[::1]:65535", want: "[::1]:65535"}, + {address: "[0:0:0:0:0:0:0:1]:05000", want: "[::1]:5000"}, + {address: "[::ffff:127.0.0.1]:50000", want: "127.0.0.1:50000"}, + {address: "localhost:50000"}, + {address: "localhost.:50000"}, + {address: "127.0.0.1.example:50000"}, + {address: "192.0.2.1:50000"}, + {address: "10.0.0.1:50000"}, + {address: "0.0.0.0:50000"}, + {address: "[::]:50000"}, + {address: "[2001:db8::1]:50000"}, + {address: "[::ffff:192.0.2.1]:50000"}, + {address: "[::1%lo]:50000"}, + {address: "dns:///127.0.0.1:50000"}, + {address: "passthrough:///127.0.0.1:50000"}, + {address: "unix:///tmp/lucius.sock"}, + {address: "http://127.0.0.1:50000"}, + {address: "127.1:50000"}, + {address: "2130706433:50000"}, + {address: "0177.0.0.1:50000"}, + {address: "127.0.0.1"}, + {address: "::1:50000"}, + {address: "127.0.0.1:0"}, + {address: "127.0.0.1:65536"}, + {address: "127.0.0.1:http"}, + {address: "127.0.0.1:+1"}, + {address: "127.0.0.1:-1"}, + {address: "127.0.0.1:"}, + {address: ""}, + } + for _, test := range tests { + t.Run(test.address, func(t *testing.T) { + got, err := loopbackAddress(test.address) + if test.want == "" { + if err == nil { + t.Fatalf("accepted prohibited plaintext target %q as %q", test.address, got) + } + return + } + if err != nil || got != test.want { + t.Fatalf("loopbackAddress(%q) = %q, %v; want %q", test.address, got, err, test.want) + } + }) + } +} + +func TestDialConfigurationErrors(t *testing.T) { + dir := t.TempDir() + invalidCA := filepath.Join(dir, "invalid.pem") + if err := os.WriteFile(invalidCA, []byte("not a certificate"), 0600); err != nil { + t.Fatal(err) + } + tests := []struct { + name string + values map[string]any + wantErr string + }{ + {name: "missing-address", wantErr: "--address is required"}, + {name: "legacy-bypass", values: map[string]any{"tls_skip_verify": true}, wantErr: "no longer supported"}, + {name: "both-options", values: map[string]any{"tls_skip_verify": true, "insecure": true}, wantErr: "no longer supported"}, + {name: "remote-plaintext", values: map[string]any{"address": "192.0.2.1:50000", "insecure": true}, wantErr: "literal loopback"}, + {name: "ca-with-plaintext", values: map[string]any{"address": "127.0.0.1:50000", "insecure": true, "tls_ca_cert": invalidCA}, wantErr: "cannot be used"}, + {name: "missing-ca", values: map[string]any{"address": "dataplane.example:50000", "tls_ca_cert": filepath.Join(dir, "missing.pem")}, wantErr: "read --tls_ca_cert"}, + {name: "invalid-ca", values: map[string]any{"address": "dataplane.example:50000", "tls_ca_cert": invalidCA}, wantErr: "no valid PEM certificates"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + viper.Reset() + t.Cleanup(viper.Reset) + for key, value := range test.values { + viper.Set(key, value) + } + conn, err := Dial() + if conn != nil { + conn.Close() + t.Fatal("invalid configuration created a connection") + } + if err == nil || !strings.Contains(err.Error(), test.wantErr) { + t.Fatalf("Dial() error = %v; want %q", err, test.wantErr) + } + }) + } +} diff --git a/dataplane/luciusctl/luciusctl.go b/dataplane/luciusctl/luciusctl.go index 1263df088..824b4b316 100644 --- a/dataplane/luciusctl/luciusctl.go +++ b/dataplane/luciusctl/luciusctl.go @@ -28,8 +28,9 @@ func New() *cobra.Command { Short: "lucius queries info from a running instance of lucius.", } cmd.PersistentFlags().StringP("address", "a", "", "Address of dataplane server") - cmd.PersistentFlags().Bool("insecure", false, "Use plaintext to dial server") - cmd.PersistentFlags().Bool("tls_skip_verify", false, "Use TLS without cert verification") + cmd.PersistentFlags().Bool("insecure", false, "Use plaintext only for a literal loopback IP and port") + cmd.PersistentFlags().Bool("tls_skip_verify", false, "Unsupported: use --tls_ca_cert instead") + cmd.PersistentFlags().String("tls_ca_cert", "", "PEM CA bundle to trust in addition to system certificate roots") cobra.OnInitialize(func() { viper.BindPFlags(cmd.Flags()) }) viper.BindPFlags(cmd.Flags()) diff --git a/dataplane/luciusctl/luciusctl_test.go b/dataplane/luciusctl/luciusctl_test.go new file mode 100644 index 000000000..3154d5c5f --- /dev/null +++ b/dataplane/luciusctl/luciusctl_test.go @@ -0,0 +1,324 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package luciusctl + +import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "io" + "math/big" + "net" + "os" + "path/filepath" + "slices" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/spf13/cobra" + "github.com/spf13/viper" + "google.golang.org/grpc" + "google.golang.org/grpc/credentials" + "google.golang.org/grpc/peer" + "google.golang.org/grpc/reflection" + + saipb "github.com/openconfig/lemming/dataplane/proto/sai" + fwdpb "github.com/openconfig/lemming/proto/forwarding" +) + +type diagnosticServer struct { + fwdpb.UnimplementedInfoServer + saipb.UnimplementedSwitchServer + calls atomic.Int32 + tlsCalls atomic.Int32 + streams atomic.Int32 +} + +func (s *diagnosticServer) record(ctx context.Context) { + s.calls.Add(1) + if p, ok := peer.FromContext(ctx); ok { + if _, ok := p.AuthInfo.(credentials.TLSInfo); ok { + s.tlsCalls.Add(1) + } + } +} + +func (s *diagnosticServer) InfoList(ctx context.Context, _ *fwdpb.InfoListRequest) (*fwdpb.InfoListReply, error) { + s.record(ctx) + return &fwdpb.InfoListReply{Names: []string{"synthetic-object"}}, nil +} + +func (s *diagnosticServer) InfoElement(ctx context.Context, _ *fwdpb.InfoElementRequest) (*fwdpb.InfoElementReply, error) { + s.record(ctx) + return &fwdpb.InfoElementReply{Content: "synthetic-content"}, nil +} + +func (s *diagnosticServer) GetSwitchAttribute(ctx context.Context, _ *saipb.GetSwitchAttributeRequest) (*saipb.GetSwitchAttributeResponse, error) { + s.record(ctx) + return &saipb.GetSwitchAttributeResponse{}, nil +} + +func serveDiagnostics(t *testing.T, host string, cert *tls.Certificate) (string, *diagnosticServer) { + t.Helper() + listener, err := net.Listen("tcp", net.JoinHostPort(host, "0")) + if err != nil { + t.Fatal(err) + } + service := &diagnosticServer{} + opts := []grpc.ServerOption{grpc.StreamInterceptor(func(srv any, stream grpc.ServerStream, _ *grpc.StreamServerInfo, handler grpc.StreamHandler) error { + service.streams.Add(1) + return handler(srv, stream) + })} + if cert != nil { + opts = append(opts, grpc.Creds(credentials.NewTLS(&tls.Config{ + Certificates: []tls.Certificate{*cert}, + MinVersion: tls.VersionTLS12, + }))) + } + server := grpc.NewServer(opts...) + fwdpb.RegisterInfoServer(server, service) + saipb.RegisterSwitchServer(server, service) + reflection.Register(server) + done := make(chan error, 1) + go func() { done <- server.Serve(listener) }() + t.Cleanup(func() { + server.Stop() + if err := <-done; err != nil && err != grpc.ErrServerStopped { + t.Errorf("test server: %v", err) + } + }) + return listener.Addr().String(), service +} + +func diagnosticCertificates(t *testing.T) (string, map[string]tls.Certificate) { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatal(err) + } + now := time.Now() + root := &x509.Certificate{ + SerialNumber: big.NewInt(1), Subject: pkix.Name{CommonName: "Disposable diagnostic test CA"}, + NotBefore: now.Add(-48 * time.Hour), NotAfter: now.Add(24 * time.Hour), + IsCA: true, BasicConstraintsValid: true, KeyUsage: x509.KeyUsageCertSign, + } + der, err := x509.CreateCertificate(rand.Reader, root, root, &key.PublicKey, key) + if err != nil { + t.Fatal(err) + } + caFile := filepath.Join(t.TempDir(), "ca.pem") + if err := os.WriteFile(caFile, pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}), 0600); err != nil { + t.Fatal(err) + } + certs := make(map[string]tls.Certificate) + for index, name := range []string{"valid", "self-signed", "expired", "wrong-name"} { + leafKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatal(err) + } + leaf := &x509.Certificate{ + SerialNumber: big.NewInt(int64(index + 2)), + NotBefore: now.Add(-time.Hour), NotAfter: now.Add(time.Hour), + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + KeyUsage: x509.KeyUsageDigitalSignature, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("::1")}, + } + parent, signer := root, key + switch name { + case "self-signed": + parent, signer = leaf, leafKey + case "expired": + leaf.NotBefore, leaf.NotAfter = now.Add(-24*time.Hour), now.Add(-time.Hour) + case "wrong-name": + leaf.IPAddresses = nil + leaf.DNSNames = []string{"wrong-name.invalid"} + } + der, err := x509.CreateCertificate(rand.Reader, leaf, parent, &leafKey.PublicKey, signer) + if err != nil { + t.Fatal(err) + } + certs[name] = tls.Certificate{Certificate: [][]byte{der}, PrivateKey: leafKey} + } + return caFile, certs +} + +func TestDiagnosticTransports(t *testing.T) { + caFile, certs := diagnosticCertificates(t) + tests := []struct { + name string + cert string + flags []string + target string + wantErr string + }{ + {name: "trusted-tls", cert: "valid", flags: []string{"--tls_ca_cert", caFile}}, + {name: "resolver-trusted-tls", cert: "valid", flags: []string{"--tls_ca_cert", caFile}, target: "dns:///"}, + {name: "unknown-ca", cert: "valid", wantErr: "certificate signed by unknown authority"}, + {name: "self-signed", cert: "self-signed", flags: []string{"--tls_ca_cert", caFile}, wantErr: "certificate signed by unknown authority"}, + {name: "expired", cert: "expired", flags: []string{"--tls_ca_cert", caFile}, wantErr: "certificate has expired"}, + {name: "wrong-name", cert: "wrong-name", flags: []string{"--tls_ca_cert", caFile}, wantErr: "IP SANs"}, + {name: "self-signed-bypass-rejected", cert: "self-signed", flags: []string{"--tls_skip_verify"}, wantErr: "no longer supported"}, + {name: "expired-bypass-rejected", cert: "expired", flags: []string{"--tls_skip_verify"}, wantErr: "no longer supported"}, + {name: "wrong-name-bypass-rejected", cert: "wrong-name", flags: []string{"--tls_skip_verify"}, wantErr: "no longer supported"}, + {name: "loopback-plaintext", flags: []string{"--insecure"}}, + {name: "plaintext-with-default-tls", wantErr: "TLS handshake"}, + {name: "both-options", flags: []string{"--insecure", "--tls_skip_verify"}, wantErr: "no longer supported"}, + {name: "resolver-plaintext", flags: []string{"--insecure"}, target: "dns:///", wantErr: "literal loopback"}, + {name: "hostname-plaintext", flags: []string{"--insecure"}, target: "localhost", wantErr: "literal loopback"}, + {name: "remote-plaintext", flags: []string{"--insecure"}, target: "192.0.2.1", wantErr: "literal loopback"}, + {name: "ca-with-plaintext", flags: []string{"--insecure", "--tls_ca_cert", caFile}, wantErr: "cannot be used"}, + } + commands := [][]string{ + {"info", "list"}, + {"info", "get", "synthetic-object"}, + {"info", "lookup", "synthetic-object", "00"}, + {"info", "port-input", "synthetic-object", "00"}, + {"info", "port-output", "synthetic-object", "00"}, + {"sai", "get-attr", "switch", "1", "cpu_port"}, + } + for _, command := range commands { + for _, test := range tests { + t.Run(strings.Join(command[:2], "-")+"/"+test.name, func(t *testing.T) { + viper.Reset() + t.Cleanup(viper.Reset) + var cert *tls.Certificate + if test.cert != "" { + value := certs[test.cert] + cert = &value + } + address, server := serveDiagnostics(t, "127.0.0.1", cert) + switch test.target { + case "dns:///": + address = test.target + address + case "localhost", "192.0.2.1": + _, port, err := net.SplitHostPort(address) + if err != nil { + t.Fatal(err) + } + address = net.JoinHostPort(test.target, port) + } + cmd := New() + cmd.SilenceUsage, cmd.SilenceErrors = true, true + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + args := append([]string{"--address", address}, test.flags...) + cmd.SetArgs(append(args, command...)) + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + err := cmd.ExecuteContext(ctx) + if test.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), test.wantErr) || server.calls.Load() != 0 || server.streams.Load() != 0 { + t.Fatalf("expected %q before RPC; err=%v calls=%d streams=%d", test.wantErr, err, server.calls.Load(), server.streams.Load()) + } + + return + } + if err != nil || server.calls.Load() != 1 { + t.Fatalf("expected completed diagnostic RPC; err=%v calls=%d", err, server.calls.Load()) + } + wantTLS := int32(0) + if test.cert != "" { + wantTLS = 1 + } + if server.tlsCalls.Load() != wantTLS { + t.Fatalf("TLS RPC count = %d, want %d", server.tlsCalls.Load(), wantTLS) + } + }) + } + } +} + +func TestLoopbackPlaintextBypassesProxy(t *testing.T) { + for _, host := range []string{"127.0.0.1", "::1"} { + t.Run(host, func(t *testing.T) { + viper.Reset() + t.Cleanup(viper.Reset) + address, server := serveDiagnostics(t, host, nil) + proxy, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { proxy.Close() }) + t.Setenv("HTTPS_PROXY", "http://"+proxy.Addr().String()) + t.Setenv("NO_PROXY", "") + viper.Set("address", address) + viper.Set("insecure", true) + cmd := New() + cmd.SetArgs([]string{"info", "list"}) + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + if err := cmd.ExecuteContext(ctx); err != nil || server.calls.Load() != 1 { + t.Fatalf("direct local RPC failed: err=%v calls=%d", err, server.calls.Load()) + } + if err := proxy.(*net.TCPListener).SetDeadline(time.Now().Add(20 * time.Millisecond)); err != nil { + t.Fatal(err) + } + conn, err := proxy.Accept() + if err == nil { + conn.Close() + t.Fatal("plaintext connection reached proxy") + } + if err, ok := err.(net.Error); !ok || !err.Timeout() { + t.Fatalf("unexpected proxy listener error: %v", err) + } + }) + } +} + +func TestSAICompletionTransportPolicy(t *testing.T) { + tests := []struct { + name string + target string + skipVerify bool + wantError bool + }{ + {name: "local-plaintext"}, + {name: "legacy-bypass", skipVerify: true, wantError: true}, + {name: "remote-plaintext", target: "192.0.2.1:50000", wantError: true}, + {name: "resolver-plaintext", target: "dns:///127.0.0.1:50000", wantError: true}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + viper.Reset() + t.Cleanup(viper.Reset) + address, server := serveDiagnostics(t, "127.0.0.1", nil) + if test.target != "" { + address = test.target + } + viper.Set("address", address) + viper.Set("insecure", true) + viper.Set("tls_skip_verify", test.skipVerify) + cmd, _, err := New().Find([]string{"sai", "get-attr"}) + if err != nil { + t.Fatal(err) + } + values, directive := cmd.ValidArgsFunction(cmd, nil, "") + if test.wantError { + if directive&cobra.ShellCompDirectiveError == 0 || server.streams.Load() != 0 { + t.Fatalf("expected completion error before reflection; directive=%v streams=%d", directive, server.streams.Load()) + } + } else if directive&cobra.ShellCompDirectiveError != 0 || !slices.Contains(values, "switch") || server.streams.Load() != 1 { + t.Fatalf("local completion failed: values=%v directive=%v streams=%d", values, directive, server.streams.Load()) + } + }) + } +} diff --git a/dataplane/luciusctl/sai/BUILD b/dataplane/luciusctl/sai/BUILD index 6275ad7c8..ebc4f34ed 100644 --- a/dataplane/luciusctl/sai/BUILD +++ b/dataplane/luciusctl/sai/BUILD @@ -9,11 +9,8 @@ go_library( importpath = "github.com/openconfig/lemming/dataplane/luciusctl/sai", visibility = ["//visibility:public"], deps = [ + "//dataplane/luciusctl/internal/transport", "@com_github_spf13_cobra//:cobra", - "@com_github_spf13_viper//:viper", - "@org_golang_google_grpc//:grpc", - "@org_golang_google_grpc//credentials", - "@org_golang_google_grpc//credentials/insecure", "@org_golang_google_grpc//reflection/grpc_reflection_v1", "@org_golang_google_protobuf//encoding/prototext", "@org_golang_google_protobuf//proto", diff --git a/dataplane/luciusctl/sai/sai.go b/dataplane/luciusctl/sai/sai.go index a19ed395d..655edee4f 100644 --- a/dataplane/luciusctl/sai/sai.go +++ b/dataplane/luciusctl/sai/sai.go @@ -15,21 +15,18 @@ package sai import ( - "crypto/tls" "fmt" "slices" "strconv" "strings" "github.com/spf13/cobra" - "github.com/spf13/viper" - "google.golang.org/grpc" - "google.golang.org/grpc/credentials" - "google.golang.org/grpc/credentials/insecure" reflectpb "google.golang.org/grpc/reflection/grpc_reflection_v1" "google.golang.org/protobuf/encoding/prototext" "google.golang.org/protobuf/reflect/protoreflect" "google.golang.org/protobuf/types/dynamicpb" + + "github.com/openconfig/lemming/dataplane/luciusctl/internal/transport" ) func New() *cobra.Command { @@ -50,7 +47,7 @@ Examples: `, Aliases: []string{"get-attr"}, RunE: func(cmd *cobra.Command, args []string) error { - conn, err := dial() + conn, err := transport.Dial() if err != nil { return err } @@ -89,7 +86,7 @@ Examples: return nil }, ValidArgsFunction: func(_ *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { - conn, err := dial() + conn, err := transport.Dial() if err != nil { return nil, cobra.ShellCompDirectiveError } @@ -128,18 +125,3 @@ func keyPrefixMatch[T any](method map[string]T, prefix string) []string { slices.Sort(matches) return matches } - -func dial() (*grpc.ClientConn, error) { - insec, tlsSkipVerify := viper.GetBool("insecure"), viper.GetBool("tls_skip_verify") - if insec && tlsSkipVerify { - return nil, fmt.Errorf("both insecure and tls skip verify are set") - } - opts := grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{ - InsecureSkipVerify: tlsSkipVerify, // nolint:gosec - })) - if insec { - opts = grpc.WithTransportCredentials(insecure.NewCredentials()) - } - - return grpc.NewClient(viper.GetString("address"), opts) -} From f99127a2ed3e6c0ec06f29d875a55ec31674d6aa Mon Sep 17 00:00:00 2001 From: Baiju Parameswaran Date: Tue, 13 Oct 2026 14:21:30 +0000 Subject: [PATCH 2/3] [luciusctl]: Attribute new files to Microsoft Signed-off-by: Baiju Parameswaran Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: db2e0f10-3729-4d5d-b4da-3654eb856d74 --- dataplane/luciusctl/internal/transport/transport.go | 2 +- dataplane/luciusctl/internal/transport/transport_test.go | 2 +- dataplane/luciusctl/luciusctl_test.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dataplane/luciusctl/internal/transport/transport.go b/dataplane/luciusctl/internal/transport/transport.go index bcb3f1d91..fba2b0790 100644 --- a/dataplane/luciusctl/internal/transport/transport.go +++ b/dataplane/luciusctl/internal/transport/transport.go @@ -1,4 +1,4 @@ -// Copyright 2026 Google LLC +// Copyright 2026 Microsoft Corporation // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/dataplane/luciusctl/internal/transport/transport_test.go b/dataplane/luciusctl/internal/transport/transport_test.go index ec5c41356..82a199eb2 100644 --- a/dataplane/luciusctl/internal/transport/transport_test.go +++ b/dataplane/luciusctl/internal/transport/transport_test.go @@ -1,4 +1,4 @@ -// Copyright 2026 Google LLC +// Copyright 2026 Microsoft Corporation // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/dataplane/luciusctl/luciusctl_test.go b/dataplane/luciusctl/luciusctl_test.go index 3154d5c5f..3f4c1440f 100644 --- a/dataplane/luciusctl/luciusctl_test.go +++ b/dataplane/luciusctl/luciusctl_test.go @@ -1,4 +1,4 @@ -// Copyright 2026 Google LLC +// Copyright 2026 Microsoft Corporation // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. From 8b2dfc0ebb364169eaa052868c75a8555864484e Mon Sep 17 00:00:00 2001 From: Baiju Parameswaran Date: Thu, 22 Oct 2026 18:39:15 +0000 Subject: [PATCH 3/3] Remove inherited test dependencies from Bazel targets Reuse the Cobra and Viper dependencies supplied by the embedded libraries instead of declaring them again on the internal test targets. Keep only these dependency attributes explicit so Gazelle does not recreate the redundant entries. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: db2e0f10-3729-4d5d-b4da-3654eb856d74 Signed-off-by: Baiju Parameswaran --- dataplane/luciusctl/BUILD | 3 +-- dataplane/luciusctl/internal/transport/BUILD | 3 ++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dataplane/luciusctl/BUILD b/dataplane/luciusctl/BUILD index fed91ddf9..6f2cfefc5 100644 --- a/dataplane/luciusctl/BUILD +++ b/dataplane/luciusctl/BUILD @@ -18,11 +18,10 @@ go_test( size = "small", srcs = ["luciusctl_test.go"], embed = [":luciusctl"], + # keep: Cobra and Viper are inherited from :luciusctl. deps = [ "//dataplane/proto/sai", "//proto/forwarding", - "@com_github_spf13_cobra//:cobra", - "@com_github_spf13_viper//:viper", "@org_golang_google_grpc//:grpc", "@org_golang_google_grpc//credentials", "@org_golang_google_grpc//peer", diff --git a/dataplane/luciusctl/internal/transport/BUILD b/dataplane/luciusctl/internal/transport/BUILD index eb809b2df..e172db039 100644 --- a/dataplane/luciusctl/internal/transport/BUILD +++ b/dataplane/luciusctl/internal/transport/BUILD @@ -18,5 +18,6 @@ go_test( size = "small", srcs = ["transport_test.go"], embed = [":transport"], - deps = ["@com_github_spf13_viper//:viper"], + # keep: Viper is inherited from :transport. + deps = [], )