Pull requests

#20 Declined

trim local certifications from any handled key

  1. Daniel Gillmor

This is in light of recent discussions on sks-devel about sks propagating certifications explicitly marked as non-exportable.

  • Learn about pull requests

Comments (11)

  1. Yaron Minsky

    What happens to all such keys in the database? I'd worry that you'll start throwing exceptions whenever such a key is encountered. Have you tested the patch?

    1. Daniel Gillmor author

      hm, bitbucket apparently wants me to top-post, apologies for that.

      Yes, i have tested this change.

      here's how i tested it.

      0) Start a stock instance of "sks db", import a trivial (small) keydump

      1) through the web interface on port 11371, upload two public keys A and B, one of which (A) has signed the other one (B) with a non-exportable signature.

      2) search for the user ID associated with (B), note that the non-exportable signature is present, both in the op=vindex and op=get views.

      3) stop sks.

      4) start a version of sks with this patch applied, using the same database.

      5) repeat the searches and queries from step 2. note that the non-exportable signature is not present, but the searches work without errors or exceptions.

      If you want me to test it in some other way, please let me know.

      Thanks for considering this patch.

  2. Yaron Minsky

    Sorry for the slow reply. A busy few days.

    I'm just confused with what happens with keys that are already in the database. Imagine that a new keyserver starts up with an out-of-date keydump, which is missing some keys that have non-exportable certifications. When that keyserver reconciles with one that does have these, aren't these going to lead to an exception being thrown somewhere?

    Rather than throwing an exception, I would have thought the right thing to do would be to filter out such certificates when you see them, or when you do a merge. Then, if one SKS server ran a search-and-destroy mission to filter out such certificates on its local copy, then it would cause merges with everyone that it gossiped with, thus squashing them from the whole network.

    That said, it's a little tricky during the period when some servers have this rule and some don't since the certificate squashing will happen on one side and not the other, leading to inconsistent handling of keys, and thus persistent differences.

    I only vaguely remember how it works, but I think there is a filter mechanism that is meant to deal with this case where different servers support different sets of filters on the keyset.

    All in, I don't see how this patch could be doing the right thing.

  3. Yaron Minsky

    Another very simple thing we can do: do this kind of filtering only on keys submitted through the HTTP interface, not through reconciled keys. Then you can stop this problem for new keys, without disrupting the replication mechanism.

  4. Daniel Gillmor author

    can you propose an alternate patch that you think would be doing the right thing? I see the filter lists in fixkey.ml, but i don't see how to implement a new one in that framework.

    i agree that the synchronization phase across machines with/without this patch would be awkward, but it's better to have the awkward phase than to actively propagate data that is marked for non-propagation.

    1. Daniel Gillmor author

      [top-posting again because of bitbucket's broken UI]

      While i certainly wouldn't object to that change, i don't see how to filter just the keys coming in via HTTP.

      Also, doing only this change would mean that a malicious keyserver operator anywhere in the network (not just an immediate gossip peer) would be able to cause the other keyservers to distribute non-exportable certifications. It would be a step in the right direction, but i don't think it's ultimately sufficient.

        1. Yaron Minsky

          Right. To do something better, someone needs to go dig into the filters mechanism and figure out how it works. I took a look at the code, and it's not the clearest thing in the world. Apologies.

          But just throwing an exception when you see one of these is a total disaster, and should not make it in. I think this current pull request should be considered dead. I'm happy to look over a new patch if someone can propose something that works.