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

[Formats] Rename Ecosystem checkpoint field to Origin #470

Merged
merged 5 commits into from
Sep 2, 2021

Conversation

AlCutter
Copy link
Member

@AlCutter AlCutter commented Sep 1, 2021

The Ecosystem is a properly of the log which is a member of that ecosystem, whereas the checkpoint is the embodiment of a claim made by that log and as such should be explicit about its origin.

This change renders checkpoints signed by the "wrong" log key invalid, and mitigates a class of targeted attack when using witnesses to protect against split views.

@AlCutter
Copy link
Member Author

AlCutter commented Sep 1, 2021

There are one or two more references to Ecosystem in the GitHub actions in this repo, but since they're actually separate modules I'll need to bump them separately once this PR has merged.

Copy link
Contributor

@smeiklej smeiklej left a comment

Choose a reason for hiding this comment

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

Two potential typos but otherwise looks good :).


The presence of this identifier forms part of the log claim, and prevents witness
signatures being moved between log checkpoints with identical contents, preventing a
particular type of targetted attack.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is just spelled "targeted" but maybe that's an American thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixxed


> Note that golang sumdb implementation is already compatible with this
`[otherdata]` extension (see
<https://github.com/golang/mod/blob/d6ab96f2441f9631f81862375ef66782fc4a9c12/sumdb/tlog/note.go#L52>).

The first signature on a checkpoint must be from the log which issued it.
The first signature on a checkpoint should be from the log which issued it, but there MUST NOT
be more than one signature from a log identity present on the checkpoint ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what's happened with the empty brackets?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I was going to add a parenthetical about it being nonsensical to have a signature on a checkpoint from a log other than the one identified by the zeroth line, but that's probably not really necessary so I've ding'd 'em.

@asraa asraa mentioned this pull request Sep 1, 2021
4 tasks
Comment on lines 60 to 62
The presence of this identifier forms part of the log claim, and prevents witness
signatures being moved between log checkpoints with identical contents, preventing a
particular type of targeted attack.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is true, but expanding this won't do us any harm. Something a little closer to a compelling explanation for me would be something like:

Including a unique log string in the checkpoint prevents distinct logs from creating checkpoints with the same contents. This prevents signatures from being transferred between checkpoints. As an example of an attack that this mitigates: a witness signature on a checkpoint confirms that the log's append-only property has been verified as of this checkpoint. If another log creates the same checkpoint, the witness signature can be used on that checkpoint which implies a verification check that may never have happened.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not at all against sharing the details, but I'm not sure whether this is the right place to put them - if I'm implementing this, I suspect that at a first pass I just want to know what the fields are and how I should use them, later on I might be interested to know why.

Would having these sorts of details in an issue or appendix section be better? (Maybe I should also remove the "preventing a particular type..." bit too?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think dropping that removes the slippery slope into explaining the whole thing here. To keep this short and still accurate, perhaps "The presence of this identifier forms part of the log claim, and means that no two logs can create byte-equivalent checkpoints.".

Witnesses, signatures, and all of that can then be moved elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reworded and remove the extra stuff for now - we can figure out where to put that later.


**Differences from sumdb root:**
The sumbdb note has `go.sum database tree` as its ecosystem string.
The sumbdb note has `go.sum database tree` as its `Origin`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really a "difference" as the title suggests?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed the section

@AlCutter AlCutter merged commit bc25207 into google:master Sep 2, 2021
@AlCutter AlCutter deleted the formats_ecosystem_to_origin branch September 2, 2021 10:19
mhutchinson added a commit to mhutchinson/trillian-examples that referenced this pull request Sep 6, 2021
This change identified a few tests that were still passing using the old origin string that was replaced in google#470. I think this is compelling that reusing the same parsing code reduces bugs.
mhutchinson added a commit that referenced this pull request Sep 7, 2021
This change identified a few tests that were still passing using the old origin string that was replaced in #470. I think this is compelling that reusing the same parsing code reduces bugs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes CLA signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants