From 491853dd52ea1f035fbd36b846d204ecfac85d36 Mon Sep 17 00:00:00 2001 From: Dan Kortschak Date: Wed, 28 Jun 2023 08:02:16 +0930 Subject: [PATCH 1/4] packetbeat/beater: make Npcap installation lazy In the fleet-managed case New is called without a complete config, which means that we have no stream to tell us not to install the Npcap DLL. Defer installation until we have a valid config with streams configured so that we can determine whether the user has blocked install and whether we need the DLL. --- CHANGELOG.next.asciidoc | 1 + packetbeat/beater/install_npcap.go | 23 ++++++++++++++++++----- packetbeat/beater/install_npcap_test.go | 2 +- packetbeat/beater/packetbeat.go | 8 -------- packetbeat/beater/processor.go | 18 ++++++++++++++++++ 5 files changed, 38 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index 0931883bb13..8dcd1e9aed3 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -185,6 +185,7 @@ automatic splitting at root level, if root level element is an array. {pull}3415 *Packetbeat* +- Fix handling of Npcap installation options from Fleet. {pull}35541[35541] {pull}35935[35935] *Winlogbeat* diff --git a/packetbeat/beater/install_npcap.go b/packetbeat/beater/install_npcap.go index 370f7712e97..3363baccf1d 100644 --- a/packetbeat/beater/install_npcap.go +++ b/packetbeat/beater/install_npcap.go @@ -23,16 +23,22 @@ import ( "os" "path/filepath" "runtime" + "sync" "time" "github.com/elastic/beats/v7/libbeat/beat" "github.com/elastic/beats/v7/packetbeat/npcap" + conf "github.com/elastic/elastic-agent-libs/config" "github.com/elastic/elastic-agent-libs/logp" ) const installTimeout = 120 * time.Second -func installNpcap(b *beat.Beat) error { +// muInstall protects use of npcap.Installer. The only writes to npcap.Installer +// are here and during init in x-pack/packetbeat/npcap/npcap_windows.go +var muInstall sync.Mutex + +func installNpcap(b *beat.Beat, cfg *conf.C) error { if !b.Info.ElasticLicensed { return nil } @@ -54,7 +60,7 @@ func installNpcap(b *beat.Beat) error { return nil } - canInstall, err := canInstallNpcap(b) + canInstall, err := canInstallNpcap(b, cfg) if err != nil { return err } @@ -67,6 +73,8 @@ func installNpcap(b *beat.Beat) error { ctx, cancel := context.WithTimeout(context.Background(), installTimeout) defer cancel() + muInstall.Lock() + defer muInstall.Unlock() if npcap.Installer == nil { return nil } @@ -95,7 +103,7 @@ func installNpcap(b *beat.Beat) error { // configurations from agent normalised to the internal packetbeat format by this point. // In the case that the beat is managed, any data stream that has npcap.never_install // set to true will result in a block on the installation. -func canInstallNpcap(b *beat.Beat) (bool, error) { +func canInstallNpcap(b *beat.Beat, rawcfg *conf.C) (bool, error) { type npcapInstallCfg struct { NeverInstall bool `config:"npcap.never_install"` } @@ -105,10 +113,15 @@ func canInstallNpcap(b *beat.Beat) (bool, error) { var cfg struct { Streams []npcapInstallCfg `config:"streams"` } - err := b.BeatConfig.Unpack(&cfg) + err := rawcfg.Unpack(&cfg) if err != nil { return false, fmt.Errorf("failed to unpack npcap config from agent configuration: %w", err) } + if len(cfg.Streams) == 0 { + // We have no stream to monitor, so we don't need to install + // anything. We may be in the middle of a config check. + return false, nil + } for _, c := range cfg.Streams { if c.NeverInstall { return false, nil @@ -119,7 +132,7 @@ func canInstallNpcap(b *beat.Beat) (bool, error) { // Packetbeat case. var cfg npcapInstallCfg - err := b.BeatConfig.Unpack(&cfg) + err := rawcfg.Unpack(&cfg) if err != nil { return false, fmt.Errorf("failed to unpack npcap config from packetbeat configuration: %w", err) } diff --git a/packetbeat/beater/install_npcap_test.go b/packetbeat/beater/install_npcap_test.go index 7a888b4e0c8..0118a448b62 100644 --- a/packetbeat/beater/install_npcap_test.go +++ b/packetbeat/beater/install_npcap_test.go @@ -213,7 +213,7 @@ func TestCanInstallNpcap(t *testing.T) { BeatConfig: cfg, Manager: boolManager{managed: test.managed}, } - got, err := canInstallNpcap(b) + got, err := canInstallNpcap(b, b.BeatConfig) if err != nil { t.Errorf("unexpected error from canInstallNpcap: %v", err) } diff --git a/packetbeat/beater/packetbeat.go b/packetbeat/beater/packetbeat.go index 29e61d7f833..725f3eebc33 100644 --- a/packetbeat/beater/packetbeat.go +++ b/packetbeat/beater/packetbeat.go @@ -93,14 +93,6 @@ func New(b *beat.Beat, rawConfig *conf.C) (beat.Beater, error) { configurator = initialConfig().FromStatic } - // Install Npcap if needed. This need to happen before any other - // work on Windows, including config checking, because that involves - // probing interfaces. - err := installNpcap(b) - if err != nil { - return nil, err - } - factory := newProcessorFactory(b.Info.Name, make(chan error, maxSniffers), b, configurator) if err := factory.CheckConfig(rawConfig); err != nil { return nil, err diff --git a/packetbeat/beater/processor.go b/packetbeat/beater/processor.go index 494b8f890b6..513dbe2871c 100644 --- a/packetbeat/beater/processor.go +++ b/packetbeat/beater/processor.go @@ -123,6 +123,24 @@ func (p *processorFactory) Create(pipeline beat.PipelineConnector, cfg *conf.C) logp.Err("Failed to generate ID from config: %v, %v", err, config) return nil, err } + if len(config.Interfaces) != 0 { + // Install Npcap if needed. This needs to happen before any other + // work on Windows, including config checking, because that involves + // probing interfaces. + // + // Users may block installation of Npcap, so we defer the install + // until we have a configuration that will tell us if it has been + // blocked. To do this we must have a valid config. + // + // When Packetbeat is managed by fleet we will only have this if + // Create has been called via the agent Reload process. We take + // the opportunity to not install the DLL if there is no configured + // interface. + err := installNpcap(p.beat, cfg) + if err != nil { + return nil, err + } + } publisher, err := publish.NewTransactionPublisher( p.beat.Info.Name, From 2c0f85cf6e84e6a70643c47bd10d30648d28144c Mon Sep 17 00:00:00 2001 From: Dan Kortschak Date: Fri, 14 Jul 2023 08:15:48 +0930 Subject: [PATCH 2/4] address pr comment --- packetbeat/beater/install_npcap_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packetbeat/beater/install_npcap_test.go b/packetbeat/beater/install_npcap_test.go index 0118a448b62..90961a26d64 100644 --- a/packetbeat/beater/install_npcap_test.go +++ b/packetbeat/beater/install_npcap_test.go @@ -210,10 +210,9 @@ func TestCanInstallNpcap(t *testing.T) { t.Fatalf("unexpected error: %v", err) } b := &beat.Beat{ - BeatConfig: cfg, - Manager: boolManager{managed: test.managed}, + Manager: boolManager{managed: test.managed}, } - got, err := canInstallNpcap(b, b.BeatConfig) + got, err := canInstallNpcap(b, cfg) if err != nil { t.Errorf("unexpected error from canInstallNpcap: %v", err) } From bda92fa87bfe532f5daff3ab88ae98b0ac549445 Mon Sep 17 00:00:00 2001 From: Dan Kortschak Date: Mon, 31 Jul 2023 16:25:22 +0930 Subject: [PATCH 3/4] add logging to decision path --- packetbeat/beater/install_npcap.go | 14 ++++++++++---- packetbeat/beater/install_npcap_test.go | 3 ++- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/packetbeat/beater/install_npcap.go b/packetbeat/beater/install_npcap.go index 3363baccf1d..017fb35e9ae 100644 --- a/packetbeat/beater/install_npcap.go +++ b/packetbeat/beater/install_npcap.go @@ -60,11 +60,11 @@ func installNpcap(b *beat.Beat, cfg *conf.C) error { return nil } - canInstall, err := canInstallNpcap(b, cfg) + log := logp.NewLogger("npcap_install") + canInstall, err := canInstallNpcap(b, cfg, log) if err != nil { return err } - log := logp.NewLogger("npcap_install") if !canInstall { log.Warn("npcap installation/upgrade disabled by user") return nil @@ -103,9 +103,10 @@ func installNpcap(b *beat.Beat, cfg *conf.C) error { // configurations from agent normalised to the internal packetbeat format by this point. // In the case that the beat is managed, any data stream that has npcap.never_install // set to true will result in a block on the installation. -func canInstallNpcap(b *beat.Beat, rawcfg *conf.C) (bool, error) { +func canInstallNpcap(b *beat.Beat, rawcfg *conf.C, log *logp.Logger) (bool, error) { type npcapInstallCfg struct { - NeverInstall bool `config:"npcap.never_install"` + Type string `config:"type"` + NeverInstall bool `config:"npcap.never_install"` } // Agent managed case. @@ -120,10 +121,12 @@ func canInstallNpcap(b *beat.Beat, rawcfg *conf.C) (bool, error) { if len(cfg.Streams) == 0 { // We have no stream to monitor, so we don't need to install // anything. We may be in the middle of a config check. + log.Info("cannot install because no configured stream") return false, nil } for _, c := range cfg.Streams { if c.NeverInstall { + log.Infof("cannot install because %s has never_install set to true", c.Type) return false, nil } } @@ -136,5 +139,8 @@ func canInstallNpcap(b *beat.Beat, rawcfg *conf.C) (bool, error) { if err != nil { return false, fmt.Errorf("failed to unpack npcap config from packetbeat configuration: %w", err) } + if cfg.NeverInstall { + log.Infof("cannot install because %s has never_install set to true", cfg.Type) + } return !cfg.NeverInstall, err } diff --git a/packetbeat/beater/install_npcap_test.go b/packetbeat/beater/install_npcap_test.go index 90961a26d64..3d8c678edaf 100644 --- a/packetbeat/beater/install_npcap_test.go +++ b/packetbeat/beater/install_npcap_test.go @@ -23,6 +23,7 @@ import ( "github.com/elastic/beats/v7/libbeat/beat" "github.com/elastic/beats/v7/libbeat/management" "github.com/elastic/elastic-agent-libs/config" + "github.com/elastic/elastic-agent-libs/logp" ) var canInstallNpcapTests = []struct { @@ -212,7 +213,7 @@ func TestCanInstallNpcap(t *testing.T) { b := &beat.Beat{ Manager: boolManager{managed: test.managed}, } - got, err := canInstallNpcap(b, cfg) + got, err := canInstallNpcap(b, cfg, logp.NewLogger("npcap_install_test")) if err != nil { t.Errorf("unexpected error from canInstallNpcap: %v", err) } From 55ad07f8f1081ad7183f4028fa9fa96524cc97b1 Mon Sep 17 00:00:00 2001 From: Dan Kortschak Date: Tue, 1 Aug 2023 07:11:10 +0930 Subject: [PATCH 4/4] alter never install behaviour to allow install if no Npcap DLL is already installed --- .../_meta/config/windows_npcap.yml.tmpl | 2 +- packetbeat/beater/install_npcap.go | 29 ++++++++++++------- packetbeat/docs/packetbeat-options.asciidoc | 2 +- 3 files changed, 21 insertions(+), 12 deletions(-) diff --git a/packetbeat/_meta/config/windows_npcap.yml.tmpl b/packetbeat/_meta/config/windows_npcap.yml.tmpl index 62605c20250..23647cc6d01 100644 --- a/packetbeat/_meta/config/windows_npcap.yml.tmpl +++ b/packetbeat/_meta/config/windows_npcap.yml.tmpl @@ -8,6 +8,6 @@ #packetbeat.npcap: # # If a specific local version of Npcap is required, installation by packetbeat # # can be blocked by setting never_install to true. No action is taken if this -# # option is set to true. +# # option is set to true unless no Npcap is already installed. # never_install: false {{- end -}} diff --git a/packetbeat/beater/install_npcap.go b/packetbeat/beater/install_npcap.go index 017fb35e9ae..c1413fdb6d9 100644 --- a/packetbeat/beater/install_npcap.go +++ b/packetbeat/beater/install_npcap.go @@ -23,6 +23,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "sync" "time" @@ -61,13 +62,21 @@ func installNpcap(b *beat.Beat, cfg *conf.C) error { } log := logp.NewLogger("npcap_install") - canInstall, err := canInstallNpcap(b, cfg, log) - if err != nil { - return err - } - if !canInstall { - log.Warn("npcap installation/upgrade disabled by user") - return nil + // Only check whether we have been requested to never_install if there + // is already an Npcap installation present. This should not be necessary, + // but the start-up logic of packetbeat is tightly coupled to the presence + // of a backing sniffer. This should really not be necessary, but the changes + // to modify this behaviour are non-trivial, so just avoid the issue. + isInstalled := strings.HasPrefix(npcap.Version(), "Npcap version") + if isInstalled { + canInstall, err := canInstallNpcap(b, cfg, log) + if err != nil { + return err + } + if !canInstall { + log.Warn("npcap installation/upgrade disabled by user") + return nil + } } ctx, cancel := context.WithTimeout(context.Background(), installTimeout) @@ -121,12 +130,12 @@ func canInstallNpcap(b *beat.Beat, rawcfg *conf.C, log *logp.Logger) (bool, erro if len(cfg.Streams) == 0 { // We have no stream to monitor, so we don't need to install // anything. We may be in the middle of a config check. - log.Info("cannot install because no configured stream") + log.Debug("cannot install because no configured stream") return false, nil } for _, c := range cfg.Streams { if c.NeverInstall { - log.Infof("cannot install because %s has never_install set to true", c.Type) + log.Debugf("cannot install because %s has never_install set to true", c.Type) return false, nil } } @@ -140,7 +149,7 @@ func canInstallNpcap(b *beat.Beat, rawcfg *conf.C, log *logp.Logger) (bool, erro return false, fmt.Errorf("failed to unpack npcap config from packetbeat configuration: %w", err) } if cfg.NeverInstall { - log.Infof("cannot install because %s has never_install set to true", cfg.Type) + log.Debugf("cannot install because %s has never_install set to true", cfg.Type) } return !cfg.NeverInstall, err } diff --git a/packetbeat/docs/packetbeat-options.asciidoc b/packetbeat/docs/packetbeat-options.asciidoc index 5266dac8d33..4a74dd5593e 100644 --- a/packetbeat/docs/packetbeat-options.asciidoc +++ b/packetbeat/docs/packetbeat-options.asciidoc @@ -61,7 +61,7 @@ On Windows {beatname} requires an Npcap DLL installation. This is provided by {b for users of the Elastic Licenced version. In some cases users may wish to use their own installed version. In order to do this the `packetbeat.npcap.never_install` option can be used. Setting this option to `true` will not attempt to install the -bundled Npcap library on start-up. +bundled Npcap library on start-up unless no Npcap is already installed. [source,yaml] ------------------------------------------------------------------------------