Relationship integrity #1980

josephschorr · 2024-07-08T21:23:53Z

Fixes: #1953

jzelinskie · 2024-07-08T21:27:06Z

internal/datastore/crdb/crdb.go

+	colRelation          = "relation"
+	colUsersetNamespace  = "userset_namespace"
+	colUsersetObjectID   = "userset_object_id"
+	colUsersetRelation   = "userset_relation"


maybe put a new line here to keep all the caveat stuff together

jzelinskie · 2024-07-08T21:27:55Z

internal/datastore/crdb/migrations/zz_migration.0007_add_integrity_columns.go

+)
+
+func init() {
+	err := CRDBMigrations.Register("add-integrity-columns", "add-relationship-counters-table", addIntegrityColumns, noAtomicMigration)


is there a must register? i swear it's been written at some point

Nope, we never added it. We could, but this seems fine

jzelinskie · 2024-07-08T21:29:32Z

internal/datastore/crdb/reader.go

@@ -36,6 +36,9 @@ var (
 		colUsersetRelation,
 		colCaveatContextName,
 		colCaveatContext,
+		colIntegrityKeyID,
+		colIntegrityHash,
+		colTimestamp,


do we need the timestamp if we have the key id?

Yes, to ensure that if a key is expired, we can still accept relationships signed from before the expiration time

jzelinskie · 2024-07-08T21:32:14Z

internal/datastore/proxy/relationshipintegrity.go

+	var sb strings.Builder
+	sb.WriteString(tpl.ResourceAndRelation.Namespace)
+	sb.WriteString(":")
+	sb.WriteString(tpl.ResourceAndRelation.ObjectId)
+	sb.WriteString("#")
+	sb.WriteString(tpl.ResourceAndRelation.Relation)
+	sb.WriteString("@")
+	sb.WriteString(tpl.Subject.Namespace)
+	sb.WriteString(":")
+	sb.WriteString(tpl.Subject.ObjectId)
+	sb.WriteString("#")
+	sb.WriteString(tpl.Subject.Relation)
+
+	if tpl.Caveat != nil && tpl.Caveat.CaveatName != "" {
+		sb.WriteString(" with ")
+		sb.WriteString(tpl.Caveat.CaveatName)
+		sb.WriteString(":")
+		sb.WriteString(tpl.Caveat.Context.String())
+	}


is this something we can put in the tuple package? maybe something similar already exists

Yes, we could put it into there

jzelinskie · 2024-07-08T21:33:37Z

internal/datastore/proxy/relationshipintegrity.go

+func (r *relationshipIntegrityProxy) CheckRevision(ctx context.Context, revision datastore.Revision) error {
+	return r.ds.CheckRevision(ctx, revision)
+}
+
+func (r *relationshipIntegrityProxy) Close() error {
+	return r.ds.Close()
+}
+
+func (r *relationshipIntegrityProxy) Features(ctx context.Context) (*datastore.Features, error) {
+	return r.ds.Features(ctx)
+}
+
+func (r *relationshipIntegrityProxy) HeadRevision(ctx context.Context) (datastore.Revision, error) {
+	return r.ds.HeadRevision(ctx)
+}
+
+func (r *relationshipIntegrityProxy) OptimizedRevision(ctx context.Context) (datastore.Revision, error) {
+	return r.ds.OptimizedRevision(ctx)
+}
+
+func (r *relationshipIntegrityProxy) ReadyState(ctx context.Context) (datastore.ReadyState, error) {
+	return r.ds.ReadyState(ctx)
+}
+
+func (r *relationshipIntegrityProxy) RevisionFromString(serialized string) (datastore.Revision, error) {
+	return r.ds.RevisionFromString(serialized)
+}
+
+func (r *relationshipIntegrityProxy) Statistics(ctx context.Context) (datastore.Stats, error) {
+	return r.ds.Statistics(ctx)
+}


i like to make functions like these 1 single line so they all stack together without newlines between. it makes it more clear that they're just boilerplate

Tried but go is reformatting it back to this

Likely because the receiver is named relationshipIntegrityProxy and thus its making the lines too long. Shall I rename the struct?

ecordell · 2024-07-08T21:33:53Z

internal/datastore/proxy/relationshipintegrity.go

+	currentKeyHMAC := hmacConfig{
+		keyID:     currentKey.ID,
+		expiredAt: currentKey.ExpiredAt,
+		hmacPool: &sync.Pool{


I'm sure it depends, but for tiger I found that a buffered channel performed better than sync.Pool (select on retrieving from the pool to return a new instance if the pool is empty, similar to sync.Pool)

ecordell · 2024-07-08T21:34:36Z

internal/datastore/proxy/relationshipintegrity.go

+			keyID:     key.ID,
+			expiredAt: key.ExpiredAt,
+			hmacPool: &sync.Pool{
+				New: func() any { return hmac.New(alg, key.Bytes) },


it's probably worth benchmarking sha3 here, since you can use it as a MAC directly instead of through an HMAC

I also learned recently about https://github.com/google/highwayhash (and its Go native impl, https://github.com/minio/highwayhash), theoretically 1 order of magnitude faster than SHA256 thanks to SIMD. The go implementation says:

HighwayHash is not a general-purpose cryptographic hash function (such as Blake2b, SHA-3 or SHA-2) and should not be used if strong collision resistance is required.

But the reference C++ implementation from Google says:

Given a strong hash function and secret seed, it appears infeasible for attackers to generate hash collisions because s and/or R are unknown. However, they can still observe the timings of data structure operations for various m. With typical table sizes of 2^10 to 2^17 entries, attackers can detect some 'bin collisions' (inputs mapping to the same bin). Although this will be costly for the attacker, they can then send many instances of such inputs, so we need to limit the resulting work for our data structure.

Did some prelim tests and highwayhash is faster by about ~50% while SHA3 seemed to be slower.

However, I'm concerned about the lack of strong collision resistance. We'll need to investigate the implications of that.

Updated branch with the highwayhash impl for testing

The scenario this attempts to protect from is unauthorized modification of the state of the database. This requires sophisticated attacks and computing running for days to find a collision that could reveal the seed. If someone has access to the database and the machinery to run such an attack, the hash is not going to get in the way honestly, they probably have enough incentives to find other ways. I don't think remote attacks, with all the level of indirections, can give a clear signal to an attacker.

This is a decision the customer could themselves make and we could make it configurable via a flag. If the customer considers only collision-resistant hashes acceptable and are willing to accept the tradeoff in computational cost, then they can choose to do so. Highwayhash is by no means a "weak" hash, but one that makes the attack cost be sufficiently high to be unfeasible to most attackers.

By contrast, 'strong' hashes such as SipHash or HighwayHash require infeasible attacker effort to find a hash collision (an expected 2^32 guesses of m per the birthday paradox) or recover the seed (2^63 requests). These security claims assume the seed is secret. It is reasonable to suppose s is initially unknown to attackers, e.g. generated on startup or even per-connection.

Consider the authors describe how complicated an attack is:

A timing attack by Wool/Bar-Yosef recovers 13-bit seeds by testing all 8K possibilities using millions of requests, which takes several days (even assuming unrealistic 150 us round-trip times). It appears infeasible to recover 64-bit seeds in this way.

After discussion, switching back to sha2

The context here was that highwayhash isn't suitable for hmac (or at least we don't know that it is)

internal/datastore/proxy/relationshipintegrity.go

ecordell

Generally looking good, I left some comments throughout, but my main concerns are:

I don't see where the integrity proxy gets configured and installed...?
I don't see where/how spicedb loads signing keys. Is a commit missing?
I have some concerns about these fields being included in every query and not just when integrity is enabled. Wouldn't it be better to not query for those fields at all? I think this will mess with covering indexes, since I don't think we want them included in indexes.
I think we need to have something in place for key rotation and, maybe more importantly, evolution of the canonical form. Let's say we add an extra field for native TTLs to tuples and we migrate caveat TTLs to a native form - that will require changes in the canonical form and re-signing with the keys.

ecordell · 2024-08-05T16:35:43Z

internal/datastore/crdb/readwrite.go

@@ -51,7 +51,7 @@ type crdbReadWriteTXN struct {

 var (
 	upsertTupleSuffix = fmt.Sprintf(
-		"ON CONFLICT (%s,%s,%s,%s,%s,%s) DO UPDATE SET %s = now(), %s = excluded.%s, %s = excluded.%s WHERE (relation_tuple.%s <> excluded.%s OR relation_tuple.%s <> excluded.%s)",
+		"ON CONFLICT (%s,%s,%s,%s,%s,%s) DO UPDATE SET %s = now(), %s = excluded.%s, %s = excluded.%s, %s = excluded.%s, %s = excluded.%s WHERE (relation_tuple.%s <> excluded.%s OR relation_tuple.%s <> excluded.%s)",


We might want to have alternate forms of this query for use when relationship integrity is enabled. Not sure what the performance penalty is for checking additional fields.

Yeah, this is a common "issue" with adding these columns, but it does mean having multiple copies, which means more testing

ecordell · 2024-08-05T22:09:51Z

internal/datastore/crdb/watch.go

+
+			if details.After != nil && details.After.IntegrityKeyID != nil && details.After.IntegrityHashAsHex != nil && details.After.TimestampAsString != nil {
+				hexString := *details.After.IntegrityHashAsHex
+				hashBytes, err := hex.DecodeString(hexString[2:]) // drop the \x


I'm assuming I'll get to a test that shows this is fine but I thought CRDB used \x prefix for just a single hex byte, and x'asdfasdf' for a whole string.

Not according to testing

ecordell · 2024-08-05T22:10:33Z

internal/datastore/crdb/watch.go

+				}
+
+				timestampString := *details.After.TimestampAsString
+				parsedTime, err := time.Parse("2006-01-02T15:04:05.999999999", timestampString)


ecordell · 2024-08-05T22:18:37Z

internal/datastore/mysql/migrations/zz_migration.0009_add_relationship_integrity_columns.go

+
+func addRelationshipIntegrityColumns(t *tables) string {
+	return fmt.Sprintf(`ALTER TABLE %s
+		ADD COLUMN integrity_key_id VARCHAR(255) DEFAULT NULL, 


do we really need 255 bytes for a key id?

No, but do you have a good limit for it?

ecordell · 2024-08-05T22:18:58Z

internal/datastore/mysql/migrations/zz_migration.0009_add_relationship_integrity_columns.go

+func addRelationshipIntegrityColumns(t *tables) string {
+	return fmt.Sprintf(`ALTER TABLE %s
+		ADD COLUMN integrity_key_id VARCHAR(255) DEFAULT NULL, 
+		ADD COLUMN integrity_hash TINYBLOB DEFAULT NULL,


Why tinyblob and not varbinary?

Will switch; wasn't aware of it

ecordell · 2024-08-05T22:45:45Z

internal/datastore/proxy/relationshipintegrity.go

+	}
+
+	hashWithoutByte := tpl.Integrity.Hash[1:]
+	if tpl.Integrity.Hash[0] != versionByte || len(hashWithoutByte) != hashLength {


why is the version byte on the hash and not a part of the key id?

ecordell · 2024-08-05T22:50:09Z

pkg/tuple/tuple.go

+		return nil, nil
+	}
+
+	var sb strings.Builder


there is a bytes.Builder

ecordell · 2024-08-05T22:54:48Z

internal/datastore/proxy/relationshipintegrity.go

+	return r.ReadWriteTransaction.BulkLoad(ctx, wrapped)
+}
+
+type wrappedBulkLoadIterator struct {


Can we name it something like hmacAddingBulkLoadIterator etc?

^

edit by @josephschorr: to what is this referring? GitHub is showing no context

internal/datastore/spanner/watch.go

internal/datastore/proxy/relationshipintegrity.go

josephschorr · 2024-08-13T21:14:43Z

Redesigned and rebased as discussed

ecordell · 2024-08-26T18:26:22Z

internal/datastore/crdb/crdb.go

@@ -447,6 +459,18 @@ func (cds *crdbDatastore) Features(ctx context.Context) (*datastore.Features, er
 	return features, err
 }

+func (cds *crdbDatastore) SupportsIntegrity() bool {


Any reason this isn't just another feature returned by the features method?

I wanted it to match an interface in the proxy around casting; if you like, I can move it into Features and just have the proxy check that call instead

I think a feature makes more sense - we do that for other stuff like Watch, etc

You added offline features, but this is still there?

Missed it. Removed

ecordell · 2024-08-26T18:37:31Z

internal/datastore/crdb/crdb_test.go

+			)
+			require.NoError(t, err)
+
+			wrapped, err := proxy.NewRelationshipIntegrityProxy(ds, defaultKeyForTesting, nil)


Why not just instantiate this inside NewCRDBDatastore so you can't forget if you set WithIntegrity(true)?

Couldn't the unwrappedTester just set WithIntegrity(false)?

Because it exists outside of CRDB's implementation; its a generic proxy that currently works with CRDB and Memdb (for testing)

It just seems confusing that you have to do both - if you forget to add the proxy in, you will think you have integrity enabled but there will be no validation

Well, no, you'd have integrity data storage enabled, but it would fail due to checks ensuring the data is actually present. Having it be in its own proxy file makes testing easier

internal/datastore/memdb/memdb.go

internal/datastore/postgres/common/bulk.go

ecordell · 2024-08-26T20:15:43Z

internal/datastore/proxy/relationshipintegrity.go

+	return r.ReadWriteTransaction.BulkLoad(ctx, wrapped)
+}
+
+type wrappedBulkLoadIterator struct {


^

edit by @josephschorr: to what is this referring? GitHub is showing no context

internal/datastore/proxy/relationshipintegrity_test.go

pkg/datastore/test/tuples.go

ecordell · 2024-08-26T20:30:57Z

pkg/tuple/tuple.go

+		return nil, nil
+	}
+
+	var sb strings.Builder


ecordell · 2024-08-26T20:46:29Z

internal/datastore/proxy/relationshipintegrity.go

+		return nil, err
+	}
+
+	key.lock.Lock()


this seems like it would be a bottleneck for large reads. what about a pool of hashers instead? either a sync.Pool or even just a chan hasher

I had a pool of them at first, but removed it because I didn't think we needed to lock it. I've added the pool back in

internal/datastore/proxy/relationshipintegrity.go

josephschorr · 2024-08-26T21:36:16Z

Updated

josephschorr · 2024-08-27T18:51:42Z

Updated

ecordell · 2024-08-29T17:45:58Z

internal/datastore/crdb/readwrite.go

+}
+
+func (rwt *crdbReadWriteTXN) queryWriteTuple() sq.InsertBuilder {
+	if rwt.withIntegrity {


sorry for the late note - can we remove these ifs from every call? i.e. pick a different implementation of crdbReadWriteTXN on initialization instead?

That would be a lot of copy-paste of code and reducing reuse a lot

Currently only supported in memdb and CRDB drivers

josephschorr requested a review from a team as a code owner July 8, 2024 21:23

josephschorr marked this pull request as draft July 8, 2024 21:23

github-actions bot added area/datastore Affects the storage system area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Jul 8, 2024

jzelinskie reviewed Jul 8, 2024

View reviewed changes

ecordell reviewed Jul 8, 2024

View reviewed changes

josephschorr force-pushed the relationship-integrity branch from 7ed1e6d to 2308290 Compare July 11, 2024 19:25

github-actions bot added the area/dependencies Affects dependencies label Jul 11, 2024

ecordell reviewed Jul 11, 2024

View reviewed changes

internal/datastore/proxy/relationshipintegrity.go Outdated Show resolved Hide resolved

josephschorr force-pushed the relationship-integrity branch 8 times, most recently from 9452735 to 3133123 Compare July 18, 2024 18:21

josephschorr marked this pull request as ready for review July 18, 2024 18:36

ecordell reviewed Aug 5, 2024

View reviewed changes

josephschorr force-pushed the relationship-integrity branch from 3133123 to f01c4f1 Compare August 13, 2024 21:14

github-actions bot added area/CLI Affects the command line area/api v1 Affects the v1 API and removed area/dependencies Affects dependencies labels Aug 13, 2024

josephschorr force-pushed the relationship-integrity branch from f01c4f1 to c3e16f7 Compare August 13, 2024 21:22

ecordell reviewed Aug 26, 2024

View reviewed changes

josephschorr force-pushed the relationship-integrity branch from c3e16f7 to 489149c Compare August 26, 2024 21:36

josephschorr force-pushed the relationship-integrity branch 2 times, most recently from c7afa27 to b6ddc46 Compare August 27, 2024 18:51

josephschorr force-pushed the relationship-integrity branch 2 times, most recently from c0bead0 to fa9bed1 Compare August 29, 2024 15:44

ecordell reviewed Aug 29, 2024

View reviewed changes

josephschorr force-pushed the relationship-integrity branch from fa9bed1 to f0c1920 Compare August 30, 2024 21:39

ecordell previously approved these changes Sep 3, 2024

View reviewed changes

Implement support for relationship integrity

707f7bf

Currently only supported in memdb and CRDB drivers

josephschorr dismissed ecordell’s stale review via 707f7bf September 3, 2024 16:47

josephschorr force-pushed the relationship-integrity branch from f0c1920 to 707f7bf Compare September 3, 2024 16:47

ecordell approved these changes Sep 3, 2024

View reviewed changes

josephschorr added this pull request to the merge queue Sep 3, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 3, 2024

josephschorr added this pull request to the merge queue Sep 3, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 3, 2024

josephschorr added this pull request to the merge queue Sep 3, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 3, 2024

josephschorr added this pull request to the merge queue Sep 3, 2024

Merged via the queue into authzed:main with commit 6772a60 Sep 3, 2024
22 checks passed

josephschorr deleted the relationship-integrity branch September 3, 2024 18:57

github-actions bot locked and limited conversation to collaborators Sep 3, 2024

Relationship integrity #1980

Relationship integrity #1980

Conversation

josephschorr commented Jul 8, 2024 • edited Loading

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

vroldanbet Jul 9, 2024 • edited Loading

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

ecordell Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

ecordell 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

ecordell Aug 26, 2024 • edited by josephschorr Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephschorr commented Aug 13, 2024

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

ecordell Aug 26, 2024 • edited by josephschorr Loading

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

josephschorr commented Aug 26, 2024

josephschorr commented Aug 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephschorr commented Jul 8, 2024 •

edited

Loading

vroldanbet Jul 9, 2024 •

edited

Loading

ecordell Jul 18, 2024 •

edited

Loading

ecordell Aug 26, 2024 •

edited by josephschorr

Loading

ecordell Aug 26, 2024 •

edited by josephschorr

Loading