Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read and apply mTLS config from policy #4398

Closed
wants to merge 33 commits into from
Closed
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
e56c7b8
wip - so far so good. Still missing several tests
AndersonQ Mar 7, 2024
c90d31d
add elastic-agent-libs override
AndersonQ Mar 7, 2024
ac843e1
.
AndersonQ Mar 7, 2024
b4e7023
added CA tests
AndersonQ Mar 7, 2024
10078f3
go mod tidy
AndersonQ Mar 7, 2024
201fdcd
certificate/key code and test
AndersonQ Mar 11, 2024
df1419a
unify SSL tests and add a proxy subtest
AndersonQ Mar 11, 2024
b886e79
better log when test fails
AndersonQ Mar 11, 2024
2077e3b
fixes and adjustments
AndersonQ Mar 11, 2024
e41f4d6
go mod tidy
AndersonQ Mar 11, 2024
878a230
revert magefile change
AndersonQ Mar 11, 2024
424cbdf
fix override
AndersonQ Mar 11, 2024
7411b93
fix overrides
AndersonQ Mar 11, 2024
6736f89
fix panic
AndersonQ Mar 12, 2024
713b7b2
fix CA ext keyusage
AndersonQ Mar 14, 2024
74c21c2
add test for encrypted certificate key
AndersonQ Mar 19, 2024
9db4742
fix go.sum after rebase
AndersonQ Mar 19, 2024
b2b3a81
mage notice
AndersonQ Mar 20, 2024
b66b67e
Merge branch 'main' into 2247-mtls
AndersonQ Mar 21, 2024
cf264db
Merge branch 'main' of github.com:elastic/elastic-agent into 2247-mtls
AndersonQ Mar 21, 2024
c7bdf50
Merge branch 'main' into 2247-mtls
AndersonQ Mar 21, 2024
2296c98
use a released version of elastic-agent-libs
AndersonQ Mar 21, 2024
4964255
Merge branch '2247-mtls' of github.com:AndersonQ/elastic-agent into 2…
AndersonQ Mar 21, 2024
89fd117
PR changes
AndersonQ Mar 21, 2024
6c9b6a8
Merge branch 'main' into 2247-mtls
AndersonQ Mar 21, 2024
e3fcadc
Merge branch 'main' into 2247-mtls
AndersonQ Mar 22, 2024
e4a6a81
fix panic in debug log
AndersonQ Mar 22, 2024
cbc9209
Merge branch '2247-mtls' of github.com:AndersonQ/elastic-agent into 2…
AndersonQ Mar 22, 2024
e279691
Merge branch 'main' of github.com:elastic/elastic-agent into 2247-mtls
AndersonQ Mar 26, 2024
c1c9289
add cchangelog
AndersonQ Mar 26, 2024
d878942
Merge branch 'main' into 2247-mtls
AndersonQ Apr 2, 2024
5d688b3
making linter happy
AndersonQ Apr 3, 2024
cd3222b
Merge branch '2247-mtls' of github.com:AndersonQ/elastic-agent into 2…
AndersonQ Apr 3, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dev-tools/notice/overrides.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{"name": "github.com/elastic/beats/v7", "licenceType": "Elastic"}
{"name": "github.com/elastic/elastic-agent-client/v7", "licenceType": "Elastic"}
{"name": "github.com/AndersonQ/elastic-agent-libs", "licenceType": "Apache-2.0"}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting elastic/elastic-agent-libs#189 to be merged

{"name": "github.com/elastic/e2e-testing", "licenceType": "Elastic"}
{"name": "github.com/gorhill/cronexpr", "licenceType": "Apache-2.0", "licenceFile":"APLv2"}
{"name": "github.com/hashicorp/cronexpr", "licenceType": "Apache-2.0", "licenceFile":"APLv2"}
Expand Down
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -282,3 +282,5 @@

// Exclude this version because the version has an invalid checksum.
exclude github.com/docker/distribution v2.8.0+incompatible

replace github.com/elastic/elastic-agent-libs => github.com/AndersonQ/elastic-agent-libs v0.0.0-20240307144511-b45f19396c67

Check failure on line 286 in go.mod

View workflow job for this annotation

GitHub Actions / lint (ubuntu-latest)

replacement are not allowed: github.com/elastic/elastic-agent-libs (gomoddirectives)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting elastic/elastic-agent-libs#189 to be merged

4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,8 @@ cloud.google.com/go/workflows v1.9.0/go.mod h1:ZGkj1aFIOd9c8Gerkjjq7OW7I5+l6cSvT
dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU=
github.com/AdaLogics/go-fuzz-headers v0.0.0-20210715213245-6c3934b029d8/go.mod h1:CzsSbkDixRphAF5hS6wbMKq0eI6ccJRb7/A0M6JBnwg=
github.com/AlecAivazis/survey/v2 v2.3.6/go.mod h1:4AuI9b7RjAR+G7v9+C4YSlX/YL3K3cWNXgWXOhllqvI=
github.com/AndersonQ/elastic-agent-libs v0.0.0-20240307144511-b45f19396c67 h1:wHK1/9r01Qd0sQoKDVs72zFfHQSBaejCBhnlTxrAyeQ=
github.com/AndersonQ/elastic-agent-libs v0.0.0-20240307144511-b45f19396c67/go.mod h1:pGMj5myawdqu+xE+WKvM5FQzKQ/MonikkWOzoFTJxaU=
github.com/Azure/azure-sdk-for-go v16.2.1+incompatible/go.mod h1:9XXNKU+eRnpl9moKnB4QOLf1HestfXbmab5FXxiDBjc=
github.com/Azure/azure-sdk-for-go v56.3.0+incompatible/go.mod h1:9XXNKU+eRnpl9moKnB4QOLf1HestfXbmab5FXxiDBjc=
github.com/Azure/go-ansiterm v0.0.0-20170929234023-d6e3b3328b78/go.mod h1:LmzpDX56iTiv29bbRTIsUNlaFfuhWRQBWjQdVyAevI8=
Expand Down Expand Up @@ -798,8 +800,6 @@ github.com/elastic/elastic-agent-autodiscover v0.6.8 h1:BSXz+QwjZAEt08G+T3GDGl14
github.com/elastic/elastic-agent-autodiscover v0.6.8/go.mod h1:hFeFqneS2r4jD0/QzGkrNk0YVdN0JGh7lCWdsH7zcI4=
github.com/elastic/elastic-agent-client/v7 v7.8.1 h1:J9wZc/0mUvSEok0X5iR5+n60Jgb+AWooKddb3XgPWqM=
github.com/elastic/elastic-agent-client/v7 v7.8.1/go.mod h1:axl1nkdqc84YRFkeJGD9jExKNPUrOrzf3DFo2m653nY=
github.com/elastic/elastic-agent-libs v0.7.5 h1:4UMqB3BREvhwecYTs/L23oQp1hs/XUkcunPlmTZn5yg=
github.com/elastic/elastic-agent-libs v0.7.5/go.mod h1:pGMj5myawdqu+xE+WKvM5FQzKQ/MonikkWOzoFTJxaU=
github.com/elastic/elastic-agent-system-metrics v0.9.2 h1:/tvTKOt55EerU0WwGFoDhBlyWLgxyv7d8xCbny0bciw=
github.com/elastic/elastic-agent-system-metrics v0.9.2/go.mod h1:VfJnKw4Jqrd9ddljXCwaGKJgN+7ADyyGk089NaXVsf0=
github.com/elastic/elastic-integration-corpus-generator-tool v0.5.0/go.mod h1:uf9N86y+UACGybdEhZLpwZ93XHWVhsYZAA4c2T2v6YM=
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@ import (
"fmt"
"io"
"net/http"
"slices"
"sort"
"time"

"gopkg.in/yaml.v2"

"github.com/elastic/elastic-agent-libs/transport/httpcommon"
"github.com/elastic/elastic-agent-libs/transport/tlscommon"
"github.com/elastic/elastic-agent/internal/pkg/agent/application/actions"
"github.com/elastic/elastic-agent/internal/pkg/agent/application/coordinator"
"github.com/elastic/elastic-agent/internal/pkg/agent/application/info"
Expand All @@ -25,7 +27,7 @@ import (
"github.com/elastic/elastic-agent/internal/pkg/config"
"github.com/elastic/elastic-agent/internal/pkg/fleetapi"
"github.com/elastic/elastic-agent/internal/pkg/fleetapi/acker"
"github.com/elastic/elastic-agent/internal/pkg/fleetapi/client"
fleetclient "github.com/elastic/elastic-agent/internal/pkg/fleetapi/client"
"github.com/elastic/elastic-agent/internal/pkg/remote"
"github.com/elastic/elastic-agent/pkg/core/logger"
)
Expand Down Expand Up @@ -104,7 +106,7 @@ func (h *PolicyChangeHandler) Handle(ctx context.Context, a fleetapi.Action, ack
}

h.log.Debugf("handlerPolicyChange: emit configuration for action %+v", a)
err = h.handleFleetServerHosts(ctx, c)
err = h.handleFleetServerConfig(ctx, c)
if err != nil {
return err
}
Expand All @@ -118,11 +120,11 @@ func (h *PolicyChangeHandler) Watch() <-chan coordinator.ConfigChange {
return h.ch
}

func (h *PolicyChangeHandler) handleFleetServerHosts(ctx context.Context, c *config.Config) (err error) {
// do not update fleet-server host from policy; no setters provided with local Fleet Server
func (h *PolicyChangeHandler) handleFleetServerConfig(ctx context.Context, c *config.Config) (err error) {
if len(h.setters) == 0 {
return nil
}

data, err := c.ToMapStr()
if err != nil {
return errors.New(err, "could not convert the configuration from the policy", errors.TypeConfig)
Expand All @@ -142,29 +144,22 @@ func (h *PolicyChangeHandler) handleFleetServerHosts(ctx context.Context, c *con
return nil
}

// only set protocol/hosts as that is all Fleet currently sends
prevProtocol := h.config.Fleet.Client.Protocol
prevPath := h.config.Fleet.Client.Path
prevHost := h.config.Fleet.Client.Host
prevHosts := h.config.Fleet.Client.Hosts
prevProxy := h.config.Fleet.Client.Transport.Proxy
h.config.Fleet.Client.Protocol = cfg.Fleet.Client.Protocol
h.config.Fleet.Client.Path = cfg.Fleet.Client.Path
h.config.Fleet.Client.Host = cfg.Fleet.Client.Host
h.config.Fleet.Client.Hosts = cfg.Fleet.Client.Hosts

// Empty proxies from fleet are ignored. That way a proxy set by --proxy-url
// it won't be overridden by an absent or empty proxy from fleet-server.
// However, if there is a proxy sent by fleet-server, it'll take precedence.
// Therefore, it's not possible to remove a proxy once it's set.
if cfg.Fleet.Client.Transport.Proxy.URL == nil ||
cfg.Fleet.Client.Transport.Proxy.URL.String() == "" {
h.log.Debug("proxy from fleet is empty or null, the proxy will not be changed")

var prevTLSNil bool
var prevCAs []string
var prevCertificateCfg tlscommon.CertificateConfig
if h.config.Fleet.Client.Transport.TLS != nil {
prevTLSNil = false
prevCAs = h.config.Fleet.Client.Transport.TLS.CAs
prevCertificateCfg = h.config.Fleet.Client.Transport.TLS.Certificate
} else {
h.config.Fleet.Client.Transport.Proxy = cfg.Fleet.Client.Transport.Proxy
h.log.Debug("received proxy from fleet, applying it")
prevTLSNil = true
}

// rollback on failure
defer func() {
if err != nil {
Expand All @@ -173,10 +168,19 @@ func (h *PolicyChangeHandler) handleFleetServerHosts(ctx context.Context, c *con
h.config.Fleet.Client.Host = prevHost
h.config.Fleet.Client.Hosts = prevHosts
h.config.Fleet.Client.Transport.Proxy = prevProxy
if prevTLSNil {
h.config.Fleet.Client.Transport.TLS = nil
} else {
h.config.Fleet.Client.Transport.TLS.CAs = prevCAs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just save the entire TLS configuration instead of a boolean and individual fields from it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tl;dr: pointers. The rollback wasn't working properly. I don't quite remember which specific property was the problem.

h.config.Fleet.Client.Transport.TLS.Certificate = prevCertificateCfg
}
h.log.Debugf("an error happened, reverting fleet-server config")
}
}()

client, err := client.NewAuthWithConfig(
h.applyConfigWithPrecedence(cfg.Fleet.Client)

client, err := fleetclient.NewAuthWithConfig(
h.log, h.config.Fleet.AccessAPIKey, h.config.Fleet.Client)
if err != nil {
return errors.New(
Expand Down Expand Up @@ -220,25 +224,80 @@ func (h *PolicyChangeHandler) handleFleetServerHosts(ctx context.Context, c *con
return nil
}

func clientEqual(k1 remote.Config, k2 remote.Config) bool {
if k1.Protocol != k2.Protocol {
// applyConfigWithPrecedence applies Proxy and TLS configs, but ignores empty or
// absent ones.
// That way a proxy or TLS config set during install/enroll using cli flags
// won't be overridden by an absent or empty proxy from fleet-server.
// However, if there is a proxy or TLS config sent by fleet-server, it'll take
// precedence. Therefore, it's not possible to remove a proxy or TLS config once
// it's set.
Comment on lines +232 to +233
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Therefore, it's not possible to remove a proxy or TLS config once it's set.

You mean it's not possible to remove the CLI parameters?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, we do not have enough information here to know what came from the cli flag and what came from fleet-server neither, IIRC, fleet-server differentiates between empty and absent. Therefore, once a proxy or TLS is set, it's set for good.

We do have a different handler for policy reassign, perhaps we could use it to be able to remove proxy and TLS configs.

However I'd argue what we actually need is for to be able to differentiate "remove proxy/TLS" from no-change

func (h *PolicyChangeHandler) applyConfigWithPrecedence(cfg remote.Config) {
defaultcfg := configuration.DefaultFleetAgentConfig()

if cfg.Protocol != defaultcfg.Client.Protocol ||
cfg.Host != defaultcfg.Client.Host ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if you're changing from localhost:5602 to localhost:5601

!slices.Equal(cfg.Hosts, defaultcfg.Client.Hosts) ||
cfg.Path != defaultcfg.Client.Path {
h.config.Fleet.Client.Protocol = cfg.Protocol
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we overwrite very parameter if only one doesn't match the default?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's set as one thing on fleetUI, thus I thought it'd the more accurate way of handling it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a built in assumption that Fleet will never change this unintentionally.

Do not assume Fleet will always perfectly conform to your expectations. Bugs can exist on both sides of the connection.

If this happened and caused settings not apply, how would we tell and debug it?

h.config.Fleet.Client.Path = cfg.Path
h.config.Fleet.Client.Host = cfg.Host
h.config.Fleet.Client.Hosts = cfg.Hosts
}

if cfg.Transport.Proxy.URL == nil ||
cfg.Transport.Proxy.URL.String() == "" {
h.log.Debug("proxy from fleet is empty or null, the proxy will not be changed")
} else {
h.config.Fleet.Client.Transport.Proxy = cfg.Transport.Proxy
h.log.Debug("received proxy from fleet, applying it")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you log what the new proxy URL is?

}

emptyCertificate := tlscommon.CertificateConfig{}
if cfg.Transport.TLS != nil {
if h.config.Fleet.Client.Transport.TLS == nil {
h.config.Fleet.Client.Transport.TLS = &tlscommon.Config{}
}

// client certificate
if cfg.Transport.TLS.Certificate == emptyCertificate {
h.log.Debug("TLS certificate/key from fleet are empty or null, the TLS config will not be changed")
} else {
h.config.Fleet.Client.Transport.TLS.Certificate = cfg.Transport.TLS.Certificate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the Certificate configuration from Fleet always complete? Does it or could it ever allow updating only individual fields?

If it does this unintentionally clears the ones that weren't set, for example if someone sets only the Certificate but not the Key and Key were set previously.

I suppose if this happened unintentionally and broke things it would be reverted, but it would be hard for a user to debug.

We either need to make debugging that failure easier or just prevent it from happening entirely.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the Certificate configuration from Fleet always complete?
Well, I don't see how one would change a certificate and not the key and the key's passphrase.

Besides again we're in the problem of "remove it" or "no change". Changing it all atomically seems to me the best and less error prone approach.

h.log.Debug("received TSL certificate/key from fleet, applying it")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
h.log.Debug("received TSL certificate/key from fleet, applying it")
h.log.Debug("received TLS certificate/key from fleet, applying it")

}

// CA
if len(cfg.Transport.TLS.CAs) == 0 {
h.log.Debug("TLS CAs from fleet are empty or null, the TLS config will not be changed")
} else {
h.config.Fleet.Client.Transport.TLS.CAs = cfg.Transport.TLS.CAs
h.log.Debug("received SSL from fleet, applying it")
Copy link
Member

@cmacknz cmacknz Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
h.log.Debug("received SSL from fleet, applying it")
h.log.Debug("received TLS CAs from fleet, applying them")

}
}
}

func clientEqual(current remote.Config, new remote.Config) bool {
if new.Protocol != "" &&
current.Protocol != new.Protocol {
return false
}
if k1.Path != k2.Path {
if new.Path != "" &&
current.Path != new.Path {
return false
}

sort.Strings(k1.Hosts)
sort.Strings(k2.Hosts)
if len(k1.Hosts) != len(k2.Hosts) {
sort.Strings(current.Hosts)
sort.Strings(new.Hosts)
if len(current.Hosts) != len(new.Hosts) {
return false
}
for i, v := range k1.Hosts {
if v != k2.Hosts[i] {
for i, v := range current.Hosts {
if v != new.Hosts[i] {
return false
}
}

// should it ignore empty headers?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty headers from Fleet? This should have the same precedence rules as everything else.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, empty proxy headers. Thinking better bout it, if there is a new or non-nil/non-empty proxy config, if the new has no headers, we need to override them. So I think it's correct as it's right now

headersEqual := func(h1, h2 httpcommon.ProxyHeaders) bool {
if len(h1) != len(h2) {
return false
Expand All @@ -255,9 +314,24 @@ func clientEqual(k1 remote.Config, k2 remote.Config) bool {
}

// different proxy
if k1.Transport.Proxy.URL != k2.Transport.Proxy.URL ||
k1.Transport.Proxy.Disable != k2.Transport.Proxy.Disable ||
!headersEqual(k1.Transport.Proxy.Headers, k2.Transport.Proxy.Headers) {
if new.Transport.Proxy.URL != nil &&
current.Transport.Proxy.URL != new.Transport.Proxy.URL ||
current.Transport.Proxy.Disable != new.Transport.Proxy.Disable ||
!headersEqual(current.Transport.Proxy.Headers, new.Transport.Proxy.Headers) {
return false
}

// different TLS config
if new.Transport.TLS != nil &&
len(new.Transport.TLS.CAs) > 0 &&
!slices.Equal(current.Transport.TLS.CAs, new.Transport.TLS.CAs) {
return false
}

emptyCert := tlscommon.CertificateConfig{}
if new.Transport.TLS != nil &&
new.Transport.TLS.Certificate != emptyCert &&
current.Transport.TLS.Certificate != new.Transport.TLS.Certificate {
return false
}

Expand Down
Loading
Loading