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
ARC-60 Algorand Wallet Structured Data Signing API #284
base: main
Are you sure you want to change the base?
Conversation
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.
Made a few comments.
I think we need a schema for 32 bytes signing auth challenge requests
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.
We could add a matching wallet.spec.ts file with a small test suite so that we can test the functions of the reference implementation
assets/arc-0060/lsig-schema.json
Outdated
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.
@joe-p can you validate this schema?
@ehanoc Thank you for your comments. The JSON schema for authentication challenge will be provided by ARC-31 as an extension of ARC-60. |
ARCs/arc-0060.md
Outdated
|
||
| ScopeType | Description | | ||
| --- | --- | | ||
| PLAIN | Signature of a simple byte string. This is the most generic scope. | |
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.
PLAIN is the same as ARBITRARY, doesn't specify WHY we are signing
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.
Agreed. I would like to keep a generic Scope for a generic message signing. What about SIGMSG
?
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.
But generic is problematic because we don't know what we are signing. SIGMSG
still doesn't state what we are signing.
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 removed generic scopes. We only have AUTH
and LSIG
now.
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.
Thanks @deanstef for putting together ARC-60!
ARCs/arc-0060.md
Outdated
metadata: StdSignMetadata, | ||
signer: Ed25519Pk, | ||
} | ||
=> Promise<(SignedDataStr | null)>; |
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.
In which cases can signData
return null? Further below, it is defined that if signData
fails, it should throw an error, not return null. So shouldn't the function either return SigendDataStr
or throw an 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.
Yes you are right. This is a leftover from a previous design with bulk signatures.
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.
Actually - in the reference implemnetation, I am returning null
if the user does not confimr the signature: https://github.com/deanstef/ARCs/blob/a8de5d08f3c558bedeb8c1addbacc25362eda7b5/assets/arc-0060/wallet.ts#L79C4-L79C5
ARCs/arc-0060.md
Outdated
`SignedDataStr` is the produced 64-byte array, ed25519 digital signature, of the signed data. | ||
|
||
```tsx | ||
export type SignedDataStr = Uint8Array; |
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.
The naming is a little confusing to me. Why does SignedDataStr
include the term Str
when it is defined as an Uint8Array
. Conversely, StdData
is also defined as a string but does not contain the term Str
.
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.
Yeah the naming got a bit messy when we changed stuff from String
to Uint8Array
. I will align the naming such that only string objects names will end with the Str
term
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.
ARCs/arc-0060.md
Outdated
|
||
#### Interface `SignedDataStr` | ||
|
||
`SignedDataStr` is the produced 64-byte array, ed25519 digital signature, of the signed data. |
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.
According to this definition, SigendDataStr
is purely the signature (64 bytes long). In the reference implementation, nacl.sign
is used which returns the signature and the original message.
If SignedDataStr
represents purely the signature, wouldn't something like Signature
be a more appropriate name for this type?
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.
Good catch. Here b244ed6 I updated both the naming and the reference implementation. I am using nacl.sign.detached
now.
/** | ||
* Canonical representation of the JSON schema for the signing data. | ||
*/ | ||
schema: string; |
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 is the schema (which is a JSON object) represented as a canonicalized JSON string?
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.
In a very first version of ARC60 I was using canonicalized versions of JSON objects to produce signatures of the entire objects. In the new version this is not needed anymore. We can get rid of the canonicalized stuff indeed.
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.
/** | ||
* Optional encoding used to represent the signing data. | ||
*/ | ||
encoding?: string; |
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 are possible encodings that should be supported?
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.
+1 to clarifying this
ARCs/arc-0060.md
Outdated
export interface SignDataError extends Error { | ||
code: number; | ||
data?: any; | ||
failingSignData: (StdData | null); | ||
} |
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.
In which case could failingSignData
be null
?
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.
If the error was caused by a missing StdData
object.
ARCs/arc-0060.md
Outdated
|
||
### Semantic Requirements | ||
|
||
The call `signData(data, metadata, signer)` **MUST** either return the signed data `ret` or reject the call throwing an error `err`. |
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.
According to the definition of SignDataFunction
above, it seems a null
return value is also possible (which I don't think is required as I mentioned earlier).
ARCs/arc-0060.md
Outdated
Upon calling `signData(data, metadata, signer)`: | ||
|
||
- the `data`, `metadata`, and `signer` **MUST NOT** be `null`, otherwise the wallet **MUST** reject the call. | ||
- `data` **MUST** be validated with respect to the JSON schema `metadata.schema`. If the validation fails, the call **MUST** be rejected with a `4300` 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.
According to the table above an invalid schema is a 4601 error code, or is this a different schema?
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.
Error code 4601 means that the JSON schema does not comply with ARC-60 (the required fields ARC60Domain
and bytes
are missing).
In this case, we are failing because data
is not a valid object according to the schema metadata.schema
.
ARCs/arc-0060.md
Outdated
- the `bytes` must be a valid byte string: | ||
- the wallet **MUST** reject if `bytes` is prepended with a forbidden domain separator `TX` or `TG`. |
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.
Is this validation here independent of the ARC60Domain
validation? Because if ARC60Domain
is set to arc60
(or anything other than ""
, "TX"
, or "TG"
), wouldn't it be fine if bytes
started with TX
or TG
? Because the wallet would have to concatenate the value of ARC60Domain
and bytes
and hence no malicious payload could be signed?
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.
Setting ARC60Domain
to ""
would make the concatenation not effective. I suggest to always validate bytes
against malicious payloads (prepended with TX
or TG
) to avoid problems.
ARCs/arc-0060.md
Outdated
|
||
| ScopeType | Description | | ||
| --- | --- | | ||
| MSGSIG | Signature of a simple message. This is the most generic scope. | |
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 don't think we should have generic scopes. They should be VERY Specific.
A generic scope doesn't tell you what schema to enforce and could potentially be used to trick the user or even the wallet.
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.
The spec doesn't make it generic. It says that the bytes must not be certain things, such as transactions. The spec says "most generic", i.e., that narrower scopes might be defined. I think the problem then becomes how to specify what things should not be. For example, specifying a scope that allows 32 arbitrary bytes to be signed for auth purposes does not preclude a 32 byte arbitrary document from being signed.
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.
We only have AUTH
and LSIG
scopes now.
ARCs/arc-0060.md
Outdated
|
||
> This section is non-normative | ||
|
||
Signatures of bytes are processed with the `signData(data, metadata, signer)` function. |
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.
@deanstef requiring the public key (a.k.a signer) would be somewhat cumbersome for HDWallets to find the correct address to use. Would have to do a recursive search.
Legacy algorand wallets are single address, with HDWallets they will support BIP44 and providing the path as optional would suit best.
Thoughts?
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 signData(data, metadata, keyInfo
Where keyInfo
can contain information of where to find the key, what keyType, hashing function to use would be extendable and offer future proof with HDWallets
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.
In the first version of ARC-60 there was the possibility of passing a HDWallet derivation path as signer parameter (see https://github.com/deanstef/ARCs/blob/67754a89ff9e211cfee6f29ebd7aa4f016b19243/ARCs/arc-0060.md?plain=1#L122)
Then we decided to keep it simple and just opt for passing the public key of the signer. I'd be happy to re-introduce HDWallets support though
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 understand. Although just the public key is ambiguous when you have N possible key pairs in a BIP32 / BIP44 path scheme.
Hi, I cannot figure out if, on the latest revision, data to sign is |
/** | ||
* Optional encoding used to represent the signing data. | ||
*/ | ||
encoding?: string; |
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.
+1 to clarifying this
The `StdDataStr` object is a string that comply with the `metadata` JSON schema. | ||
|
||
```tsx | ||
export type StdDataStr = string; |
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 don't understand how this can contain an logicsig program if it's a string
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.
Yes, I prefer a Buffer or Uint8Array.
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.
The way I read the spec is that the StdDataStr is a stringified JSON object that must comply with the metadata supplied as an argument to the sign function, of type StdSignMetadata. The metadata specifies which field contains data to be signed, and not the whole json object. Part of the problem is this line in the first paragraphs:
data is a string representing the structured data being signed.
This is not true according to the rest of the spec. The data being signed is whichever field of the StdDataStr json object is specified to be signed.
Another problem is that there is not defined here the standard encoding for JSON bytes data. Typically in web applications it is Base64 encoded strings, and OpenAPI spec clients/servers will work with that, but it's only by common convention, and I don't know the base specs like JSON Schema say anything about it (off the top of my head)
In general I find the spec confusing because of a) the use of both TypeScript types and JSON types, and I don't like the use of things like UInt8Array at all, as they are TS domain specific concepts and don't belong in language agnostic specs. b) slightly misleading terminology, like "Scope" instead of "Schema"...
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.
Thank you @FrankSzendzielarz
This commit c08b995 should fix the issue you raised.
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.
Also, @jasonpaulos @mxmauro please refer to the Backwards Compatibility section of the ARC.
The data to sign is an encoded field of the input JSON text. It will always be a string, of course, as the input is JSON. What encoding depends on the metadata (and I am informed that the spec will be updated to clarify this). What does seem to be clear though, given the comments like this one, is that either the spec is not being read, or not being understood, so I'd suggest specs like these get plain English TLDRs at the top. |
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 made another pass through the ARC, please see my comments below. The ARC is making good progress!
```json | ||
{ | ||
"ARC60Domain" : "arc60", | ||
"bytes" : "[99, 50, 97, 57, 51, 99, ..., 57, 49, 53]" |
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 the bytes here really be a string or rather a proper array like so:
"bytes" : [99, 50, 97, 57, 51, 99, ..., 57, 49, 53]
`signingData` is a `StdSigData` object composed of the signing `data` that instantiates a known JSON Schema and the `signer`'s public key. | ||
|
||
`metadata` is a `StdSignMetadata` object that describes the signature `scope`, the JSON `schema`, and the `encoding` used to represent the signing bytes. The JSON schema takes two elements: the `bytes` being signed and an optional `prefix`. |
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.
Is it intentional that the letter "n" is missing in StdSigData
/ present in StdSignMetadata
? Should it be StdSignData
and StdSignMetadata
?
{ | ||
"data": "/{.../}", | ||
"signer": "xxxx" | ||
} | ||
{ | ||
"scope": "...", | ||
"schema": "/{.../}", | ||
"encoding": "..." | ||
} |
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.
Can we give those objects a name? For example:
signingData = {
"data": "{...}",
"signer": "xxxx"
};
metadata = {
"scope": "...",
"schema": "{...}",
"encoding": "..."
};
Also, should it be "data": "{...}"
rather than "data": "/{.../}"
? (same for schema
)
```json | ||
{ | ||
"data": "/{.../}", | ||
"signer": "xxxx", | ||
"hdPath": { | ||
"purpose": 44, | ||
"coinType": 0, | ||
"account": 0, | ||
"change": 0, | ||
"addrIdx": 0 | ||
} | ||
} | ||
{ | ||
"scope": "...", | ||
"schema": "/{.../}", | ||
"encoding": "..." | ||
} | ||
``` |
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.
same as above:
signingData = {
"data": "{...}",
"signer": "xxxx",
"hdPath": {
"purpose": 44,
"coinType": 0,
"account": 0,
"change": 0,
"addrIdx": 0
}
};
metadata = {
"scope": "...",
"schema": "{...}",
"encoding": "..."
};
- `schema` JSON schema to be used according to the `scope`. ARC-60 compliant JSON schemas must have two mandatory fields: | ||
- `ARC60Domain` domain separator for the signing bytes. | ||
- `bytes` signing bytes. | ||
- `encoding` specifies the encoding used for the `schema.bytes` field. If not specified, encoding should be `base64`. |
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 have two questions here:
- Does this encoding apply to
(metadata.)schema.bytes
or(signingData.)data.bytes
? Could it be that when you writeschema.bytes
you implicitly meandata.bytes
? - Since
encoding
is an optional parameter, this means if no encoding is provided it should be consideredbase64
? In yourwallet.ts
example no encoding is specified, but the bytes array is represented as a JSON array of integers, not a base64-string
- the wallet **MUST** reject if `ARC60Domain="TX"`. | ||
- the wallet **MUST** reject if `ARC60Domain="TG"`. |
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.
Same as above: are there any other forbidden tags like "Program"?
- `data.bytes` must be a valid byte string: | ||
- it **MUST** be encoded as `metadata.encoding` if present, otherwise `base64`. | ||
- the wallet **MUST** throw a `4602` error if the decoding fails with the given encoding value. | ||
- the wallet **MUST** reject if `data.bytes` field is prepended with a forbidden domain separator `TX` or `TG`. |
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.
Just marking this for completeness -- same as above
|
||
This API was not designed to sign Algorand transactions or a group of transactions (see ARC-1 instead). | ||
|
||
## Backwards Compatibility |
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'm wondering if we should even mention ARC-47 in this ARC here. I'm inclined to remove it for the sake of simplicity and to keep this ARC as short and concise as possible. If there's demand for signing lsigs with ARC-60, somebody can propose a new ARC, specify the scope, and define the schema, etc.
|
||
Users must be aware of what they are signing and for what purpose. The wallet **MUST** always compute a new message dynamically according to the received `signingData` and `metadata`. This prevents the injection of malicious or misleading messages and provides a clear scope of the signing action. | ||
|
||
Wallets only support known `ScopeTypes` and schemas. It ensures that signing actions have a clear scope and mitigates the risk of producing unexpected signatures. |
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 agree that wallets need to hard-code the ScopeTypes
and schemas that they support. But this raises the question for me if we even need to include the schema
in the metadata
field?
Given the ScopeType
, the wallet could look up its hard-coded schema and match the signingData
against this schema.
Perhaps it's nice to have the schema also in the metadata to make the request self-descriptive. But in an implementation I'd likely ignore metadata.schema
and match signingData.bytes
against my hard-coded schema. Would that be a problem?
- the wallet **MUST** follow the ARC-47 specification: compile the program from the received template and check its integrity with the `hash`. | ||
- the compiled program **MUST** be used with the prefix to compute the signing message `msg`, as `msg="Program" || <compiled_lsig>`. | ||
|
||
## Test Cases |
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.
Once the ARC is stable and about to be finalized, it'd be nice to add some test cases. We could specify a keypair, a message (according to the AUTH schema) and show the resulting signature as a test case.
In another test case we could have a keypair derived from a hd-wallet.
An API for signing structured bytes with Algorand wallets. It unlocks many use cases ranging from self-custodial digital identities, authentication, document signing etc.
The API provides a standard interface for a signData() function that takes as an argument a structured data object and metadata and returns the requested digital signature.
It is compatible with rekeyed accounts, multi-signature and HD wallets.This API is inspired and based on ARC-1. It has been conceived to be secure by design with semantics and security controls. It does not allow the signing of canonical Algorand transactions or Groups.