From 3414e09cbb6e06980c68c05fb4abfea0fdc62a5f Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Mon, 22 Jan 2024 10:49:17 -0800 Subject: [PATCH] Remove announced deprecated code allowing ports in target addrs (#4265) We made it an error in the API to do this a while back; this cleans up the logic to disallow it for existing targets too. --- CHANGELOG.md | 5 ++++ .../handlers/targets/target_service.go | 14 ++++------- .../targets/tcp/target_service_test.go | 24 ++++++++++++------- internal/target/repository.go | 6 ++--- 4 files changed, 28 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2995900934..32c0049dcf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,11 @@ Canonical reference for changes, improvements, and bugfixes for Boundary. removed. Since 0.13.0, unless the `use_deprecated_kms_auth_method` value was set on the worker config, the new `kms` mechanism was already being used; this is simply no longer an available option. +* Per the notes in Boundary 0.12.0 and 0.14.0, it is now an error if an address + on a host or target contains a port. As of this release, this restriction also + affects existing addresses (not just creation/updating via the API) so any + existing addresses containing a port will not be able to be used as part of a + target's session authorization call. ### New and Improved diff --git a/internal/daemon/controller/handlers/targets/target_service.go b/internal/daemon/controller/handlers/targets/target_service.go index 2143d4ff77..0be8a53849 100644 --- a/internal/daemon/controller/handlers/targets/target_service.go +++ b/internal/daemon/controller/handlers/targets/target_service.go @@ -935,22 +935,16 @@ func (s Service) AuthorizeSession(ctx context.Context, req *pbs.AuthorizeSession } // Ensure we don't have a port from the address, which would be unexpected - // FIXME: We've decided to hold off on making this an error until 0.14. In - // the meantime, ignore any port coming from the host address. - hostWithoutPort, _, err := net.SplitHostPort(h) + _, _, err = net.SplitHostPort(h) switch { case err != nil && strings.Contains(err.Error(), globals.MissingPortErrStr): // This is what we expect case err != nil: return nil, errors.Wrap(ctx, err, op, errors.WithMsg("error when parsing the chosen endpoint host address")) case err == nil: - h = hostWithoutPort - // Use below logic for 0.14: - /* - return nil, handlers.ApiErrorWithCodeAndMessage( - codes.FailedPrecondition, - "Address specified for use unexpectedly contains a port.") - */ + return nil, handlers.ApiErrorWithCodeAndMessage( + codes.FailedPrecondition, + "Address specified for use unexpectedly contains a port.") } // Generate the endpoint URL diff --git a/internal/daemon/controller/handlers/targets/tcp/target_service_test.go b/internal/daemon/controller/handlers/targets/tcp/target_service_test.go index ee2c1038da..135ae89f2c 100644 --- a/internal/daemon/controller/handlers/targets/tcp/target_service_test.go +++ b/internal/daemon/controller/handlers/targets/tcp/target_service_test.go @@ -2916,7 +2916,6 @@ func TestAuthorizeSession(t *testing.T) { shsWithPort := static.TestSets(t, conn, hcWithPort.GetPublicId(), 1)[0] _ = static.TestSetMembers(t, conn, shsWithPort.GetPublicId(), []*static.Host{hWithPort}) hWithPortBareAddress := hWithPort.GetAddress() - hWithPort.Address = fmt.Sprintf("%s:54321", hWithPort.GetAddress()) hWithPort, _, err = staticRepo.UpdateHost(ctx, hcWithPort.GetProjectId(), hWithPort, hWithPort.GetVersion(), []string{"address"}) require.NoError(t, err) @@ -3822,7 +3821,6 @@ func TestAuthorizeSession_Errors(t *testing.T) { HostSourceIds: []string{hs.GetPublicId()}, }) require.NoError(t, err) - h.Address = fmt.Sprintf("%s:54321", h.GetAddress()) repo, err := staticHostRepoFn() require.NoError(t, err) _, _, err = repo.UpdateHost(ctx, hc.GetProjectId(), h, h.GetVersion(), []string{"address"}) @@ -3931,7 +3929,6 @@ func TestAuthorizeSession_Errors(t *testing.T) { name: "no worker", setup: []func(tcpTarget target.Target) uint32{hostExists, libraryExists}, useTargetId: true, - wantErr: true, wantErrContains: "No workers are available to handle this session", }, { @@ -3943,7 +3940,6 @@ func TestAuthorizeSession_Errors(t *testing.T) { name: "no target", setup: []func(tcpTarget target.Target) uint32{workerExists, hostExists, libraryExists}, useTargetId: false, - wantErr: true, wantErrContains: "Resource not found", }, { @@ -3951,25 +3947,37 @@ func TestAuthorizeSession_Errors(t *testing.T) { setup: []func(tcpTarget target.Target) uint32{workerExists, hostWithoutPort, libraryExists}, useTargetId: true, }, + { + name: "host port", + setup: []func(tcpTarget target.Target) uint32{ + workerExists, func(tcpTarget target.Target) uint32 { + tcpTarget.SetAddress("127.0.0.1:22") + repo, err := repoFn() + require.NoError(t, err) + tcpTarget, _, err = repo.UpdateTarget(ctx, tcpTarget, tcpTarget.GetVersion(), []string{"address"}) + require.NoError(t, err) + return tcpTarget.GetVersion() + }, + }, + wantErrContains: "Address specified for use unexpectedly contains a port", + useTargetId: true, + }, { name: "no hosts", setup: []func(tcpTarget target.Target) uint32{workerExists, hostSetNoHostExists, libraryExists}, useTargetId: true, - wantErr: true, wantErrContains: "No host sources or address found for given target", }, { name: "bad library configuration", setup: []func(tcpTarget target.Target) uint32{workerExists, hostExists, misConfiguredlibraryExists}, useTargetId: true, - wantErr: true, wantErrContains: "external system issue: error #3014: Error making API request", }, { name: "expired token library", setup: []func(tcpTarget target.Target) uint32{workerExists, hostExists, expiredTokenLibrary}, useTargetId: true, - wantErr: true, wantErrContains: "vault.newClient: invalid configuration", }, } @@ -3990,7 +3998,7 @@ func TestAuthorizeSession_Errors(t *testing.T) { res, err := s.AuthorizeSession(ctx, &pbs.AuthorizeSessionRequest{ Id: id, }) - if tc.wantErr { + if tc.wantErrContains != "" { require.Error(t, err) require.Nil(t, res) require.ErrorContains(t, err, tc.wantErrContains) diff --git a/internal/target/repository.go b/internal/target/repository.go index e0015a278c..cc589b6535 100644 --- a/internal/target/repository.go +++ b/internal/target/repository.go @@ -659,9 +659,9 @@ func (r *Repository) UpdateTarget(ctx context.Context, target Target, version ui return nil, db.NoRowsAffected, errors.New(ctx, errors.EmptyFieldMask, op, "empty field mask") } - // The Address field is not apart of the target schema in the database. - // It is apart of a different table called target_address, which is why - // the Address field must be filtered out of the dbMask & nullFields slices. + // The Address field is not a part of the target schema in the database. It + // is a part of a different table called target_address, which is why the + // Address field must be filtered out of the dbMask & nullFields slices. var updateAddress, deleteAddress bool var filteredDbMask, filteredNullFields []string for _, f := range dbMask {