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

Remove forcing localhost for insecure bootstrap #2198

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# Kind can be one of:
# - breaking-change: a change to previously-documented behavior
# - deprecation: functionality that is being removed in a later release
# - bug-fix: fixes a problem in a previous version
# - enhancement: extends functionality but does not break or fix existing behavior
# - feature: new functionality
# - known-issue: problems that we are aware of in a given version
# - security: impacts on the security of a product or a user’s deployment.
# - upgrade: important information for someone upgrading from a prior version
# - other: does not fit into any of the other categories
kind: bug-fix

# Change summary; a 80ish characters long description of the change.
summary: fix insecure fleet-server bootstrap

# Long description; in case the summary is not enough to describe the change
# this field accommodate a description without length limits.
description: Disable passing `localhost` when bootstrapping an insecure fleet-server instance.

# Affected component; a word indicating the component this changeset affects.
component: fleet-server

# PR number; optional; the PR number that added the changeset.
# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added.
# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number.
# Please provide it if you are adding a fragment for a different PR.
#pr: 1234

# Issue number; optional; the GitHub issue related to this changeset (either closes or is part of).
# If not present is automatically filled by the tooling with the issue linked to the PR number.
issue: 2197
4 changes: 0 additions & 4 deletions internal/pkg/agent/cmd/enroll_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -395,10 +395,6 @@ func (c *enrollCmd) prepareFleetTLS() error {
}
if c.options.FleetServer.Cert == "" && c.options.FleetServer.CertKey == "" {
if c.options.FleetServer.Insecure {
// running insecure, force the binding to localhost (unless specified)
Copy link
Member

Choose a reason for hiding this comment

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

What was the original reason this was done? Just covering the case where the host was unset? Is there a test we can add to keep this bug from happening again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why this was done here. I'll try to add a couple unit tests for this method.

I think we need to do some minor cleanup around bootstapping (in another pr) as we have both this method and createFleetServerBootstrapConfig altering config at different locations

Copy link
Contributor

@michalpristas michalpristas Jan 31, 2023

Choose a reason for hiding this comment

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

cc @blakerouse do you remember why we added it here?
introduced here: elastic/beats#24142

Copy link
Contributor

Choose a reason for hiding this comment

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

This was added for security reasons, we didn't want the Fleet Server to be exposed outside of localhost insecure mode.

We only allow it to be exposed in insecure mode when the user is specific and provide a host. I think we should really think about this before we make this change.

Copy link
Member

Choose a reason for hiding this comment

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

Reading the issue description it sounds like when it was explicitly configured to bind to 0.0.0.0 it binds to localhost anyway.

Does c.options.FleetServer.Host evaluate to "" when the host was set to 0.0.0.0 intentionally?

Copy link
Contributor

Choose a reason for hiding this comment

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

@cmacknz to clarify, I ran into this issue by setting the host in the Fleet Server integration, through Kibana:

# kibana.yml
# ...
        inputs:
          - type: fleet-server
            vars:
              - name: host
                value: 0.0.0.0

If I configure the host directly in the Elastic Agent, it works.

I think Kibana's value is ignored because Elastic Agent has no way to tell whether the user explicitly requested 0.0.0.0 or simply relied on the default, which happens to be 0.0.0.0 as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Kibana value is always ignored for Fleet Server, and it has always been that way. Only the values used during the bootstrap process is what is used for Fleet Server.

Copy link
Contributor

Choose a reason for hiding this comment

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

@blakerouse in that case I'm fine with having to explicitly set the host in the Elastic Agent, since the root cause of the issue is my assumption that Kibana's values were actually taken into account.

It's still confusing to upgrade from 8.5 and 8.6 and realize that things aren't working anymore, but this could have easily been addressed with a changelog entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we consider removing the ability to alter the host value in Kibana to make it clear that it's ignored?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO yes, since this isn't obvious at all.

if c.options.FleetServer.Host == "" {
c.options.FleetServer.Host = defaultFleetServerInternalHost
}
c.options.URL = fmt.Sprintf("http://%s:%d", host, port)
c.options.Insecure = true
return nil
Expand Down
69 changes: 64 additions & 5 deletions internal/pkg/agent/cmd/enroll_cmd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"bytes"
"context"
"crypto/tls"
"errors"
"fmt"
"io"
"io/ioutil"
"net"
Expand All @@ -18,7 +20,7 @@ import (
"strconv"
"testing"

"github.com/pkg/errors"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/elastic/elastic-agent/internal/pkg/agent/configuration"
Expand All @@ -42,7 +44,7 @@ func (m *mockStore) Save(in io.Reader) error {
}

buf := new(bytes.Buffer)
io.Copy(buf, in) // nolint:errcheck //not required
io.Copy(buf, in) //nolint:errcheck // not required
m.Content = buf.Bytes()
return nil
}
Expand Down Expand Up @@ -324,6 +326,7 @@ func TestEnroll(t *testing.T) {
require.False(t, store.Called)
},
))

}

func TestValidateArgs(t *testing.T) {
Expand Down Expand Up @@ -372,6 +375,62 @@ func TestValidateArgs(t *testing.T) {
require.NotContains(t, cleanedTags, "")
}

func TestPrepareFleetTLS(t *testing.T) {
log, _ := logger.New("tst", false)
t.Run("fleet-server insecure", func(t *testing.T) {
store := &mockStore{}
cmd, err := newEnrollCmdWithStore(
log,
&enrollCmdOption{
FleetServer: enrollCmdFleetServerOption{
ConnStr: "https://localhost:9200",
ServiceToken: "test-service-token",
PolicyID: "test-fleet-policy",
Insecure: true,
},
},
"",
store,
)
require.NoError(t, err)
err = cmd.prepareFleetTLS()
require.NoError(t, err)

assert.True(t, cmd.options.Insecure)
assert.Equal(t, "http://localhost:8220", cmd.options.URL)

assert.Equal(t, "", cmd.options.FleetServer.Host)
})
t.Run("fleet-server secure", func(t *testing.T) {
store := &mockStore{}
cmd, err := newEnrollCmdWithStore(
log,
&enrollCmdOption{
FleetServer: enrollCmdFleetServerOption{
ConnStr: "https://localhost:9200",
ServiceToken: "test-service-token",
PolicyID: "test-fleet-policy",
},
},
"",
store,
)
require.NoError(t, err)
err = cmd.prepareFleetTLS()
require.NoError(t, err)

assert.False(t, cmd.options.Insecure)
assert.NotEmpty(t, cmd.options.FleetServer.Cert)
assert.NotEmpty(t, cmd.options.FleetServer.CertKey)
assert.NotEmpty(t, cmd.options.CAs)

hostname, _ := os.Hostname()
assert.Equal(t, fmt.Sprintf("https://%s:8220", hostname), cmd.options.URL)

assert.Equal(t, "", cmd.options.FleetServer.Host)
})
}

func withServer(
m func(t *testing.T) *http.ServeMux,
test func(t *testing.T, host string),
Expand Down Expand Up @@ -402,7 +461,7 @@ func withTLSServer(

port := listener.Addr().(*net.TCPAddr).Port

s := http.Server{
s := http.Server{ //nolint:gosec // test server config
Handler: m(t),
TLSConfig: &tls.Config{
Certificates: []tls.Certificate{serverCert},
Expand All @@ -411,7 +470,7 @@ func withTLSServer(
}

// Uses the X509KeyPair pair defined in the TLSConfig struct instead of file on disk.
go s.ServeTLS(listener, "", "") // nolint:errcheck //not required
go s.ServeTLS(listener, "", "") //nolint:errcheck // not required

test(t, ca.Crt(), "localhost:"+strconv.Itoa(port))
}
Expand All @@ -422,7 +481,7 @@ func bytesToTMPFile(b []byte) (string, error) {
if err != nil {
return "", err
}
f.Write(b) // nolint:errcheck //not required
f.Write(b) //nolint:errcheck // not required
if err := f.Close(); err != nil {
return "", err
}
Expand Down