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 32 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,200 @@
# 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;
- October 10th 2021: First review;

## Status

PROPOSED

## Abstract

Currently, when verifying an application link or a chain link we assume that their plain text values have been encoded using UTF-8.
However, there is a major problem with the UTF-8 encoding: it does not support all bytes properly. For this reason, we SHOULD change
the encoding of the plain text from UTF-8 to HEX in order to avoid any possible encoding problem that would result in a signature
that it's impossible to verify correctly.

## Context

The `x/profiles` module gives users the possibility to link their profile to some external account(s), either centralized applications
(eg. GitHub, Reddit, Twitter) or other blockchains (eg. Cosmos, Solana, Polkadot). When linking a Desmos profile to any of these accounts,
we use a signature-based authentication process in order to make sure that the user controls such accounts. In both the applications
and chains links, we currently expect the user to use the UTF-8 encoding when sending over the plain text used to create the signature.
However, since the UTF-8 encoding is not able to correctly represent all bytes, there might be cases in which we end up with a signature
that it's impossible to verify. For example, this is what happens if the original plain text was encoded before being signed with another
encoding such as UTF-16, Unicode, etc.

## 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

When saving a `ChainLink`, we use the `Proof` object in order to verify the signature. To make sure it supports the HEX encoding
instead of the UTF-8 one, we need to change how the `Validate` method checks for the validity of such proof:
```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
}
```
Second, we need to change how the `Proof#Verify` method verifies the signature provided in order to make sure that it deserializes
the plain text as an HEX value instead of an UTF-8 one:
``go
dadamu marked this conversation as resolved.
Show resolved Hide resolved
// Verify verifies the signature using the given plain text and public key.
// It returns and error if something is invalid.
dadamu marked this conversation as resolved.
Show resolved Hide resolved
func (p Proof) Verify(unpacker codectypes.AnyUnpacker, address AddressData) error {
var pubkey cryptotypes.PubKey
err := unpacker.UnpackAny(p.PubKey, &pubkey)
if err != nil {
return fmt.Errorf("failed to unpack the public key")
}

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

sig, err := hex.DecodeString(p.Signature)
if err != nil {
return fmt.Errorf("invalid hex-encoded signature")
}

if !pubkey.VerifySignature(value, sig) {
return fmt.Errorf("failed to verify the signature")
}

valid, err := address.VerifyPubKey(pubkey)
if err != nil {
return err
}

if !valid {
return fmt.Errorf("invalid address and public key combination provided")
}

return nil
}
``
dadamu marked this conversation as resolved.
Show resolved Hide resolved

Finally, we need to modify the `generateChainLinkJSON` function to return a HEX encoded plain text:
```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) {

...

sig, pubkey, err := keyBase.Sign(keyName, []byte(value))
if err != nil {
return profilescliutils.ChainLinkJSON{}, err
}

return profilescliutils.NewChainLinkJSON(
profilestypes.NewBech32Address(addr, chain.Prefix),
profilestypes.NewProof(pubkey, hex.EncodeToString(sig), hex.EncodeToString([]byte(value))),
profilestypes.NewChainConfig(chain.Name),
), nil
}
```

### Chain link implementation
dadamu marked this conversation as resolved.
Show resolved Hide resolved

While dealing with application links, we use the `Result_Success_` type to identify a successfully verified link. In order to force
the plain text to be HEX encoded, we need to modify the `Validate` function to perform such check:

```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, we also need to change the function that is currently used by users to generate the signature using the Desmos CLI to
make sure it returns the plain text using the HEX encoding:
```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]
bz, pubKey, err := txFactory.Keybase().Sign(key.GetName(), []byte(value))
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: hex.EncodeToString([]byte(value)),
}

// Serialize the output as JSON and print it
bz, err = json.Marshal(&signatureData)
if err != nil {
return err
}
dadamu marked this conversation as resolved.
Show resolved Hide resolved

return clientCtx.PrintBytes(bz)
},
}

flags.AddTxFlagsToCmd(cmd)

return cmd
}
```
## Consequences

### Backwards Compatibility

With this approach there should not be any problem with old chain and application links since since the signature was
dadamu marked this conversation as resolved.
Show resolved Hide resolved
verified during the creation process and this ADR only targets the new links that will be created. However, in order to
make sure that clients can verify all the links at the same way, we SHOULD keep the on-chain data consistent using a migration script
that transforms all currently stored plain texts from being UTF-8 encoded strings into HEX encoded strings.
As a result, this feature is backwards compatible.

### Positive

* Ensure the signing and verification process for chain and application links work properly with arbitrary bytes

### Negative

(none known)

### Neutral

(none known)

## Further Discussions

## Test Cases [optional]

## References

- Issue [#636](https://github.com/desmos-labs/desmos/issues/636)
- [The basics of UTF-8](https://www.codeguru.com/cplusplus/the-basics-of-utf-8/)