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

TrustStore: Implement inserter #3225

Merged
merged 1 commit into from
Oct 7, 2019
Merged

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Oct 4, 2019

Adds:

  • Implement TRC verification and insertion logic.
  • The forwarding inserter registers the new trust material with the
    local certificate server before inserting into the database.
    It is supposed to be used by the beacon and path server.

This change is Reviewable

@oncilla oncilla added the c/CPPKI SCION Control-plane PKI label Oct 4, 2019
@oncilla oncilla added this to the Q4S1 milestone Oct 4, 2019
@oncilla oncilla requested a review from scrye October 4, 2019 12:02
Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 6 files at r1.
Reviewable status: 1 of 6 files reviewed, all discussions resolved (waiting on @scrye)

@scrye scrye self-requested a review October 7, 2019 07:53
Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

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

a discussion (no related file):
Replace all the common.NewBasicError with serrors calls.



go/lib/infra/modules/trust/v2/export_test.go, line 32 at r1 (raw file):

	// inserter for black-box testing.
	NewFwdInserter = newTestFwdInserter
	// NewInserter allows instantiating the private forwarding inserter for

extra forwarding.


go/lib/infra/modules/trust/v2/export_test.go, line 65 at r1 (raw file):

}

// newTestFwdInserter returns a new inserter for testing.

missing forwarding.


go/lib/infra/modules/trust/v2/inserter.go, line 116 at r1 (raw file):

// InsertChain verifies the signed certificate chain and inserts it into the
// database. The issuing TRC is queried through the provider function, when
// necessary.Before insertion, the certificate chain is forwarded to the

Add a space before Before.


go/lib/infra/modules/trust/v2/inserter_test.go, line 85 at r1 (raw file):

			mctrl := gomock.NewController(t)
			defer mctrl.Finish()
			// Prepare the test.

Nit: You can delete the comments and leave a new line instead, it's less clutter. Code is self-explanatory.


go/lib/infra/modules/trust/v2/router.go, line 41 at r1 (raw file):

}

// ChooseServer always routs to the local CS.

s/routs/routes


go/lib/infra/modules/trust/v2/router.go, line 88 at r1 (raw file):

	info, err := r.db.GetTRCInfo(ctx, destination, scrypto.Version(scrypto.LatestVer))
	notFound := xerrors.Is(err, ErrNotFound)
	switch {

Convert to cascaded if, the fact that all cases are for err != nil is a bit weird. Also, this gets rid of a notFound predicate:

if err != nil {
    if notFound {
        return r.isd
    }
    return err
}

Adds:
- Implement TRC verification and insertion logic.
- The forwarding inserter registers the new trust material with the
  local certificate server before inserting into the database.
  It is supposed to be used by the beacon and path server.
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: 1 of 6 files reviewed, 6 unresolved discussions (waiting on @scrye)

a discussion (no related file):

Previously, scrye (Sergiu Costea) wrote…

Replace all the common.NewBasicError with serrors calls.

Ah, cherry-picking without making sure it uses serrors :sad-panda:



go/lib/infra/modules/trust/v2/export_test.go, line 32 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

extra forwarding.

Done.


go/lib/infra/modules/trust/v2/export_test.go, line 65 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

missing forwarding.

Done.


go/lib/infra/modules/trust/v2/inserter.go, line 116 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Add a space before Before.

Done.


go/lib/infra/modules/trust/v2/router.go, line 41 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

s/routs/routes

Done.


go/lib/infra/modules/trust/v2/router.go, line 88 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Convert to cascaded if, the fact that all cases are for err != nil is a bit weird. Also, this gets rid of a notFound predicate:

if err != nil {
    if notFound {
        return r.isd
    }
    return err
}

Done.

Copy link
Contributor

@scrye scrye 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 5 of 5 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@oncilla oncilla merged commit 06e9df0 into scionproto:master Oct 7, 2019
@oncilla oncilla deleted the pub-inserter branch October 7, 2019 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/CPPKI SCION Control-plane PKI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants