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

provider record encryption #3

Open
wants to merge 32 commits into
base: noot/prefix-lookup
Choose a base branch
from

Conversation

noot
Copy link

@noot noot commented Sep 21, 2022

  • encrypt peer IDs when providing content
  • decrypt peer IDs if encrypted when receiving content providers
  • when sending ADD_PROVIDER message, include signature of message key || encrypted peer ID
  • on receipt of ADD_PROVIDER message, verify this signature
  • update ProviderManager to only deal with peer IDs, simplifies the code somewhat as it was only accepting peer IDs anyways

@noot noot changed the title [wip] provider record encryption provider record encryption Oct 3, 2022
@noot noot marked this pull request as ready for review October 3, 2022 14:57
pb/dht.pb.go Outdated
// signature of the provided key + encrypted peer ID for ADD_PROVIDER messages
Signature []byte `protobuf:"bytes,5,opt,name=signature,proto3" json:"signature,omitempty"`
// public key of the peer for ADD_PROVIDER messages
PublicKey *pb1.PublicKey `protobuf:"bytes,6,opt,name=publicKey,proto3" json:"publicKey,omitempty"`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the public key required in the message's content?

If we only want peers to publish content that they provide themselves, then this is not required. The public key can be derived from the peerID. However, it is useful in the case we want to allow peers to publish content that they do not provide.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, is it possible to derive the public key from the peer ID? I thought the peer ID was a hash of the public key, (https://github.com/libp2p/go-libp2p/blob/1ad0a50be62b1e00388b9d85fc997d66e8910c9d/core/peer/peer.go#L179) so I had to send the public key in the message to verify the signature.

"github.com/multiformats/go-multihash"
)

const (

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to get these constants from somewhere where they are already defined instead of defining them again? This would help with code maintainability :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately the Go AES package doesn't define these :/ I'll try looking somewhere for them!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to get Nonce size from cipher.AEAD interface.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

}

ct := aesgcm.Seal(nil, nonce, plaintext, nil)
return append(nonce, ct...), nil

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need to append encryption algorithm here too so that the encryption function rotation doesn't require coordinated client update. I.e. for example AESGCM || nonce || enc(payload). That would also allow to have different encryption functions in the same DHT.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, will add!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to add 6 bytes to every encrypted value. Can we use a varint as code for AESGSM, then update the spec to document it?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@masih do you have a pointer to the AESGCM varint?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we will have to define this mask ourselves and then document in the spec what that number means

Copy link

@masih masih Jan 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. We'd need to define those in the spec ourselves.

This can also be a new multicodec. I recommend picking a varint that's not reserved already to have the option of adding it to the multicodec table later on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants