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

SPKI: TRC signature combination #3341

Merged
merged 2 commits into from
Nov 13, 2019
Merged

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Nov 8, 2019

Add support for combining TRC signature parts into a final signed TRC.

The version to sign can be provided via command line flag.
If no version is specified, the tool searches the newest TRC
configuration file and uses its version.


This change is Reviewable

@oncilla oncilla added the c/tooling SCION network tools label Nov 8, 2019
@oncilla oncilla added this to the Q4S2 milestone Nov 8, 2019
@oncilla oncilla requested a review from scrye November 8, 2019 11:15
@oncilla oncilla force-pushed the pub-spki-combine branch 2 times, most recently from 2166bd5 to e60b595 Compare November 12, 2019 08:42
@scrye scrye removed this from the Q4S2 milestone Nov 12, 2019
@oncilla oncilla requested review from lukedirtwalker and removed request for scrye November 12, 2019 15:52
Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @oncilla)


go/tools/scion-pki/internal/v2/trcs/combine.go, line 69 at r1 (raw file):

		for fname, part := range parts[isd] {
			if !bytes.Equal(proto.Signed.EncodedTRC, part.EncodedTRC) {
				pkicmn.QuietPrint("Ignoring signed in %s. Payload is different\n", fname)

isn't a continue needed here? why isn't that catched by a unit test?


go/tools/scion-pki/internal/v2/trcs/validator.go, line 28 at r1 (raw file):

}

func (v validator) Validate(combined map[addr.ISD]signedMeta) error {

I think a short comment about what this validates would be helpful.

Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker)


go/tools/scion-pki/internal/v2/trcs/combine.go, line 69 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

isn't a continue needed here? why isn't that catched by a unit test?

ugh, right. Great catch.
This branch was never executed in the test. Fixed now.


go/tools/scion-pki/internal/v2/trcs/validator.go, line 28 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I think a short comment about what this validates would be helpful.

Done.

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Add support for combining TRC signature parts into a final signed TRC.

The version to sign can be provided via command line flag.
If no version is specified, the tool searches the newest TRC
configuration file and uses its version.
@oncilla oncilla merged commit 7f84253 into scionproto:master Nov 13, 2019
@oncilla oncilla deleted the pub-spki-combine branch November 13, 2019 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/tooling SCION network tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants