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

Invite can not be accepted when using a different app key #1

Open
mindshade opened this issue Feb 14, 2019 · 14 comments
Open

Invite can not be accepted when using a different app key #1

mindshade opened this issue Feb 14, 2019 · 14 comments

Comments

@mindshade
Copy link
Contributor

While experimenting with ssb I have been using custom app key when configuring the server, like for example:

        caps: {
            shs: 'fjZnztSijc/aoGDhCvkFoqoR7JHHOmSXJvKmOP58IrM='
        }

This works fine, and peers connect etc. But when I try to create and use an invite I get the following problems:

Hub side error:

Error: shs.server: client sent invalid challenge (phase 1), possibly they tried to speak a different protocol or had wrong application cap
    at Object.cb (/root/ssb-node/node_modules/secret-handshake/protocol.js:88:30)

Client side error:

Error: could not connect to server
    at /root/ssb-node/node_modules/ssb-invite/index.js:251:29
...
  Error: could not connect to sbot
    at /root/ssb-node/node_modules/ssb-client/index.js:100:23
...
  Error: shs.client: error when expecting server to accept challenge (phase 1).
possibly the server is busy, does not speak shs or uses a different application cap
    at abort (/root/ssb-node/node_modules/secret-handshake/protocol.js:37:45)
...
  Error: stream ended with:0 but wanted:64
    at drain (/root/ssb-node/node_modules/pull-reader/index.js:43:26)

When switching back to the default ssb app key (i.e not providing my own), invites work fine again.

I have tried to figure out where the old key gets picked up (if that is the actual problem), but so far I've been unsuccessful.

If @dominictarr could give me a pointer I would be happy to dig deeper and provide a PR for this.

@dominictarr
Copy link
Contributor

The trick is that the shs cap needs to be the same when connecting to process the invite. at this line I think: https://github.com/ssbc/ssb-invite/blob/master/index.js#L225 @regular and @jfr are doing this I think with a shs cap appended to the invite, i think?

@mindshade
Copy link
Contributor Author

Not sure if this helps, but I added some good old console.log debugging at https://github.com/ssbc/ssb-invite/blob/master/index.js#L225 and the config.caps here is the "new" app key (not the standard).

@dominictarr
Copy link
Contributor

and are you sure the server is also using your custom app key?

@dominictarr
Copy link
Contributor

If you want to make a PR, a failing test case would really help

@mindshade
Copy link
Contributor Author

Created a PR with a test that exposes the problem I have.

@regular
Copy link
Contributor

regular commented Feb 19, 2019

@dominictarr @mindshade that sounds like a bug I ran into too. Does this fix it? ssbc/ssb-server@cdcd385

There's some stuff going on in ssb-client when null is passed as keys that makes the accept call fail. If you pass any keypair (including a new one), that code is circumvented.

@mindshade
Copy link
Contributor Author

mindshade commented Feb 20, 2019

@regular Yes, I can confirm that passing anything instead of null when the config.caps is set makes the new test pass:

ssb-invite/index.js:223

       ...
       function connect (cb) {
          ssbClient(config.caps ? "x" : null, {
            caps: config.caps,
            remote: invite,
            manifest: {invite: {use: 'async'}, getAddress: 'async'}
          }, cb)
        }
       ...

So this issue should be transfered to another repo where the code you refer to resides.

Looks like the problem is related to that default config is loaded when key is null here:

https://github.com/ssbc/ssb-client/blob/master/index.js#L44

Ping @dominictarr

@regular
Copy link
Contributor

regular commented Feb 20, 2019

@mindshade yes, that's where it is. No idea why we would want to call createConfig when we have a perfectly good config and keys == null.

@dominictarr
Copy link
Contributor

oh, the problem here is that in an invite, the address contains a seed, which means it doesn't need a key.
It looks like this "easy" default loading stuff shouldn't be in the code path for things like this at all. It should be way more explicit. Should refactor it so that you can just pass config in, without ssb-client loading config at all.

@dominictarr
Copy link
Contributor

update: ssb-server now runs ssb-client's tests, so should catch future regressions caused by new ssb-server releases.

@mindshade
Copy link
Contributor Author

Great! So, now the ssb-invite plugin needs to use the new ssb-client api to get everything to party.

I tried updatring the dependency against ssb-client to 4.7.0, and changed the import in ssb-invite to:

var ssbClient = require('ssb-client/client')

And then modified the creation of the client in the connect method https://github.com/ssbc/ssb-invite/blob/master/index.js#L223 to:

function connect (cb) {
  var clientConfig = {
    keys: config.keys,
    config: {
      caps: {
        shs: (config.caps && config.caps.shs) ? new Buffer(config.caps.shs, 'base64') : config.appKey
      }
    },
    manifest: {invite: {use: 'async'}, getAddress: 'async'},
    remote: invite
  };
  ssbClient(clientConfig, cb)
}

This make the tests in the PR #2 succeed but I'm not really confident about why config.caps.shs needs to override config.appKey sometimes. I think I'm still missing some pieces here.

Maybe there is something weird hiding under the hood somewhere @dominictarr?

@dominictarr
Copy link
Contributor

appKey was what it was called originally, but later the idea was extended so that every time cryptography is used, there is another cap. especially if signatures are used - see hmac_key here https://github.com/ssbc/ssb-keys#signobjkeys-hmac_key-obj and the chosen protocol attack: https://www.schneier.com/academic/paperfiles/paper-chosen-protocol.pdf

@dominictarr
Copy link
Contributor

oh also, we should probably replace all instances of appKey with caps.shs I think the main reason I didn't do this already is that appKey is used in some old tests, so it was easier to keep it working. (but having now encountered the consequences of this, it's probably better to remember to migrate to the new style)

@mixmix
Copy link
Member

mixmix commented Aug 6, 2020

I think there's a passing test for this now (with caps.shs)

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

No branches or pull requests

4 participants