search

google / santa

A binary authorization and monitoring system for macOS
https://santa.dev
Apache License 2.0
4.37k stars 295 forks source link

sync: Add a protobuf for the existing sync protocol #1359

Closed russellhancox closed 1 month ago

russellhancox commented 1 month ago

This PR is intended to have no impact on existing sync servers. The fields and enum values in the protobuf have been named such that their JSON equivalents match the existing constants we have in the codebase.

Adding this provides a few benefits:

1) The protobuf serves as canonical documentation of the protocol in a form that's much easier to read than the existing code. 2) Protobuf parsing of JSON is likely to be better than our hand-written version. 3) We can (in a later PR) add a configuration option to use binary encoding instead of JSON, saving network during syncs. 4) Servers written in other languages are easier to write and update as time goes on, especially as we extend the protocol.

tburgin commented 1 month ago

Can you git mv the .m -> .mm files so we can see the diff?

tburgin commented 1 month ago

Oh all but SNTSyncEventUpload are diff'd, it would be good to get that one too.

pmarkowsky commented 1 month ago

This is awesome. However given how strict proto3's JSON parsing can be.

We should be really careful to make sure we're backwards compatible.

russellhancox commented 1 month ago

Oh all but SNTSyncEventUpload are diff'd, it would be good to get that one too.

Unfortunately Git has no concept of a mv (the mv command is just shorthand for doing git add + git rm), it considers a file renamed if the name changes and a high percentage of the file is content is the same, which I've gone under in this one file. Before opening the PR I tried separating out the changes SNTSyncEventUpload from the rename but that didn't help either as it's still a single PR and GitHub is combining the diff results.

But I've just force-pushed the branch with 2 separate commits again as the "Files Changed" view still shows it as a new file if you view changes from all commits but you can view just the event upload commit on its own to see the smaller diff.

russellhancox commented 1 month ago

This is awesome. However given how strict proto3's JSON parsing can be.

We should be really careful to make sure we're backwards compatible.

All the existing unit tests pass and I've tested a bunch with our internal sync server. Given how strict our hand-written parser was, this should work generally with other servers. We'll also get Moroz testing once this is merged and I'm planning to call this change out in the release notes as something to be very aware of.