From d14c2fd5000b51c5808ca3413d41a0954b25ad09 Mon Sep 17 00:00:00 2001 From: Nabendu Maiti Date: Sat, 1 Aug 2026 08:33:44 +0000 Subject: [PATCH] fix(cira): require opt-in for non-forward-secret ciphers The CIRA listener unconditionally advertised three RSA key-exchange suites for older AMT firmware: TLS_RSA_WITH_AES_128_GCM_SHA256 TLS_RSA_WITH_AES_128_CBC_SHA TLS_RSA_WITH_AES_256_CBC_SHA All three lack perfect forward secrecy, so a default install shipped a weakened TLS posture on :4433 even when every enrolled device could negotiate ECDHE. Gate them behind the existing allow_insecure_ciphers setting (APP_ALLOW_INSECURE_CIPHERS, already false by default), which governs the same trade-off on outbound WSMAN connections. The listener now offers only Go's secure defaults unless an operator opts in, and logs a warning at startup when they do. Behaviour change: deployments that depend on the RSA suites must set APP_ALLOW_INSECURE_CIPHERS=true. Signed-off-by: Nabendu Maiti --- .env.example | 3 + config/config.yml | 3 + internal/app/app.go | 2 +- internal/controller/tcp/cira/tunnel.go | 79 +++++++++++++-------- internal/controller/tcp/cira/tunnel_test.go | 51 +++++++++++++ 5 files changed, 108 insertions(+), 30 deletions(-) diff --git a/.env.example b/.env.example index 53c0459b5..a52066180 100644 --- a/.env.example +++ b/.env.example @@ -7,6 +7,9 @@ APP_REPO=device-management-toolkit/console # AES-192 or AES-256) and must not be repetitive — Console refuses to start # otherwise. Generate one with: openssl rand -base64 24 APP_ENCRYPTION_KEY= +# Offers non-forward-secret RSA cipher suites to older AMT generations, both on +# the CIRA listener (:4433) and on outbound WSMAN connections. Leave false +# unless a device fails to negotiate TLS without them. APP_ALLOW_INSECURE_CIPHERS=false APP_COMMON_NAME=console.local APP_DISABLE_CIRA=true diff --git a/config/config.yml b/config/config.yml index 782bd309e..957407d85 100644 --- a/config/config.yml +++ b/config/config.yml @@ -3,6 +3,9 @@ app: repo: device-management-toolkit/console version: DEVELOPMENT encryption_key: "" + # Offers non-forward-secret RSA cipher suites to older AMT generations, both + # on the CIRA listener (:4433) and on outbound WSMAN connections. Leave false + # unless a device fails to negotiate TLS without them. allow_insecure_ciphers: false http: host: localhost diff --git a/internal/app/app.go b/internal/app/app.go index 2c48620b5..e74d140ea 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -122,7 +122,7 @@ func setupCIRAServer(cfg *config.Config, log logger.Interface, closer io.Closer, ciraCertFile := fmt.Sprintf("config/%s_cert.pem", cfg.CommonName) ciraKeyFile := fmt.Sprintf("config/%s_key.pem", cfg.CommonName) - ciraServer, err := cira.NewServer(ciraCertFile, ciraKeyFile, usecases.Devices, log) + ciraServer, err := cira.NewServer(ciraCertFile, ciraKeyFile, cfg.AllowInsecureCiphers, usecases.Devices, log) if err != nil { _ = closer.Close() diff --git a/internal/controller/tcp/cira/tunnel.go b/internal/controller/tcp/cira/tunnel.go index ebf23b8ec..8f20991eb 100644 --- a/internal/controller/tcp/cira/tunnel.go +++ b/internal/controller/tcp/cira/tunnel.go @@ -22,37 +22,47 @@ import ( ) const ( - maxIdleTime = 300 * time.Second - port = "4433" - readBufferSize = 4096 - weakCipherSuiteCount = 3 - keepAliveInterval = 30 - keepAliveTimeout = 90 - apfSessionTimeout = 3 * time.Second + maxIdleTime = 300 * time.Second + port = "4433" + readBufferSize = 4096 + keepAliveInterval = 30 + keepAliveTimeout = 90 + apfSessionTimeout = 3 * time.Second ) // ErrChannelOpenFailed is returned when an APF channel open request fails. var ErrChannelOpenFailed = errors.New("channel open failed") +// insecureCipherSuites are RSA key-exchange suites that some older AMT +// generations require. They lack perfect forward secrecy, so they are only +// offered when the operator opts in via APP_ALLOW_INSECURE_CIPHERS. +var insecureCipherSuites = []uint16{ + tls.TLS_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_RSA_WITH_AES_128_CBC_SHA, + tls.TLS_RSA_WITH_AES_256_CBC_SHA, +} + type Server struct { - certificates tls.Certificate - notify chan error - listener net.Listener - devices devices.Feature - log logger.Interface + certificates tls.Certificate + notify chan error + listener net.Listener + devices devices.Feature + log logger.Interface + allowInsecureCiphers bool } -func NewServer(certFile, keyFile string, d devices.Feature, l logger.Interface) (*Server, error) { +func NewServer(certFile, keyFile string, allowInsecureCiphers bool, d devices.Feature, l logger.Interface) (*Server, error) { cert, err := tls.LoadX509KeyPair(certFile, keyFile) if err != nil { return nil, err } s := &Server{ - certificates: cert, - notify: make(chan error, 1), - devices: d, - log: l, + certificates: cert, + notify: make(chan error, 1), + devices: d, + log: l, + allowInsecureCiphers: allowInsecureCiphers, } s.start() @@ -73,6 +83,28 @@ func (s *Server) Notify() <-chan error { return s.notify } +// cipherSuites returns the TLS 1.2 cipher suites the listener offers to AMT +// devices. Only Go's secure defaults are advertised unless the operator has +// explicitly enabled the non-forward-secret RSA suites needed by older AMT +// firmware. TLS 1.3 suites are not configurable and are always negotiated. +func (s *Server) cipherSuites() []uint16 { + defaultCipherSuites := tls.CipherSuites() + suites := make([]uint16, 0, len(defaultCipherSuites)+len(insecureCipherSuites)) + + for _, suite := range defaultCipherSuites { + suites = append(suites, suite.ID) + } + + if !s.allowInsecureCiphers { + return suites + } + + s.log.Warn("CIRA server - insecure cipher suites enabled for AMT compatibility; " + + "these lack perfect forward secrecy. Disable APP_ALLOW_INSECURE_CIPHERS unless required.") + + return append(suites, insecureCipherSuites...) +} + func (s *Server) ListenAndServe() error { config := &tls.Config{ Certificates: []tls.Certificate{s.certificates}, @@ -81,18 +113,7 @@ func (s *Server) ListenAndServe() error { MinVersion: tls.VersionTLS12, } - defaultCipherSuites := tls.CipherSuites() - config.CipherSuites = make([]uint16, 0, len(defaultCipherSuites)+weakCipherSuiteCount) - - for _, suite := range defaultCipherSuites { - config.CipherSuites = append(config.CipherSuites, suite.ID) - } - // add the weak cipher suites for AMT device compatibility - config.CipherSuites = append(config.CipherSuites, - tls.TLS_RSA_WITH_AES_128_GCM_SHA256, - tls.TLS_RSA_WITH_AES_128_CBC_SHA, - tls.TLS_RSA_WITH_AES_256_CBC_SHA, - ) + config.CipherSuites = s.cipherSuites() listener, err := tls.Listen("tcp", ":"+port, config) if err != nil { diff --git a/internal/controller/tcp/cira/tunnel_test.go b/internal/controller/tcp/cira/tunnel_test.go index 6d299e47e..625b1c1c0 100644 --- a/internal/controller/tcp/cira/tunnel_test.go +++ b/internal/controller/tcp/cira/tunnel_test.go @@ -1,6 +1,7 @@ package cira import ( + "crypto/tls" "errors" "net" "testing" @@ -222,6 +223,56 @@ func verifyConnectionRemoved(t *testing.T, authenticated bool, deviceID string) // fakeConn is a minimal net.Conn implementation for tests. type fakeConn struct{ net.Conn } +func TestServer_cipherSuites(t *testing.T) { + t.Parallel() + + secureIDs := make([]uint16, 0, len(tls.CipherSuites())) + for _, suite := range tls.CipherSuites() { + secureIDs = append(secureIDs, suite.ID) + } + + t.Run("insecure suites are not advertised by default", func(t *testing.T) { + t.Parallel() + + s := &Server{log: logger.New("error")} + + suites := s.cipherSuites() + + assert.Equal(t, secureIDs, suites) + + for _, insecure := range insecureCipherSuites { + assert.NotContains(t, suites, insecure, "insecure suite must be opt-in") + } + }) + + t.Run("insecure suites are advertised when explicitly allowed", func(t *testing.T) { + t.Parallel() + + s := &Server{log: logger.New("error"), allowInsecureCiphers: true} + + suites := s.cipherSuites() + + want := make([]uint16, 0, len(secureIDs)+len(insecureCipherSuites)) + want = append(want, secureIDs...) + want = append(want, insecureCipherSuites...) + + assert.Equal(t, want, suites) + }) + + t.Run("every opt-in suite is one Go considers insecure", func(t *testing.T) { + t.Parallel() + + goInsecure := make([]uint16, 0, len(tls.InsecureCipherSuites())) + for _, suite := range tls.InsecureCipherSuites() { + goInsecure = append(goInsecure, suite.ID) + } + + for _, suite := range insecureCipherSuites { + assert.Contains(t, goInsecure, suite) + } + }) +} + func TestConnectionContext_registerDevice(t *testing.T) { t.Parallel()