Add basic authentication classes #166
Conversation
| binary.BigEndian.PutUint32(buf, header.Nonce) | ||
| hash := crypto.Keccak256(buf) | ||
|
|
||
| publicKeyBytes, err := hexutil.Decode(header.AccountID) |
There was a problem hiding this comment.
nit: does accountId need any validation before decoding?
There was a problem hiding this comment.
Shouldn't, if the string is invalid hex then the Decode method will fail.
|
|
||
| // Ensure the signature is 65 bytes (Recovery ID is the last byte) | ||
| if len(sig) != 65 { | ||
| return fmt.Errorf("signature length is unexpected: %d", len(sig)) |
There was a problem hiding this comment.
probably log accountId to help debug or whatever other identifier to uniquely identify this request
| // Nonce | ||
| Nonce uint32 `json:"nonce"` | ||
| // AuthenticationData is the signature of the blob header by the account ID | ||
| AuthenticationData []byte `json:"authentication_data"` |
There was a problem hiding this comment.
why don't we call it Signature?
There was a problem hiding this comment.
It's possible this could be more generic in the future... For instance, this could end up being data which gets passed along to the rollup account smart contract, whose semantics the EigenDA protocol is agnostic to. But I'm fine with calling it signature for now.
| @@ -40,6 +40,17 @@ type Blob struct { | |||
| Data []byte | |||
| } | |||
|
|
|||
| type BlobAuthHeader struct { | |||
| // Commitments | |||
| BlobCommitments `json:"commitments"` | |||
There was a problem hiding this comment.
can you comment on what this means and why we need this field?
There was a problem hiding this comment.
Added a comment to the BlobAuthHeader.
| // AccountID is the account that is paying for the blob to be stored | ||
| AccountID AccountID `json:"account_id"` | ||
| // Nonce | ||
| Nonce uint32 `json:"nonce"` |
There was a problem hiding this comment.
does it need to be incremented for every call? looks like Nonce is just being used as a message to sign
There was a problem hiding this comment.
Eventually this will be an incremental nonce (https://docs.google.com/document/d/1w2H38txbe21ex8bwxNMAcjRq4K5WHq0NCv6jYy8yo54/edit#heading=h.2ty1ce44eg7b). However, since we don't yet support payments we can simplify things. I think that the random challenge approach is the quickest way to get us the minimal capability that we want.
| // Commitments | ||
| BlobCommitments `json:"commitments"` | ||
| // AccountID is the account that is paying for the blob to be stored | ||
| AccountID AccountID `json:"account_id"` |
There was a problem hiding this comment.
Is this decided already (to be the eth address)?
There was a problem hiding this comment.
There's some uncertainty about how this will marry with the payments design, but I think it's okay to move forward with supporting a simple ECDSA-based authentication. We can swap this out pretty easily, imo.
Currently, the AccountID shoul be the ECDSA public key. I'll update the comment.
| } | ||
| } | ||
|
|
||
| func (s *signer) SignBlobRequest(header core.BlobAuthHeader) ([]byte, error) { |
There was a problem hiding this comment.
should it also sign BlobCommitments?
There was a problem hiding this comment.
Authentication will have two stages:
-
Authentication by Disperser (now)
-
Authentication by Nodes (future)
-
will be needed when we add payments. At that point, we will need to implement the full protocol, which will include signing the kzg commitment. But as we are currently only doing Authentication by the Disperser, we can do a more lightweight approach. Signing the challenge parameter just lets the disperser know that the requester has the actual private key, and not merely a signature from the private key.
If we add signing the kzg now, it implies additional work, such as actually verifying the kzg commitment. We can add this quickly, but I'm trying to minimize scope for now.
Why are these changes needed?
Add authentication classes for signing blob request header and checking signature.
Checks