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.

Sign up for GitHub

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

Jump to bottom

Retrieve long term static key for Conn #4

Merged
merged 5 commits into from
Feb 4, 2018
Merged

Retrieve long term static key for Conn #4

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
73b524a
added StaticKey()
nikkolasg Jan 22, 2018
4c05fea
fixed some commits
nikkolasg Jan 24, 2018
2d4c4b1
fix build errors
nikkolasg Jan 24, 2018
ecc3f3c
added statickey test
nikkolasg Jan 24, 2018
e421856
one proposal
nikkolasg Jan 26, 2018
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
18 changes: 17 additions & 1 deletion noise/conn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ type Conn struct {
// handshake
config *Config // configuration passed to constructor
hs handshakeState
remotePub [32]byte // remote static public key
Copy link
Owner

Choose a reason for hiding this comment

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

Because the hs.rs is cleared when the handshake completes (c.hs.clear()) and the clearing clears up also the public key. I think it could be relaxed to only clear up the private key but better be safe than sorry.

So. Can we think of good reasons for clear() to clear the public keys? If not, I'd rather have us modify the behavior of clear() rather than adding new fields here :o

Copy link
Contributor Author

@nikkolasg nikkolasg Jan 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The reasons to clear a key from memory is to avoid an attacker being able to read it (either because he's on the same machine or by using heartbleed style attacks, etc). Usually we clear a private key because that's what is most urgent to keep secret.
In a far far fetched scenario, we might also want to clear up the static public key to be able to deny later that we were participating in the communication ? (if we load the key via Yubikey on a computer which got compromised just after we used it so it still has the memory kept ? )
Lol this seems way too far fetched from me. So if you also can't find a realistic scenario, then let me know and I'll modify clear() so it only clears the private key.

Copy link
Owner

Choose a reason for hiding this comment

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

yeah I'm not particularly interested in the deniability property which Noise doesn't provide in any case. I think a better idea would be to stop clear() from removing the public keys. If you can make this change I'll accept your PR! Thanks again for the initial PR and the changes :) you rock

remotePubSet bool // true when remote public key is set in remotePub
handshakeComplete bool
handshakeMutex sync.Mutex

Expand Down Expand Up @@ -239,7 +241,7 @@ func (c *Conn) Handshake() error {
var remoteKeyPair *KeyPair
if c.config.RemoteKey != nil {
if len(c.config.RemoteKey) != 32 {
return errors.New("Noise: the provided remote key is not 32-byte.")
return errors.New("noise: the provided remote key is not 32-byte")
}
remoteKeyPair = &KeyPair{}
copy(remoteKeyPair.PublicKey[:], c.config.RemoteKey)
Expand Down Expand Up @@ -319,6 +321,8 @@ ContinueHandshake:
if !c.config.PublicKeyVerifier(hs.rs.PublicKey[:], receivedPayload) {
return errors.New("Noise: the received public key could not be authenticated")
}
copy(c.remotePub[:], hs.rs.PublicKey[:])
c.remotePubSet = true
}
}

Expand Down Expand Up @@ -351,6 +355,18 @@ func (c *Conn) IsRemoteAuthenticated() bool {
return c.isRemoteAuthenticated
}

// StaticKey returns the static key of the remote peer. It is useful in case the
// static key is only transmitted during the handshake.
func (c *Conn) StaticKey() ([]byte, error) {
if !c.handshakeComplete {
Copy link
Owner

Choose a reason for hiding this comment

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

is it important that the handshake has to be finished?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mhhhh I wanna make sure that the key I get when calling StaticKey is the right one (i.e. verified by PublicKeyVerifier). Maybe it is possible to deliver it before the handshake completely finishes, but in my use case, I'm fine with waiting the handshake to be finished so I'm sure.
In any case, looking at the code where the PublicKeyVerifier gets called, it's almost at the end of the Handshake method so not even sure what we can do "earlier".

return nil, errors.New("noise: handshake not completed")
}
if !c.remotePubSet {
return nil, errors.New("noise: no remote static key given")
}
return c.remotePub[:], nil
}

//
// input/output functions
//
Expand Down
17 changes: 16 additions & 1 deletion noise/patterns_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,13 @@ func TestNoiseXX(t *testing.T) {
if _, err = serverSocket.Write([]byte("ca va?")); err != nil {
t.Fatal("server can't write on socket")
}

clientStatic, err := serverSocket.(*Conn).StaticKey()
if err != nil {
t.Fatal("client static key not retrieved")
}
if !bytes.Equal(clientStatic, clientKeyPair.PublicKey[:]) {
t.Fatal("client static retrieved not correct")
}
}()

// Run the client
Expand All @@ -211,6 +217,15 @@ func TestNoiseXX(t *testing.T) {
if !bytes.Equal(buf[:n], []byte("ca va?")) {
t.Fatal("server message failed")
}

serverStatic, err := clientSocket.StaticKey()
if err != nil {
t.Fatal("server static key not retrieved after exchange", err)
}

if !bytes.Equal(serverStatic, serverKeyPair.PublicKey[:]) {
t.Fatal("static key received different than server's one")
}
}

func TestNoiseN(t *testing.T) {
Expand Down