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

Retrieve long term static key for Conn #4

Merged
merged 5 commits into from
Feb 4, 2018
Merged
Changes from 1 commit
Commits
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
21 changes: 20 additions & 1 deletion noise/conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ type Conn struct {
// handshake
config *Config // configuration passed to constructor
hs handshakeState
remotePub []byte
Copy link
Owner

Choose a reason for hiding this comment

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

why not making it a [32]byte to avoid having to make during the handshake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Becase I wanted to see if you can see it ? ahah. well spotted.

handshakeComplete bool
handshakeMutex sync.Mutex

Expand Down Expand Up @@ -239,7 +240,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 +320,9 @@ ContinueHandshake:
if !c.config.PublicKeyVerifier(hs.rs.PublicKey[:], receivedPayload) {
return errors.New("Noise: the received public key could not be authenticated")
}
c.isRemoteAuthenticated = true
Copy link
Owner

Choose a reason for hiding this comment

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

good point, I was toying with this idea of having a isRemoteAuthenticated value you can check.

I'm not sure it is useful as the handshake you're using should make it pretty clear that you're authenticating the remote, but I thought it could be "useful" as a defense-in-depth mechanism. What do you think?

It should also probably be exported if it's meant to be used by developers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put it there because I used the IsAuthenticated method in StaticKey. If it's not really needed (see comment below) then we can totally drop that.
In my opinion, the semantic of IsAuthenticated is a bit vague or redundant: do you mean authenticated with a longterm static key ? Because users of that library must intentionally declare the pattern they want to use and if the handshake succeeds and a static key must have been authenticated in some way, then it is by default Authenticated no ?

Copy link
Owner

@mimoo mimoo Jan 24, 2018

Choose a reason for hiding this comment

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

authenticated with a longterm static key?

not necessarily, I am trying to get back into the state of mind I had when I wrote this. But I think I wanted to create an out-of-band authentication. For example something like Noise_NN where you would extract a fingerprint and authenticate the connection post-handshake. Noise_NNoob maybe? Although I'm not sure it deserves a name. By having this isRemoteAuthenticated you can check if peers have actually verified the fingerprint. You can think of some application that would want to show some features only if this var been set.

c.remotePub = make([]byte, 32)
copy(c.remotePub, hs.rs.PublicKey[:])
Copy link
Owner

Choose a reason for hiding this comment

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

shouldn't you do this when receiving the s token?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment above. I just want to get the static key when it has been verified.
BTW, It is possible also to retrieve the value directly from the PublicKeyVerifier but having a StaticKey method is more practical code-wise.

}
}

Expand Down Expand Up @@ -351,6 +355,21 @@ 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.IsRemoteAuthenticated() {
Copy link
Owner

Choose a reason for hiding this comment

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

Are there usecases where you would still want to obtain the public key, even if the peer has not been authenticated yet? Maybe if you're planning to authenticate post-handshake (using a short-authentication string or something like that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I was not sure about that, so I just thought going to the safest path. But Is it really possible to get the static key without verifying ? This library panics if there's no PublicKeyVerifier in the Config IIRC .

Copy link
Owner

Choose a reason for hiding this comment

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

true.

Another scenario I can think of: setting up the PublicKeyVerifier() based on the static public key you received. But this is currently not possible with this library.

return nil, errors.New("noise: remote peer not authenticated")
}
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.remotePub == nil {
return nil, errors.New("noise: no remote static key given")
}
return c.remotePub, nil
Copy link
Owner

Choose a reason for hiding this comment

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

why not returning hs.rs.PublicKey instead? This way you wouldn't have to create a c.remotePub and copy the public key into it.

Copy link
Contributor Author

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.

}

//
// input/output functions
//
Expand Down