External signer API (first iteration) #18079
Conversation
holiman
changed the title
signer/core/api.go
Outdated
There was a problem hiding this comment.
I think this s slipped in by accident
holiman
force-pushed
the
externalsigner
branch
from
797c614 to
813bf50
Compare
|
Formatted, rebased |
holiman
force-pushed
the
externalsigner
branch
from
813bf50 to
94d2185
Compare
|
Formatted, conflicts fixed, rebased |
|
As promised, here's my critique of I think this method is too specific. Code in accounts/ shouldn't know about clique and other use cases that require custom display/rule logic will come up later. I propose you add the following method instead of // SignData signs hashfunc(data). The only supported hash function
// is "keccak256". The mimetype paremeter
// describes the type of data being signed.
SignData(mimetype, hashfunc string, data []byte) (sig []byte, err error)This method can be supported by all account management backends including clef. If a backend wants to present the signed data to the user in a custom way, it can do so by looking at the mime type. For other backends such as the keystore, the mime type serves no particular purpose and is discarded. Using this method, the clique code would sign headers by calling When a clique header is being signed through clef, the frontend would parse the RLP header and display it accordingly. The same processing may happen in rule scripts. The rule script engine would need to be extended to know about the mime type of the data being signed so it can decode it. |
|
Ok I have now addressed your concern @fjl , except I didn't see any point in having a |
|
formatted. ptal @fjl |
|
Since I've been messing about with the clique signing, let's not merge this until after the monday release. When we merge it, I want to try it out on the rinkeby signer before it makes it into a release. |
holiman
force-pushed
the
externalsigner
branch
from
b751928 to
f242e52
Compare
|
Rebased, ptal @fjl |
holiman
force-pushed
the
externalsigner
branch
from
c602dd7 to
cd47db8
Compare
fjl
left a comment
fjl
left a comment
There was a problem hiding this comment.
The integration looks very clean now. I think the hash function parameter is required though. Clef should not allow feeding arbitrary data into the signature calculation because it might enable attacks on signature math, leading to key extraction. It is much safer to perform hashing on the clef side.
accounts/accounts.go
Outdated
| // Note, the method requires the extra data to be at least 65 bytes, otherwise it | ||
| // panics. This is done to avoid accidentally using both forms (signature present | ||
| // or not), which could be abused to produce different hashes for the same header. | ||
| func CliqueHash(header *types.Header) (hash common.Hash) { |
There was a problem hiding this comment.
Do we still need this function?
There was a problem hiding this comment.
I'm honestly not sure right now. I'll take a look if it can be removed
I'm not sure what you are referring to. Clef doesn't allow arbitrary data being signed anywhere...? |
3 participants
This PR is still a bit of a WIP, but I think it can be a good first step. It implements an additional backend for the account manager. The backend integrates with
clef, and invokes an external signer for operations.It currently can sign only transactions, for more advanced usages, such as signing Clique headers and/or data, we'll wait for the PR from @PaulRBerg to get implemented into
clef.Also some other things are missing, and could be added later on:
localandexternalaccounts, without too much confusionipcintegration, not only httpI'm not sure if the current scheme for
Accounts.URLis the best, not quite sure how it is intended (e..g, should we have wallet URL be the same as the Backend URL? -- and should we really expose the plain backend URL or use some indirection, such as a hash or the remote url? )