feat: add optional token hashing #25982

gwossum · 2025-02-06T21:30:48Z

Add optional token hashing with --use-hashed-tokens command line option.

davidby-influx

A variety of smaller issues. I will need to review this again to fully understand the larger changes.

authorization/hasher.go

authorization/http_server_test.go

testing/onboarding.go

davidby-influx · 2025-02-10T21:24:08Z

authorizer/task_test.go

+				Bucket:                 "holder",
+				RetentionPeriodSeconds: 1,
+			})
+			if err != nil {


require.NoError?

davidby-influx · 2025-02-10T21:24:51Z

authorizer/task_test.go

-		t.Fatal(err)
+			})
+			if err != nil {
+				t.Fatal(err)


require.NoError?

cmd/influxd/recovery/auth/auth.go

davidby-influx

My usual add more context to error and log messages and a few more substantive questions and suggestions

authorization/hasher.go

davidby-influx · 2025-02-20T18:40:35Z

authorization/http_server_test.go

@@ -185,21 +182,16 @@ func TestService_handlePostAuthorization(t *testing.T) {
 				handler.handlePostAuthorization(w, r)

 				res := w.Result()
-				content := res.Header.Get("Content-Type")
+				contentType := res.Header.Get("Content-Type")
 				body, _ := io.ReadAll(res.Body)


maybe an error check on ReadAll? probably never fails, though....

Handled now.

davidby-influx · 2025-02-20T18:43:10Z

authorization/http_server_test.go

 				}
+				diff, err := jsonDiff(string(body), tt.wants.body)
+				require.NoError(t, err)
+				require.Empty(t, diff)


A little context message in these two would help debugging: diff failed on <whatever this is doing> and
expected no difference in authorization response or something like that.

Added an error message.

davidby-influx · 2025-02-20T18:43:29Z

authorization/http_server_test.go

 			}
+			diff, err := jsonDiff(string(body), tt.wants.body)
+			require.NoError(t, err)
+			require.Empty(t, diff)


Added an error message.

davidby-influx · 2025-02-20T18:44:30Z

authorization/http_server_test.go

-				t.Errorf("%q. handleDeleteAuthorization() = %v, want %v", tt.name, content, tt.wants.contentType)
+			require.Equal(t, tt.wants.statusCode, res.StatusCode)
+			if tt.wants.contentType != "" {
+				require.Equal(t, tt.wants.contentType, contentType)


mention handleDeleteAuthorization in the log message.

davidby-influx · 2025-02-20T23:19:11Z

authorization/storage_authorization.go

+}
+
+// deleteIndices removes indices for the given token and hashedToken.
+func (s *Store) deleteIndices(ctx context.Context, tx kv.Tx, token, hashedToken string) error {


I'm wondering if this should be more robust. Even if we can't get one bucket, remove the (hashed) token from the other, and if we can't remove one token, still try to remove the other. Would that leave us in a better state for recovery, or not?

If error will get returned up the call tree and cause the BoltDB transaction to be rolled back, so we won't leave BoltDB in an inconsistent state.

authorization/storage_authorization.go

cmd/influxd/recovery/auth/auth.go

testing/onboarding.go

davidby-influx

All changes pertain to comments, but important one is whether to delete or include a commented out code section

authorization/storage_authorization_test.go

cmd/influxd/recovery/auth/auth.go

Add optional token hashing with `--use-hashed-tokens` command line option.

Create duplicate tokens from being created. This is a bug introduced earlier in this PR. Also improve tests so they detect the bug and use testify throughout.

Fix a bug that only allowed hashed tokens to be looked up if they used the currently active hashing algorithm. Also added tests for configuration migration scenarios (enabling and disabling hashing, changing hashing scheme).

Address PR issues on comments, error handling, and add final token matching check in `Store.GetAuthorizationByToken`.

Changes in addition to minor cleanups: - `authentication.Store` can now log info and warnings - Improved logic in `UpdateAuthorization` when both Token and HashedToken are set. Added supporting test cases.

devanbenz

Just a few comments mostly about extra safety checks and some error messaging. I've been able to get about half way through the PR. Going to step away for a minute to take a little brain break and then finish up the rest of the PR for my first pass.

devanbenz · 2025-02-26T20:41:10Z

authorization/hasher.go

+	// Create the hasher used for hashing new tokens before storage.
+	hasher, err := influxdb2_algo.New(influxdb2_algo.WithVariant(options.hasherVariant))
+	if err != nil {
+		return nil, fmt.Errorf("creating hasher for AuthorizationHasher: %w", err)


display hasherVariant for error?

fmt.Errorf("creating hasher with variant %s for AuthorizationHasher: %w", options.hasherVariant, err)

devanbenz · 2025-02-26T20:46:32Z

authorization/hasher.go

+// AllHashes generates a list of PHC-encoded hashes of token for all deterministic (i.e. non-salted) supported hashes.
+func (h *AuthorizationHasher) AllHashes(token string) ([]string, error) {
+	hashes := make([]string, len(h.allHashers))
+	for idx, hasher := range h.allHashers {


Could there ever be a code path where h.allHashers could be empty and we expect for it not to be? Would it be a good idea to check h.allHashers len and warn on it if its == 0?

devanbenz · 2025-02-26T20:48:16Z

authorization/hasher.go

+	for idx, hasher := range h.allHashers {
+		digest, err := hasher.Hash(token)
+		if err != nil {
+			variantName := "N/A"


Hardcoding "N/A" here seems weird to me. Could this be an enum or a constant of some sort?

devanbenz · 2025-02-26T21:06:30Z

authorization/http_server_test.go

-				t.Fatal(err)
-			}
+				storage, err := NewStore(context.Background(), s, useHashedTokens)
+				require.NoError(t, err)


Add some context to this error so we know what failed

require.NoError(t, err, "creating new store")

devanbenz · 2025-02-26T21:06:49Z

authorization/http_server_test.go

-				t.Fatalf("failed to unmarshal authorization: %v", err)
-			}
+				req, err := newPostAuthorizationRequest(tt.args.authorization)
+				require.NoError(t, err)


Similar to above - add small context string.

devanbenz · 2025-02-26T21:42:03Z

authorization/storage.go

+			return nil
+		})
+		if err != nil {
+			return fmt.Errorf("error migrating hashed tokens: %w", err)


Adding some more context to the error such as ID and maybe token description? Similar to the s.log.Warn above?

devanbenz · 2025-02-26T21:45:19Z

authorization/storage_authorization.go

+	// Redact Token, if needed. This is done at the lowest level so it is impossible to serialize
+	// raw tokens if hashing is enabled.
+	if s.useHashedTokens {
+		// Redact a copy, not the original. The raw Token value is still needed by the caller in some cases.


Nice thank you for the comment, makes the code below much easier to reason about.

devanbenz · 2025-02-26T21:46:37Z

authorization/storage_authorization.go

+// error is returned.
+func (s *Store) transformToken(a *influxdb.Authorization) error {
+	// Verify Token and HashedToken match if both are set.
+	if a.Token != "" && a.HashedToken != "" {


since 'a' is a pointer should there be a nil check for safety before field access?

devanbenz · 2025-02-26T21:47:15Z

authorization/storage_authorization.go

+			// Note that even if a.HashedToken is set, we will regenerate it here. This ensures
+			// that a.HashedToken will be stored using the currently configured hashing algorithm.
+			if hashedToken, err := s.hasher.Hash(a.Token); err != nil {
+				return fmt.Errorf("error hashing token: %w", err)


Any other information such as description or ID?

devanbenz · 2025-02-26T21:48:10Z

authorization/storage_authorization.go

+	if auth.HashedToken != "" {
+		match, err := s.hasher.Match(auth.HashedToken, token)
+		if err != nil {
+			return false, fmt.Errorf("error matching hashed token for validation: %w", err)


More information like ID and description?

devanbenz

Adding a few more comments to storage_authorization.go I noticed that a bunch of my comments for the require.NoError(..) is similar to what david commented so my apologies for that :P

devanbenz · 2025-02-27T16:05:34Z

authorization/storage_authorization.go

@@ -71,42 +142,12 @@ func (s *Store) CreateAuthorization(ctx context.Context, tx kv.Tx, a *influxdb.A
 		a.ID = id
 	}

+	// Token must be unique to create authorization.


If s.uniqueAuthToken fails should the mutation of a.HashedToken = hashedToken be reverted/rolled back at all? https://github.com/influxdata/EAR/issues/5819#issuecomment-2686492432

devanbenz · 2025-02-27T16:24:31Z

authorization/storage_authorization.go

+				return string(got) == token
+			}
+			if len(allHashes) > 0 {
+				if got, _, _, err := jsonparser.Get(value, "hashedToken"); err == nil {


Should you check if err is not nil? It appears that this method will throw errors for a few different reasons. It may be good to add a safety check if err != nil.

devanbenz · 2025-02-27T16:26:29Z

authorization/storage_authorization.go

+		allHashes, err := s.hasher.AllHashes(token)
+		if err != nil {
+			s.log.Error("error generating hashes in filterPredicateFn", zap.Error(err))
+			// On error, continue onward. allHashes is empty and we'll effectively ignore hashedToken,


devanbenz

Adding in a few more comments

devanbenz · 2025-02-27T16:28:25Z

authorizer/task_test.go

+				Bucket:                 "holder",
+				RetentionPeriodSeconds: 1,
+			})
+			require.NoError(t, err)


Context string for NoError

devanbenz · 2025-02-27T16:29:48Z

authorizer/task_test.go

-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)


context string for NoError

devanbenz · 2025-02-27T16:29:57Z

authorizer/task_test.go

-		t.Fatal(err)
-	}
+	err = svc.CreateOrganization(context.Background(), otherOrg)
+	require.NoError(t, err)


context string for NoError

devanbenz · 2025-02-27T16:30:33Z

authorizer/task_test.go

-		t.Fatal(err)
-	}
+	err := all.Up(context.Background(), zaptest.NewLogger(t), store)
+	require.NoError(t, err)


Context string for NoError

devanbenz · 2025-02-27T16:30:41Z

authorizer/task_test.go

-		t.Fatal(err)
-	}
+	authStore, err := authorization.NewStore(context.Background(), store, useHashedTokens)
+	require.NoError(t, err)


Context string for NoError

devanbenz · 2025-02-27T16:49:05Z

cmd/influxd/recovery/auth/auth.go

 	ctx := context.Background()
 	store := bolt.NewKVStore(cmd.logger.With(zap.String("system", "bolt-kvstore")), cmd.boltPath)
 	if err := store.Open(ctx); err != nil {
 		return err
 	}
-	defer store.Close()
+	defer func() {
+		rErr = errors.Join(store.Close(), rErr)


For these defers can errors2.Capture be used?
see

influxdb/tsdb/index/tsi1/partition.go

Line 301 in 1efb8da

defer errors2.Capture(&rErr, dir.Close)()

devanbenz · 2025-02-27T16:50:06Z

cmd/influxd/recovery/auth/auth.go

+		if t.Token != "" {
+			token = t.Token
+		} else if t.HashedToken != "" {
+			token = "REDACTED"


Could "N/A" and "REDACTED" be Enums or consts?

devanbenz · 2025-02-27T16:50:36Z

cmd/influxd/recovery/testhelper/influxd.bolt.testdata

Should this be added? I assume yes because its post fixed with testdata, but, just checking.

devanbenz · 2025-02-27T16:52:02Z

cmd/influxd/upgrade/security_test.go

+
+				for _, want := range tc.want {
+					actual, err := v2.authSvc.FindAuthorizationByToken(ctx, want.Token)
+					require.NoError(t, err)


Please add string context to NoError

devanbenz · 2025-02-27T16:52:11Z

cmd/influxd/upgrade/security_test.go

-				}
-				if diff := cmp.Diff(want.Status, actual.Status); diff != "" {
-					t.Fatal(diff)
+					require.NoError(t, err)


String context to NoError

devanbenz

Going to pick back up my review next week for a second pass. Consider this first pass finished up.

devanbenz · 2025-02-27T18:24:00Z

cmd/influxd/upgrade/security_test.go

-		t.Run(tc.name, func(t *testing.T) { // better do not run in parallel
-			ctx := context.Background()
-			log := zaptest.NewLogger(t)
+		for _, useHashedTokens := range []bool{false, true} {


Please add strings with context to NoError's in this function.

Also did not mean to approve. Dismissing my approval.

Dismissing

gwossum added kind/feature area/2.x OSS 2.0 related issues and PRs labels Feb 6, 2025

gwossum self-assigned this Feb 6, 2025

gwossum mentioned this pull request Feb 6, 2025

feat: add optional token hashing #25979

Closed

davidby-influx requested changes Feb 10, 2025

View reviewed changes

davidby-influx requested changes Feb 20, 2025

View reviewed changes

davidby-influx requested changes Feb 25, 2025

View reviewed changes

gwossum added 9 commits February 25, 2025 15:36

feat: add optional token hashing

1fdaa41

Add optional token hashing with `--use-hashed-tokens` command line option.

chore: rollback changes to v1/authorization

acd9dd5

chore: fix static check issues, take advantage of go1.23 feature

ef29348

chore: replace deprecated call

9a87ce1

fix: prevent duplicate tokens from being created

5279970

Create duplicate tokens from being created. This is a bug introduced earlier in this PR. Also improve tests so they detect the bug and use testify throughout.

chore: variety of minor fixes

9d008ea

Address PR issues on comments, error handling, and add final token matching check in `Store.GetAuthorizationByToken`.

chore: improve comments, error handling, logging, and edge cases

80cf2e6

Changes in addition to minor cleanups: - `authentication.Store` can now log info and warnings - Improved logic in `UpdateAuthorization` when both Token and HashedToken are set. Added supporting test cases.

chore: fix spelling and grammar mistakes, remove dead code

6aa2d1a

gwossum force-pushed the gw/edge306/token_hashing branch from 15f8e92 to 6aa2d1a Compare February 25, 2025 21:42

devanbenz requested changes Feb 26, 2025

View reviewed changes

devanbenz reviewed Feb 27, 2025

View reviewed changes

devanbenz previously approved these changes Feb 28, 2025

View reviewed changes

devanbenz self-requested a review February 28, 2025 19:00

feat: add optional token hashing #25982

Are you sure you want to change the base?

feat: add optional token hashing #25982

Conversation

gwossum commented Feb 6, 2025

davidby-influx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidby-influx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidby-influx left a comment

Choose a reason for hiding this comment

devanbenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devanbenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devanbenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devanbenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment