Prevent to change Server's trust domain #644

MarcosDY · 2018-11-30T19:15:08Z

Validate Server's trust domain using persisted attestation nodes and registration entries

…t domains Signed-off-by: Marcos Yacob <marcos@scytale.io>

Signed-off-by: Marcos Yacob <marcos@scytale.io>

evan2645 · 2018-11-30T19:30:08Z

pkg/server/server.go

@@ -26,9 +26,22 @@ import (
 	"github.com/spiffe/spire/pkg/server/svid"
 	"google.golang.org/grpc"

+	"errors"
+
+	"github.com/spiffe/spire/proto/server/datastore"


nit: looks like these imports ended up in the wrong place

evan2645 · 2018-11-30T19:31:02Z

pkg/server/server.go

+const (
+	invalidTrustDomainAttestedNode = "An attested node with trust domain '%v' has been detected, " +
+		"which does not match the configured trust domain of '%v'. If you want to change the trust domain, " +
+		"please delete all existing attested nodes"


I don't think this is currently possible? #27 is related

evan2645 · 2018-11-30T19:43:14Z

pkg/server/server.go

+	fetchResponse, err := ds.ListRegistrationEntries(ctx, &datastore.ListRegistrationEntriesRequest{})
+	if err != nil {
+		s.config.Log.Error(err)
+		return errors.New("error trying to fetch entries")


Why not return the error here instead of logging it and returning a different one? Or something like this

return fmt.Errorf("check existing entries: %v", err)

evan2645 · 2018-11-30T19:48:12Z

pkg/server/server.go

+func (s *Server) validateTrustDomain(ctx context.Context, ds datastore.DataStore) error {
+	trustDomain := s.config.TrustDomain.Host
+
+	fetchResponse, err := ds.ListRegistrationEntries(ctx, &datastore.ListRegistrationEntriesRequest{})


I think this will be problematic for production deployments, since we will be scanning the whole table every time we start/restart... we should add a pagination option or something (perhaps as part of a separate PR/Issue?)

evan2645 · 2018-11-30T19:51:50Z

pkg/server/server.go

+func validateSpiffeId(spiffeId string, trustDomain string, errMsg string) error {
+	id, err := url.Parse(spiffeId)
+	if err != nil {
+		return fmt.Errorf("could not parse SPIFFE ID: %v", err)


it would be good if we could include more information here... that it is a registration entry that is bad, and perhaps the entry ID so the user can inspect it/delete it

…s a node with different trust domain Signed-off-by: Marcos Yacob <marcos@scytale.io>

Signed-off-by: Marcos Yacob <marcos@scytale.io>

azdagron · 2018-12-05T21:25:55Z

proto/server/datastore/datastore.proto

@@ -189,14 +189,21 @@ message BySelectors {
    MatchBehavior match = 2;
 }

+message Pagination {
+    uint32 token = 1;


i think a string "token" is more flexible... what do others think?

Agree that was my first approach too

My only gripe is that the sql plugin code is a little pointier when this is a string because we have to convert it and check for errors in a spot that would otherwise be error-free

yes, I added a parser, to prevent it to happens, basically parse to int and put that int into select, and an error is returned in case parse is not valid

azdagron · 2018-12-05T21:30:33Z

pkg/server/plugin/datastore/sql/sql.go

-		if err := entryTx.Where(entryIDs).Find(&models).Error; err != nil {
-			return nil, sqlError.Wrap(err)
+		db := entryTx.Where(entryIDs)
+		if err := findRegisteredEntries(req, db, &models); err != nil {


i think this is less confusing with a functional approach:

models, pagination, err := findRegisteredEntries(db, req.Pagination) if err != nil {

azdagron · 2018-12-05T21:31:28Z

pkg/server/plugin/datastore/sql/sql.go

+}
+
+// update pagination token based in last result in returned list
+func updatePaginationToken(p *datastore.Pagination, entries *[]RegisteredEntry) {


it's seems weird to take a pointer to a slice...

azdagron · 2018-12-05T21:34:15Z

pkg/server/server_test.go

 	"io/ioutil"
 	"net/url"
 	"os"
 	"testing"

+	"fmt"


"fmt" and "bytes" should be grouped with the rest of the std imports

azdagron · 2018-12-05T21:44:15Z

test/fakes/fakedatastore/fakedatastore.go

@@ -347,6 +347,19 @@ func (s *DataStore) ListRegistrationEntries(ctx context.Context,
 	s.mu.Lock()
 	defer s.mu.Unlock()

+	// no pagination allow for this fake, for now it return only one page


should we implement pagination in the fake datastore so the server startup code that paginates entries for trust domain validation can be tested?

…aginate entries Signed-off-by: Marcos Yacob <marcos@scytale.io>

Signed-off-by: Marcos Yacob <marcos@scytale.io>

evan2645 · 2018-12-06T21:54:59Z

pkg/server/server.go

+	trustDomain := s.config.TrustDomain.Host
+
+	var token string
+	// Repeat until no more results are returned


I don't think we need to iterate over all the entries... perhaps we just make the request with PageSize 1 and call it a day? Same for attested nodes

evan2645 · 2018-12-06T22:16:16Z

proto/server/datastore/datastore.proto

@@ -189,14 +189,21 @@ message BySelectors {
    MatchBehavior match = 2;
 }

+message Pagination {
+    uint32 token = 1;


My only gripe is that the sql plugin code is a little pointier when this is a string because we have to convert it and check for errors in a spot that would otherwise be error-free

Signed-off-by: Marcos Yacob <marcos@scytale.io>

azdagron · 2018-12-19T17:31:31Z

pkg/server/plugin/datastore/sql/sql.go

@@ -656,8 +666,14 @@ func listAttestedNodes(tx *gorm.DB, req *datastore.ListAttestedNodesRequest) (*d
 		return nil, sqlError.Wrap(err)
 	}

+	if p != nil && p.PageSize > 0 && len(models) > 0 {
+		lastEntry := (models)[len(models)-1]


nit: () on (models) is unnecessary

Signed-off-by: Marcos Yacob <marcos@scytale.io>

MarcosDY added 2 commits November 30, 2018 12:35

validate trust domain when starting server to prevent changes in trus…

f8ba53f

…t domains Signed-off-by: Marcos Yacob <marcos@scytale.io>

add unit tests for validate trust domain

c251066

Signed-off-by: Marcos Yacob <marcos@scytale.io>

MarcosDY requested review from amartinezfayo, azdagron, evan2645, martincapello and walmav as code owners November 30, 2018 19:15

evan2645 reviewed Nov 30, 2018

View reviewed changes

MarcosDY added 3 commits December 3, 2018 10:42

Update error message and return warning instead of error when there i…

33c248c

…s a node with different trust domain Signed-off-by: Marcos Yacob <marcos@scytale.io>

Add pagination feature to list registered entries

7dd1ece

Signed-off-by: Marcos Yacob <marcos@scytale.io>

add test for invalid spiffe ids in registered entries or attested nodes

a32526b

Signed-off-by: Marcos Yacob <marcos@scytale.io>

azdagron reviewed Dec 5, 2018

View reviewed changes

MarcosDY and others added 5 commits December 5, 2018 23:29

refactor pagination to use strings as token, allow fakedatastore to p…

1fb7546

…aginate entries Signed-off-by: Marcos Yacob <marcos@scytale.io>

in case token is not specified set "0" as default

51be261

Signed-off-by: Marcos Yacob <marcos@scytale.io>

Merge branch 'master' into validate-trustdomain-when-starting

0e9e655

validate token before using in query

2c8619e

Signed-off-by: Marcos Yacob <marcos@scytale.io>

Merge branch 'master' into validate-trustdomain-when-starting

e5d4dd8

evan2645 reviewed Dec 6, 2018

View reviewed changes

MarcosDY and others added 2 commits December 6, 2018 20:51

add pagination to list attested nodes

42e46e5

Signed-off-by: Marcos Yacob <marcos@scytale.io>

Merge branch 'master' into validate-trustdomain-when-starting

1ea945f

azdagron previously approved these changes Dec 19, 2018

View reviewed changes

remove unnecessary ()

191955b

Signed-off-by: Marcos Yacob <marcos@scytale.io>

MarcosDY dismissed azdagron’s stale review via 191955b December 19, 2018 18:01

azdagron approved these changes Dec 19, 2018

View reviewed changes

MarcosDY merged commit 0776a9b into spiffe:master Dec 19, 2018

MarcosDY deleted the validate-trustdomain-when-starting branch December 19, 2018 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent to change Server's trust domain #644

Prevent to change Server's trust domain #644

MarcosDY commented Nov 30, 2018

evan2645 Nov 30, 2018

evan2645 Nov 30, 2018

evan2645 Nov 30, 2018

evan2645 Nov 30, 2018

evan2645 Nov 30, 2018

azdagron Dec 5, 2018

MarcosDY Dec 6, 2018

evan2645 Dec 6, 2018

MarcosDY Dec 6, 2018

azdagron Dec 5, 2018

MarcosDY Dec 6, 2018

azdagron Dec 5, 2018

MarcosDY Dec 6, 2018

azdagron Dec 5, 2018

MarcosDY Dec 6, 2018

azdagron Dec 5, 2018

MarcosDY Dec 6, 2018

evan2645 Dec 6, 2018

evan2645 Dec 6, 2018

azdagron Dec 19, 2018

Prevent to change Server's trust domain #644

Prevent to change Server's trust domain #644

Conversation

MarcosDY commented Nov 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment