Remove linked addresses from KV #7725

N-o-Z · 2024-05-06T09:46:37Z

Change Description

Background

Improve performance on write request using Get/Link physical address
Today address are written to KV under the repository partition for validation purposes.
This requires reading, writing and deleting of the linked address kv entry for every write.
As a result we are limited to 500 requests/sec.
To remove this limitation, it was decided to not track physical addresses generated by lakeFS in KV.
Instead, we chose a more lenient approach which only verifies the linked address can be used for a specific repository/branch/path and in a given timeframe

Testing Details

Added/modified unit tests

Breaking Change?

No

Additional info

Introduced a new configuration variable for the secret signing key for the linked address signature. It is required, but for backward compatibility purposes was given a default value which must be modified in production

github-actions · 2024-05-06T09:47:48Z

♻️ PR Preview 63ee494 has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

github-actions · 2024-05-06T09:52:53Z

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed

Thanks!

No large changes. Most worrying one is actually that the we encourage confusing the URL signing key with the really secret signing keys.

Also having the signing configuration in blockstore makes sense, so maybe the digest code should live in block/adapter.go?

arielshaqed · 2024-05-06T09:49:51Z

pkg/api/controller.go

@@ -631,6 +652,53 @@ func (c *Controller) StsLogin(w http.ResponseWriter, r *http.Request, body apige
 	writeResponse(w, r, http.StatusOK, responseToken)
 }

+func (c *Controller) verifyLinkAddress(repository, branch, path, physicalAddress string) error {


I think I might prefer for this to be on Catalog. Controller is supposed to manage the protocol, while Catalog is supposed to manage... other things. Suppose we decide to add some fake S3 call to the gateway that links - how would we acccess it?

It would also make this function easier to test -- pkg/api is our most slowest most complex "unit" test.

In any case not in this PR, but consider opening an issue.

arielshaqed · 2024-05-06T09:50:37Z

pkg/api/controller.go

@@ -631,6 +652,53 @@ func (c *Controller) StsLogin(w http.ResponseWriter, r *http.Request, body apige
 	writeResponse(w, r, http.StatusOK, responseToken)
 }

+func (c *Controller) verifyLinkAddress(repository, branch, path, physicalAddress string) error {
+	address, signature, found := strings.Cut(physicalAddress, ",")


This assumes storage namespace does not contain any colons. But it could!

How would you recommend to approach this?
Any set of characters can potential exist in the storage namespace.

arielshaqed · 2024-05-06T09:53:54Z

pkg/api/controller.go

@@ -5121,27 +5188,6 @@ func resolvePathList(objects, prefixes *[]string) []catalog.PathRecord {
 	return pathRecords
 }

-func NewController(cfg *config.Config, catalog *catalog.Catalog, authenticator auth.Authenticator, authService auth.Service, authenticationService authentication.Service, blockAdapter block.Adapter, metadataManager auth.MetadataManager, migrator Migrator, collector stats.Collector, cloudMetadataProvider cloud.MetadataProvider, actions actionsHandler, auditChecker AuditChecker, logger logging.Logger, sessionStore sessions.Store, pathProvider upload.PathProvider, usageReporter stats.UsageReporterOperations) *Controller {


Yah, looks like you moved it to a better place.

arielshaqed · 2024-05-06T09:54:32Z

pkg/api/controller.go

@@ -5325,7 +5371,7 @@ func (c *Controller) GetUsageReportSummary(w http.ResponseWriter, r *http.Reques
 	writeResponse(w, r, http.StatusOK, response)
 }

-func (c *Controller) CreateUserExternalPrincipal(w http.ResponseWriter, r *http.Request, body apigen.CreateUserExternalPrincipalJSONRequestBody, userID string, params apigen.CreateUserExternalPrincipalParams) {
+func (c *Controller) CreateUserExternalPrincipal(w http.ResponseWriter, r *http.Request, _ apigen.CreateUserExternalPrincipalJSONRequestBody, userID string, params apigen.CreateUserExternalPrincipalParams) {


Why this change now?

Just my fmt

arielshaqed · 2024-05-06T09:56:54Z

pkg/config/defaults.go

 		viper.SetDefault(BlockstoreTypeKey, "local")
 	}

+	viper.SetDefault("blockstore.encrypt.secret_key", DefaultSecretSigningKey)


Please let's use a different default here. This secret key considerably less secret, is really cheap to change. By using the same default we encourage people to give it the same value.

arielshaqed · 2024-05-06T09:58:57Z

pkg/api/controller.go

+	return nil
+}
+
+func (c *Controller) encryptAddress(logicalAddress string) string {


Suggested change

func (c *Controller) encryptAddress(logicalAddress string) string {

func (c *Controller) signAddress(logicalAddress string) string {

arielshaqed · 2024-05-06T09:59:51Z

pkg/api/controller.go

+
+func (c *Controller) getAddressWithSignature(repository, branch, path string) string {
+	physicalPath := c.PathProvider.NewPath()
+	return physicalPath + "," + c.encryptAddress(getAddressToSign(repository, branch, path, physicalPath))


"," should be a const somewhere, it will make it clearer that verification uses the same thing.

arielshaqed · 2024-05-06T10:01:24Z

pkg/api/controller.go

+	h := hmac.New(sha256.New, []byte(c.Config.Blockstore.Encrypt.SecretKey))
+	h.Write([]byte(stringToVerify))
+	calculated := h.Sum(nil)


I think this can and should use encryptAddress() (or whatever it's new name will be).

arielshaqed · 2024-05-06T10:02:54Z

pkg/config/config.go

@@ -221,6 +221,9 @@ type Config struct {
 		} `mapstructure:"ui_config"`
 	} `mapstructure:"auth"`
 	Blockstore struct {
+		Encrypt struct {


Suggested change

Encrypt struct {

Signing struct {

arielshaqed · 2024-05-06T10:54:27Z

One more thing!

Deploying this change will invalidate all in-fought presigned URLs. We need to announce that. Please add a changelog entry to that effect.

itaiad200

Partial review, will continue later

itaiad200 · 2024-05-06T12:43:31Z

CHANGELOG.md

@@ -1,6 +1,11 @@
 # Changelog

-# v1.20.0
+# Unreleased


Do we still do this?

Request by @arielshaqed

itaiad200 · 2024-05-06T12:45:42Z

esti/lakectl_util.go

@@ -31,7 +31,7 @@ var (
 	reShortCommitID   = regexp.MustCompile(`[\d|a-f]{16}`)
 	reChecksum        = regexp.MustCompile(`([\d|a-f]{32})|(0x[0-9A-F]{15})`)
 	reEndpoint        = regexp.MustCompile(`https?://\w+(:\d+)?/api/v\d+/`)
-	rePhysicalAddress = regexp.MustCompile(`/data/[0-9a-v]{20}/[0-9a-v]{20}`)
+	rePhysicalAddress = regexp.MustCompile(`/data/[0-9a-v]{20}/[0-9a-v]{20}?(,.+)*`)


Now you have 2 problems....
And one of them is /data/aaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaa,sd,cds,sdc,sdc,sdc,sd,csdc,

itaiad200 · 2024-05-06T12:53:47Z

pkg/api/controller_test.go

-		t.Fatal(err)
-	}
-
-	t.Run("get and link physical address", func(t *testing.T) {


AFAIC you don't have an happy flow test now..

itaiad200 · 2024-05-06T12:55:19Z

pkg/config/config.go

@@ -221,6 +221,9 @@ type Config struct {
 		} `mapstructure:"ui_config"`
 	} `mapstructure:"auth"`
 	Blockstore struct {
+		Signing struct {
+			SecretKey SecureString `mapstructure:"secret_key" validate:"required"`


Breaking change... remove the required part

Its required but has a default per discussion

It cannot work otherwise

itaiad200 · 2024-05-06T12:56:17Z

pkg/graveler/graveler.go

Best PR EVER!!!

itaiad200 · 2024-05-06T12:59:10Z

pkg/catalog/catalog.go

@@ -2768,6 +2744,53 @@ func (c *Catalog) listRepositoriesHelper(ctx context.Context) ([]*graveler.Repos
 	return repos, nil
 }

+func (c *Catalog) VerifyLinkAddress(repository, branch, path, physicalAddress string) error {
+	address, signature, found := strings.Cut(physicalAddress, LinkAddressSigningDelimiter)


Will the following storage namespace work?
s3://bucket/s,o,m,e/p,a,t,h/

This still fails for some storage namespaces.

It seems to me that we are putting a lot of effort into writing a single incorrect line of code, when we could write <5 correct lines like here.

@N-o-Z as Ariel mentioned, the issue still exists

itaiad200 · 2024-05-06T13:11:01Z

pkg/catalog/catalog.go

+}
+
+func getAddressToSign(repository, branch, path, physicalAddress string) string {
+	return repository + branch + path + physicalAddress


Will these calls return different address to sign?

getAddressToSign("aa", "bb", "cc", "dd")

getAddressToSign("a", "ab", "bc", "cdd")

Not sure. Why not just sign the actual generated physical address?

We said that signature should be dependant on repo, branch and path (i.e. you can only use a linked address with a single logical path)

Yes

@N-o-Z how so? It's just string concatenation..

OK, to answer both questions:

If we sign only the physical address, then users can user generated linked addresses of repo1/branch2/path3 in repo4/branch5/path6

Because physical address is always unique, the prefix for the address to sign will always be unique and therefore we will never get to a situation of:
getAddressToSign("aa", "bb", "cc", "dd")
getAddressToSign("a", "ab", "bc", "cdd")

However, regarding 2, if it makes more sense I can add some separator between the parts of the signature

itaiad200 · 2024-05-06T13:20:44Z

cmd/lakefs/cmd/run.go


 	jobData := []struct {
 		name     string
 		interval time.Duration
 		fn       func(context.Context)
 	}{
-		{
-			name:     "delete expired link addresses",


Minor: Some tokens will be kept forever in the kv..

I think we shouldn't invest on the cleanup as this will probably won't make a dent in the DB

itaiad200 · 2024-05-06T13:21:43Z

pkg/catalog/catalog.go

+	h := hmac.New(sha256.New, []byte(c.signingKey))
+	h.Write([]byte(stringToVerify))
+	calculated := h.Sum(nil)


Extract to func?

pretty please? 🥺

Sorry, missed that

arielshaqed

Thanks!

I think we can do a bit better on signing the exact string of the physical address, and on precisely extracting the digest. They are both <10 lines of code, combined.

arielshaqed · 2024-05-06T13:43:59Z

pkg/catalog/catalog.go

@@ -2768,6 +2744,53 @@ func (c *Catalog) listRepositoriesHelper(ctx context.Context) ([]*graveler.Repos
 	return repos, nil
 }

+func (c *Catalog) VerifyLinkAddress(repository, branch, path, physicalAddress string) error {
+	address, signature, found := strings.Cut(physicalAddress, LinkAddressSigningDelimiter)


This still fails for some storage namespaces.

It seems to me that we are putting a lot of effort into writing a single incorrect line of code, when we could write <5 correct lines like here.

arielshaqed · 2024-05-06T13:45:10Z

pkg/catalog/catalog.go

+}
+
+func getAddressToSign(repository, branch, path, physicalAddress string) string {
+	return repository + branch + path + physicalAddress


Not sure. Why not just sign the actual generated physical address?

itaiad200

Some of the comments were not resolved..

itaiad200 · 2024-05-07T16:37:05Z

pkg/catalog/catalog.go

@@ -2768,6 +2744,53 @@ func (c *Catalog) listRepositoriesHelper(ctx context.Context) ([]*graveler.Repos
 	return repos, nil
 }

+func (c *Catalog) VerifyLinkAddress(repository, branch, path, physicalAddress string) error {
+	address, signature, found := strings.Cut(physicalAddress, LinkAddressSigningDelimiter)


@N-o-Z as Ariel mentioned, the issue still exists

itaiad200 · 2024-05-07T16:37:35Z

pkg/catalog/catalog.go

+	h := hmac.New(sha256.New, []byte(c.signingKey))
+	h.Write([]byte(stringToVerify))
+	calculated := h.Sum(nil)


pretty please? 🥺

itaiad200 · 2024-05-07T16:38:22Z

pkg/catalog/catalog.go

+}
+
+func getAddressToSign(repository, branch, path, physicalAddress string) string {
+	return repository + branch + path + physicalAddress


Yes

@N-o-Z how so? It's just string concatenation..

arielshaqed

Thanks! But please be sure of the regexp :-/

esti/lakectl_util.go

itaiad200

LGTM

Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>

* Remove linked addresses from KV * CR Fixes * Add changelog * Change delimiter and encoding * More fixes * More fixes 2 * More More Fixes * Update esti/lakectl_util.go Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io> * Fix azure regex --------- Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>

N-o-Z added include-changelog PR description should be included in next release changelog area/KV Improvements to the KV store implementation labels May 6, 2024

N-o-Z requested a review from arielshaqed May 6, 2024 09:46

N-o-Z self-assigned this May 6, 2024

arielshaqed requested changes May 6, 2024

View reviewed changes

N-o-Z requested a review from arielshaqed May 6, 2024 12:40

itaiad200 requested changes May 6, 2024

View reviewed changes

arielshaqed reviewed May 6, 2024

View reviewed changes

N-o-Z requested review from arielshaqed and itaiad200 May 6, 2024 14:02

guy-har added the performance label May 7, 2024

itaiad200 requested changes May 7, 2024

View reviewed changes

N-o-Z requested a review from itaiad200 May 8, 2024 11:58

arielshaqed approved these changes May 8, 2024

View reviewed changes

esti/lakectl_util.go Outdated Show resolved Hide resolved

itaiad200 approved these changes May 8, 2024

View reviewed changes

N-o-Z and others added 8 commits May 8, 2024 18:19

Remove linked addresses from KV

94aeeda

CR Fixes

8088771

Add changelog

1b3b41b

Change delimiter and encoding

e77bb24

More fixes

32481fc

More fixes 2

b89a26d

More More Fixes

46a55a5

Update esti/lakectl_util.go

17f6a4a

Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>

N-o-Z force-pushed the task/remove-linked-addresses-from-kv-7716 branch from 6edb8c4 to 17f6a4a Compare May 8, 2024 15:19

N-o-Z enabled auto-merge (squash) May 8, 2024 15:20

Fix azure regex

63ee494

N-o-Z disabled auto-merge May 8, 2024 16:56

N-o-Z merged commit 686802c into master May 8, 2024
35 of 36 checks passed

N-o-Z deleted the task/remove-linked-addresses-from-kv-7716 branch May 8, 2024 17:01

	func (c *Controller) encryptAddress(logicalAddress string) string {
	func (c *Controller) signAddress(logicalAddress string) string {

Remove linked addresses from KV #7725

Remove linked addresses from KV #7725

Conversation

N-o-Z commented May 6, 2024

Change Description

Background

Testing Details

Breaking Change?

Additional info

github-actions bot commented May 6, 2024 • edited

github-actions bot commented May 6, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed 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

Choose a reason for hiding this comment

arielshaqed commented May 6, 2024

itaiad200 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaiad200 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

arielshaqed left a comment

Choose a reason for hiding this comment

itaiad200 left a comment

Choose a reason for hiding this comment

github-actions bot commented May 6, 2024 •

edited