From b9cf6579e69ada0424414cc0464f6182c1196fff Mon Sep 17 00:00:00 2001 From: John Landa Date: Fri, 28 Apr 2023 10:57:30 -0500 Subject: [PATCH] Remove artificial ACLTokenMaxTTL limit for configuring acl token expiry (#17066) * Remove artificial ACLTokenMaxTTL limit for configuring acl token expiry * Add changelog * Remove test on default MaxTokenTTL * Change to imperitive tense for changelog entry --- .changelog/17066.txt | 3 ++ agent/consul/acl_endpoint_test.go | 15 -------- agent/consul/config.go | 18 +++++----- command/acl/token/create/token_create_test.go | 34 +++++++++++++++++-- 4 files changed, 45 insertions(+), 25 deletions(-) create mode 100644 .changelog/17066.txt diff --git a/.changelog/17066.txt b/.changelog/17066.txt new file mode 100644 index 000000000..9b8b18185 --- /dev/null +++ b/.changelog/17066.txt @@ -0,0 +1,3 @@ +```release-note:improvement +command: Allow creating ACL Token TTL with greater than 24 hours with the -expires-ttl flag. +``` diff --git a/agent/consul/acl_endpoint_test.go b/agent/consul/acl_endpoint_test.go index 9507f9c62..6f674810b 100644 --- a/agent/consul/acl_endpoint_test.go +++ b/agent/consul/acl_endpoint_test.go @@ -3261,21 +3261,6 @@ func TestACLEndpoint_AuthMethodSet(t *testing.T) { err := aclEp.AuthMethodSet(&req, &resp) testutil.RequireErrorContains(t, err, "MaxTokenTTL 1ms cannot be less than") }) - - t.Run("Create with MaxTokenTTL too big", func(t *testing.T) { - reqMethod := newAuthMethod("test") - reqMethod.MaxTokenTTL = 25 * time.Hour - - req := structs.ACLAuthMethodSetRequest{ - Datacenter: "dc1", - AuthMethod: reqMethod, - WriteRequest: structs.WriteRequest{Token: TestDefaultInitialManagementToken}, - } - resp := structs.ACLAuthMethod{} - - err := aclEp.AuthMethodSet(&req, &resp) - testutil.RequireErrorContains(t, err, "MaxTokenTTL 25h0m0s cannot be more than") - }) } func TestACLEndpoint_AuthMethodDelete(t *testing.T) { diff --git a/agent/consul/config.go b/agent/consul/config.go index 61ed8b8ea..e35fceb55 100644 --- a/agent/consul/config.go +++ b/agent/consul/config.go @@ -238,9 +238,9 @@ type Config struct { AutoConfigAuthzAllowReuse bool // TombstoneTTL is used to control how long KV tombstones are retained. - // This provides a window of time where the X-Consul-Index is monotonic. + // This provides a window of time when the X-Consul-Index is monotonic. // Outside this window, the index may not be monotonic. This is a result - // of a few trade offs: + // of a few trade-offs: // 1) The index is defined by the data view and not globally. This is a // performance optimization that prevents any write from incrementing the // index for all data views. @@ -248,10 +248,10 @@ type Config struct { // is also monotonic. This prevents deletes from reducing the disk space // used. // In theory, neither of these are intrinsic limitations, however for the - // purposes of building a practical system, they are reasonable trade offs. + // purposes of building a practical system, they are reasonable trade-offs. // // It is also possible to set this to an incredibly long time, thereby - // simulating infinite retention. This is not recommended however. + // simulating infinite retention. This is not recommended, however. // TombstoneTTL time.Duration @@ -524,11 +524,13 @@ func DefaultConfig() *Config { TombstoneTTLGranularity: 30 * time.Second, SessionTTLMin: 10 * time.Second, ACLTokenMinExpirationTTL: 1 * time.Minute, - ACLTokenMaxExpirationTTL: 24 * time.Hour, + // Duration is stored as an int64. Setting the default max + // to the max possible duration (approx 290 years). + ACLTokenMaxExpirationTTL: 1<<63 - 1, // These are tuned to provide a total throughput of 128 updates - // per second. If you update these, you should update the client- - // side SyncCoordinateRateTarget parameter accordingly. + // per second. If you update these, you should update the client-side + // SyncCoordinateRateTarget parameter accordingly. CoordinateUpdatePeriod: 5 * time.Second, CoordinateUpdateBatchSize: 128, CoordinateUpdateMaxBatches: 5, @@ -560,7 +562,7 @@ func DefaultConfig() *Config { }, }, - // Stay under the 10 second aggregation interval of + // Stay under the 10-second aggregation interval of // go-metrics. This ensures we always report the // usage metrics in each cycle. MetricsReportingInterval: 9 * time.Second, diff --git a/command/acl/token/create/token_create_test.go b/command/acl/token/create/token_create_test.go index 876cc1668..747d94151 100644 --- a/command/acl/token/create/token_create_test.go +++ b/command/acl/token/create/token_create_test.go @@ -7,12 +7,14 @@ import ( "encoding/json" "strings" "testing" + "time" + + "github.com/mitchellh/cli" + "github.com/stretchr/testify/require" "github.com/hashicorp/consul/agent" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/testrpc" - "github.com/mitchellh/cli" - "github.com/stretchr/testify/require" ) func TestTokenCreateCommand_noTabs(t *testing.T) { @@ -119,6 +121,34 @@ func TestTokenCreateCommand_Pretty(t *testing.T) { require.Equal(t, "3d852bb8-5153-4388-a3ca-8ca78661889f", token.AccessorID) require.Equal(t, "3a69a8d8-c4d4-485d-9b19-b5b61648ea0c", token.SecretID) }) + + // create with an expires-ttl (<24h) + t.Run("expires-ttl_short", func(t *testing.T) { + token := run(t, []string{ + "-http-addr=" + a.HTTPAddr(), + "-token=root", + "-policy-name=" + policy.Name, + "-description=test token", + "-expires-ttl=1h", + }) + + // check diff between creation and expires time since we + // always set the token.ExpirationTTL value to 0 at the moment + require.Equal(t, time.Hour, token.ExpirationTime.Sub(token.CreateTime)) + }) + + // create with an expires-ttl long (>24h) + t.Run("expires-ttl_long", func(t *testing.T) { + token := run(t, []string{ + "-http-addr=" + a.HTTPAddr(), + "-token=root", + "-policy-name=" + policy.Name, + "-description=test token", + "-expires-ttl=8760h", + }) + + require.Equal(t, 8760*time.Hour, token.ExpirationTime.Sub(token.CreateTime)) + }) } func TestTokenCreateCommand_JSON(t *testing.T) {