-
Notifications
You must be signed in to change notification settings - Fork 20
feat: Simpler authdb #821
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release-tee-caff-node
Are you sure you want to change the base?
feat: Simpler authdb #821
Conversation
Co-authored-by: Warp <warp@warp.dev>
66006be
to
e9655f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR simplifies the authenticated database (AuthDB) implementation by adopting a "tag every entry" approach instead of selectively tagging specific data types. The changes drastically reduce code complexity while maintaining cryptographic integrity guarantees.
Key changes:
- Unified authentication using a generic suffix-based tagging scheme for all key-value entries
- Introduced
AuthBatch
to ensure batched writes are authenticated atomically - Moved Espresso-specific operations to a dedicated
operations.go
file with type enforcement - Added
AuthIterator
to handle authentication during database iteration - Enhanced ancient store support with
AncientItemWithTag
wrapper
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
File | Description |
---|---|
espresso/authdb/schema.go |
Replaced per-type auth tag prefixes with a generic suffix-based approach |
espresso/authdb/operations.go |
New file containing Espresso-specific authenticated operations with type enforcement |
espresso/authdb/operations_test.go |
Added security enforcement tests verifying AuthDB/AuthBatch type requirements |
espresso/authdb/authdb.go |
Major refactor: unified Put/Get authentication logic, added AuthBatch/AuthIterator, enhanced ancient store support |
espresso/authdb/geth_schema.go |
Removed entire file (345 lines) - no longer needed with simplified approach |
espresso/authdb/accessors_chain.go |
Removed entire file (136 lines) - chain-specific accessors no longer required |
espresso/authdb/DEV.md |
Added comprehensive documentation explaining AuthDB architecture and security model |
arbnode/espresso_caff_node.go |
Updated to use new operations.go functions instead of direct AuthDB methods |
arbnode/espresso_batcher_addr_monitor.go |
Updated to use new operations.go functions for authenticated read/write operations |
justfile |
Simplified test command and added cleanup for new test directory |
espresso/authdb/.gitignore |
Added testancient directory to ignored paths |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
espresso/authdb/authdb.go
Outdated
func (it *AuthIterator) Value() []byte { | ||
key := it.Key() | ||
val := it.inner.Value() | ||
if it.db.mac == nil || !bytes.HasSuffix(key, genericAuthTagSuffix) { |
Copilot
AI
Oct 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition checks if the key has the suffix, but we're iterating over actual data keys that shouldn't have the suffix. The logic should be if it.db.mac == nil
only. The suffix check appears incorrect here since we already filtered out tag keys in Next().
if it.db.mac == nil || !bytes.HasSuffix(key, genericAuthTagSuffix) { | |
if it.db.mac == nil { |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is crucial comment, when I develop this, I also thought I shouldn't add extra filtering logic here since AuthIter.Next()
already done that, but if I don't, somehow the authdb_test
failed.
Then upon careful inspection, I realize the problem is the it.db.Get()
below is causing an undesirable recursion: because inside db.Get()
it will try to query -tag-tag
etc. which is clearly doesn't exist.
anyhow, this is a logic error that i fixed in 510516e, but directly accessing the underlying db via it.db.db.Get()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alxiong I didnt understand this, can you expand this on Slack or something for me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alxiong I wonder if we should also add tests for ancients because the current tests dont verify that
@@ -0,0 +1,144 @@ | |||
# Authenticated Storage (AuthDB) Architecture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should we not just name this file README.md
func (d *AuthDB) authenticateAncientData(kind string, number uint64, data []byte) ([]byte, error) { | ||
switch kind { | ||
case rawdb.ChainFreezerHashTable: | ||
dataSize := len(data) - d.mac.Size() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand what you are doing here:
dataSize := len(data) - d.mac.Size()
expectedTag := data[dataSize:]
actualData := data[:dataSize]
Why is d.mac.Size() relevant here if we always reset the mac?
// Only if tee is enalbed, add the auth tag to the auth db | ||
d.mac.Write(nextHotshotBlockNumKey) | ||
d.mac.Write(EncodeUint64(num)) | ||
tag := d.mac.Sum(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are now resetting before creating the tag?
|
||
func (d *AuthDB) ModifyAncients(fn func(ethdb.AncientWriteOp) error) (int64, error) { | ||
return d.db.ModifyAncients(fn) | ||
return op.inner.AppendRaw(kind, number, append(item, tag...)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep it consistent between Append and AppendRaw basically what we can do is create an authItem
and then rlp encode it so that its bytes are stored instead of doing this where we are no longer adding the authItem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay after reading the code more, I have realized that the data can always be added in AppendRaw and Append format. The only difference is one is rlp encoded and one is not. So AppendRaw should always contain the same data but just have it in rlp encoded form as I mentioned above
return &AuthBatch{inner: inner, authDB: d} | ||
} | ||
|
||
func (d *AuthDB) WasmDataBase() (ethdb.KeyValueStore, uint32) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we check if we need to auth around these wasm stuff as well?
if b.entries == nil { | ||
b.entries = make(map[string][]byte) | ||
} | ||
b.entries[string(key)] = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if inners Put
has an error? we still add it to the entries
} | ||
|
||
// Write commits the batch, adding auth tags for each entry if MAC is enabled | ||
func (b *AuthBatch) Write() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alxiong I saw that InspectFreezerTable also has MerkleStateFreezerName, VerkleStateFreezerName
not sure if we need to deal with those as well. Actually we need to look into WriteStateHistory
it has more tables which it is adding to ancients
} | ||
|
||
return height, nil | ||
func (d *AuthDB) ReadAncients(fn func(ethdb.AncientReaderOp) error) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we not adding auth to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alxiong one issue with this Ancients design is that when I try calling the reader.Ancients
on a database which is already tagged, it will not return anything to me because the authentication will fail because of this code. So I think we should still store it in the original format and store the tags separately
case rawdb.ChainFreezerBodiesTable, rawdb.ChainFreezerHeaderTable, rawdb.ChainFreezerReceiptTable:
authItem := new(AncientItemWithTag)
if err := rlp.DecodeBytes(data, authItem); err != nil {
log.Error("invalid freezer rlp", "err", err)
return nil, err
}
var buf bytes.Buffer
if err := rlp.Encode(&buf, authItem.item); err != nil {
log.Error("failed to RLP encode", "err", err)
return nil, err
}
tag := d.computeMacForAncient(kind, number, buf.Bytes())
if !hmac.Equal(tag, authItem.tag) {
log.Error("auth tag mismatch in AncientItemWithTag")
return nil, fmt.Errorf("auth tag mismatch for AncientItemWithTag, kind: %v, number: %v", kind, number)
}
return buf.Bytes(), nil
Closes #<ISSUE_NUMBER>
This PR:
Thanks to @jbearer's suggestion, our first draft should be the simplest but correct solution of "add auth tag to every entry in the kv store"
AuthDB.Get()
andAuthDB.Put()
to generic auth tag generation and verification, no more switch-case and different logic for each case.AuthBatch
that make sure the batchedWrite()
will add tags to all accumulated entries.operations.go
and enforce the reader/writer are AuthDB/Batch for correctness. (with a dedicated test inoperations_test.go
)AuthIterator
to wrap around a plainIterator
which originally step through the db without going throughPut
and Get`, thus require auth checks.AncientItemWithTag
and ensure theAncientKeyStore
write will append auth tag (to the struct ifAppend()
, or directly to the bytes ifAppendRaw()
) and read will check the tag.(please don't squash merge.)