IPFS secio handshake patch likely not forward compatible

[cpacia]

In the code segment below when establishing a new outgoing connection s.remotePeer is set to the peerID of the node we're trying to connect to. Ultimately this will either be an old style (hashed) peerID or a new style inline key depending on the actual ID that is used when we try to make a connection.

So far so good. Also peer.IDFromPublicKey is currently programmed to return an old style (hashed) peerID since we set AdvancedEnableInlining to false.

OK . Now take a look at the secio handshake... this is their raw code without any modifications:

// get peer id
    actualRemotePeer, err := peer.IDFromPublicKey(s.remote.permanentPubKey)
    if err != nil {
        return err
    }
    switch s.remotePeer {
    case actualRemotePeer:
        // All good.
    case "":
        // No peer set. We're accepting a remote connection.
        s.remotePeer = actualRemotePeer
    default:
        // Peer mismatch. Bail.
        s.insecure.Close()
        log.Debugf("expected peer %s, got peer %s", s.remotePeer, actualRemotePeer)
        return ErrWrongPeer
    }

If we ever enable inline keys, old nodes which have not upgraded, when trying to connect to a new style peerID, will pass in a new style ID into s.remotePeer but actualRemotePeer will still be an old style key. Hence s.remotePeer != actualRemotePeer and the handshake will fail.

If an upgraded node tries to connect to a non-upgraded peerID then peer.IDFromPublicKey will return a new style peerID while s.remotePeer is an old style key. Hence it will fail to connect.

So to make our current release forward compatible with a future release using inline keys I've done:

// get peer id
    actualRemotePeer, err := peer.IDFromPublicKey(s.remote.permanentPubKey)
    if err != nil {
        return err
    }
    switch s.remotePeer {
    case actualRemotePeer:
        // All good.
    case "":
        // No peer set. We're accepting a remote connection.
        s.remotePeer = actualRemotePeer
    default:
        pubkeyBytes, err := s.remote.permanentPubKey.Bytes()
        if err != nil {
            return err
        }
        oldMultihash, err := mh.Sum(pubkeyBytes, mh.SHA2_256, 32)
        if err != nil {
            return err
        }
        oldStylePeer, err := peer.IDB58Decode(oldMultihash.B58String())
        if err != nil {
            return err
        }
        if s.remotePeer != oldStylePeer {
            // Peer mismatch. Bail.
            s.insecure.Close()
            log.Debugf("expected peer %s, got peer %s", s.remotePeer, actualRemotePeer)
            return ErrWrongPeer
        }
    }

But looking at it, s.remotePeer would be an inline peerID and oldStylePeer would be an old style key, so I think this is screwed up and would prevent old nodes from connecting to new nodes. I think our current release should be a newStylePeer and compare it to s.remotePeer rather than an oldStylePeer.

And then a subsequent release with inline keys should use the code snippet above. (edited)

[cpacia]

This will likely mean we need to fix this for next release and then push out the timeline for updating to the inline keys until at least enough people upgrade to this next release.

[placer14]

Thinking through this problem with annotations around the code:

    primaryTest, err := peer.IDFromPublicKey(s.remote.permanentPubKey)
    // *snip*
    switch s.remotePeer {
    case primaryTest:
        // (Primary Case) All good.
    case "":
        // Ignore
    default:
        // (Backup Case)
        // preparation and checking *snipped*
        if s.remotePeer != oldStylePeer {
          // Failure Case *snip*
        }
    }

Scenarios:

1. OldNode (hashed) connecting to OldNode (hashed, inline off):
   remote (hashed) == primaryTest (hashed)... success in Primary Case
2. OldNode (hashed) connecting to NewNode (inline, inline on):
   remote (hashed) != primaryTest (inline)... remote (hashed) == oldStyle (hashed) success in Backup
3. NewNode (inline) connecting to OldNode (hashed, inline off):
   remote (inline) != primaryTest (hashed)... remote (inline) != oldStyle (hashed) ...always fails
4. NewNode (inline) connecting to NewNode (inline, inline on)
   remote (inline) == primaryTest (inline)... success in Primary Case

I see the problem you're describing... it seems the backup test is always stuck producing the hashed peerID when we seem to want the backupTest to use the other algo opposite from the primaryTest.

I think the way forward for us will be a function ipfs.AlternativeIDFromPublicKey(crypto.PubKey) (peer.ID, error) which looks at the state of AdvancedEnableInlining and applies the opposite algorithm than what IDFromPublicKey applies. And then we can use that function inside of default until we've completed our inline key migration completely. Does that seem reasonable, @cpacia?

[cpacia]

That seems like a good approach. Better than when I was suggesting.