Skip to content
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

docs(adr): ADR-008: Change the plain text encoding of links to hex #642

Merged
merged 42 commits into from
Oct 14, 2021
Merged
Changes from 9 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
debf7fd
Init adr from template
dadamu Oct 5, 2021
0b602db
Rename the topic
dadamu Oct 6, 2021
df02225
Complete writing
dadamu Oct 7, 2021
c351d82
Finish ADR 008
dadamu Oct 7, 2021
f9fddac
Fix comments
dadamu Oct 7, 2021
7c13e4c
Fix positive consequences
dadamu Oct 7, 2021
9f22945
Fix changelog
dadamu Oct 7, 2021
c0cde24
Fix phrase
dadamu Oct 7, 2021
d7341de
Fix phrase
dadamu Oct 7, 2021
f0bd99f
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 8, 2021
3914d84
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 8, 2021
9b8ae32
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 8, 2021
d4153aa
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 8, 2021
d40339f
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 8, 2021
061ebf6
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 8, 2021
0fb6ded
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 8, 2021
984dabf
Fix typo
dadamu Oct 8, 2021
a3ea657
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 12, 2021
65ba85e
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 12, 2021
ad8f959
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 12, 2021
9b73911
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 12, 2021
f0936e9
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 12, 2021
db9d5d3
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 12, 2021
1af668c
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 12, 2021
433c6eb
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 12, 2021
537e98f
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 12, 2021
b43f385
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 12, 2021
c118be6
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 12, 2021
e3483e4
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 12, 2021
3fdeea1
Apply suggestion for context
dadamu Oct 12, 2021
66664a2
Update changelog
dadamu Oct 12, 2021
3e411e1
Fix indent
dadamu Oct 12, 2021
737f221
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 13, 2021
ae080cb
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 13, 2021
368c310
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 13, 2021
6fd72bc
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 13, 2021
b42b7b1
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 13, 2021
6b3f945
Apply suggestion
dadamu Oct 13, 2021
281cf58
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 13, 2021
5753310
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 13, 2021
d5920b5
Update docs/architecture/adr-008-change-the-plain-text-encoding-the-l…
dadamu Oct 13, 2021
dd3d7be
Merge branch 'master' into paul/adr-008-change-the-link-signature-enc…
mergify[bot] Oct 14, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
# ADR 008: Change the plain text encoding of links to hex

## Changelog

- October 5th 2021: Initial draft;
- October 7th 2021: Moved from DRAFT to PROPOSED;

## Status

PROPOSED

## Abstract

Currently, when verifying an application link or a chain link, we assume that the plain text value of them has been encoded using UTF-8.
Unfortunately, Ledger or common wallets does not support to sign something due to the fact that they do not allow to sign arbitrary strings.
We SHOULD change encoding of the plain text to hex in order to avoid this problem.
dadamu marked this conversation as resolved.
Show resolved Hide resolved

## Context

Desmos `profiles` module give the possibility to link the desmos profile to external account. There are two objects to prove
the connection between them, which are application link for centralized network and chain link for blockchain network.
Both application link and chain link contains a signature signed with the plain text by a private key and a public key
from the private key generating the signature. In addition, the plain text used by the signature is assumed as UTF-8 encoded
dadamu marked this conversation as resolved.
Show resolved Hide resolved
but it occurs the problem if the encoding of plain text is others like UTF-16, Unicode and etc.
dadamu marked this conversation as resolved.
Show resolved Hide resolved

## Decision

We propose to change the encoding of the plain text of both application link and chain link to hex.

### The implementation of chain link
dadamu marked this conversation as resolved.
Show resolved Hide resolved

In chain link, `Proof` is a object to contains the data related verification. We will check the plain text if it is hex-encoded.
dadamu marked this conversation as resolved.
Show resolved Hide resolved
The `Validate` function will be like:
```go
// Validate checks the validity of the Proof
func (p Proof) Validate() error {

...

_, err = hex.DecodeString(p.PlainText)
if err != nil {
return fmt.Errorf("invalid hex-encoded plain text")
}

return nil
}
```
In addition, the function to generate chain link json in CLI tool is `generateChainLinkJSON`.
We will modify it to use hex-encoded plain text, it will be like:
dadamu marked this conversation as resolved.
Show resolved Hide resolved
```go
// generateChainLinkJSON returns build a new ChainLinkJSON intance using the provided mnemonic and chain configuration
dadamu marked this conversation as resolved.
Show resolved Hide resolved
func generateChainLinkJSON(mnemonic string, chain chainlinktypes.Chain) (profilescliutils.ChainLinkJSON, error) {

...

plainText := hex.EncodeToString([]byte(value)) // Make plain text be hex-encoded
sig, pubkey, err := keyBase.Sign(keyName, []byte(plainText))
if err != nil {
return profilescliutils.ChainLinkJSON{}, err
}

return profilescliutils.NewChainLinkJSON(
profilestypes.NewBech32Address(addr, chain.Prefix),
profilestypes.NewProof(pubkey, hex.EncodeToString(sig), plainText),
profilestypes.NewChainConfig(chain.Name),
), nil
dadamu marked this conversation as resolved.
Show resolved Hide resolved
}
```

### The implementation of application link
dadamu marked this conversation as resolved.
Show resolved Hide resolved

In application link, the object to show the proof is `Result_Success_` which is a sub object inside `Result`.
We will modify the `Validate` function to ensure the plain text is hex-encoded.
dadamu marked this conversation as resolved.
Show resolved Hide resolved

```go
// Validate returns an error if the instance does not contain valid data
func (r Result_Success_) Validate() error {

...

if _, err := hex.DecodeString(r.Value); err != nil {
return fmt.Errorf("invalid hex-encoded plain text")
}

return nil
}
```

Besides, the function to generate signature in the CLI tool is `GetSignCmd`.
We will use hex-encoded plain text inside it, it will be like:
dadamu marked this conversation as resolved.
Show resolved Hide resolved
```go
// GetSignCmd returns the command allowing to sign an arbitrary for later verification
dadamu marked this conversation as resolved.
Show resolved Hide resolved
leobragaz marked this conversation as resolved.
Show resolved Hide resolved
func GetSignCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "sign [value]",
Short: "Allows to sign the given value using the private key associated to the address or key specified using the --from flag",
RunE: func(cmd *cobra.Command, args []string) error {

...

value := args[0]
plainText := hex.EncodeToString([]byte(value)) // Make plain text be hex-encoded
bz, pubKey, err := txFactory.Keybase().Sign(key.GetName(), []byte(plainText))
if err != nil {
return err
}

// Build the signature data output
signatureData := SignatureData{
Address: strings.ToLower(pubKey.Address().String()),
Signature: strings.ToLower(hex.EncodeToString(bz)),
PubKey: strings.ToLower(hex.EncodeToString(pubKey.Bytes())),
Value: plainText,
}
dadamu marked this conversation as resolved.
Show resolved Hide resolved

// Serialize the output as JSON and print it
bz, err = json.Marshal(&signatureData)
if err != nil {
return err
}

return clientCtx.PrintBytes(bz)
},
}

flags.AddTxFlagsToCmd(cmd)

return cmd
}
```
## Consequences

### Backwards Compatibility

Currently, all the plain text in the old links are UTF-8 encoded, there is no problem to them since the signature was
dadamu marked this conversation as resolved.
Show resolved Hide resolved
verified during the creation process and this ADR only targets to the new links. It can be kept consistent on-chain by the migration script
to transform all currently stored links into hex-encoded.
As a result, it is backwards compatibility.
dadamu marked this conversation as resolved.
Show resolved Hide resolved

### Positive

* Ensure signing and verification process works properly with arbitrary strings
dadamu marked this conversation as resolved.
Show resolved Hide resolved

### Negative

(none known)

### Neutral

(none known)

## Further Discussions

## Test Cases [optional]

## References

- Issue [#636](https://github.com/desmos-labs/desmos/issues/636)
dadamu marked this conversation as resolved.
Show resolved Hide resolved