From 0e1e38011a622a6a9ab081cd6b5c57346038ee5f Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Tue, 25 Aug 2015 22:43:42 -0700 Subject: [PATCH] Review fixes pt. 2 --- ca/certificate-authority_test.go | 2 +- cmd/policy-loader/main.go | 5 +- docs/database/db_schema-pa.sql | 15 --- policy/_db/dbconf.yml | 6 +- ...olicy-data.go => policy-authority-data.go} | 34 +++--- policy/policy-authority-data_test.go | 88 +++++++++++++++ policy/policy-authority_test.go | 9 +- policy/policy-data_test.go | 102 ------------------ ra/registration-authority_test.go | 10 +- test/create_db.sh | 4 +- wfe/web-front-end_test.go | 4 +- 11 files changed, 123 insertions(+), 156 deletions(-) delete mode 100644 docs/database/db_schema-pa.sql rename policy/{policy-data.go => policy-authority-data.go} (90%) create mode 100644 policy/policy-authority-data_test.go delete mode 100644 policy/policy-data_test.go diff --git a/ca/certificate-authority_test.go b/ca/certificate-authority_test.go index 90051c017..b45e8f73d 100644 --- a/ca/certificate-authority_test.go +++ b/ca/certificate-authority_test.go @@ -344,7 +344,7 @@ const caKeyFile = "../test/test-ca.key" const caCertFile = "../test/test-ca.pem" const ( - paDBConnStr = "mysql+tcp://boulder@localhost:3306/boulder_pa_test" + paDBConnStr = "mysql+tcp://boulder@localhost:3306/boulder_policy_test" caDBConnStr = "mysql+tcp://boulder@localhost:3306/boulder_ca_test" saDBConnStr = "mysql+tcp://boulder@localhost:3306/boulder_sa_test" ) diff --git a/cmd/policy-loader/main.go b/cmd/policy-loader/main.go index 78773e279..18bfac6d3 100644 --- a/cmd/policy-loader/main.go +++ b/cmd/policy-loader/main.go @@ -45,7 +45,10 @@ func setupFromContext(context *cli.Context) (*policy.PolicyAuthorityDatabaseImpl func main() { app := cli.NewApp() app.Name = "policy-loader" - app.Version = "0.0.1" + app.Usage = "Loads/dumps rules into/from the policy database" + app.Version = cmd.Version() + app.Author = "Boulder contributors" + app.Email = "ca-dev@letsencrypt.org" app.Flags = []cli.Flag{ cli.StringFlag{ diff --git a/docs/database/db_schema-pa.sql b/docs/database/db_schema-pa.sql deleted file mode 100644 index 4e0a8c083..000000000 --- a/docs/database/db_schema-pa.sql +++ /dev/null @@ -1,15 +0,0 @@ --- --- Copyright 2015 ISRG. All rights reserved --- This Source Code Form is subject to the terms of the Mozilla Public --- License, v. 2.0. If a copy of the MPL was not distributed with this --- file, You can obtain one at http://mozilla.org/MPL/2.0/. --- --- This file defines the table schema, foreign keys and indicies of the --- Policy Authority database, which is logically separate from that which --- is utilized by the Storage Authority and administrator tools. --- - -CREATE TABLE `ruleList` ( - `host` varchar(255) NOT NULL, - `type` char(11) NOT NULL -) ENGINE=InnoDB DEFAULT CHARSET=utf8; diff --git a/policy/_db/dbconf.yml b/policy/_db/dbconf.yml index 5f1be4e2b..74496c6cc 100644 --- a/policy/_db/dbconf.yml +++ b/policy/_db/dbconf.yml @@ -1,9 +1,9 @@ development: driver: mysql - open: boulder@tcp(localhost:3306)/boulder_pa_development + open: boulder@tcp(localhost:3306)/boulder_policy_development test: driver: mysql - open: boulder@tcp(localhost:3306)/boulder_pa_test + open: boulder@tcp(localhost:3306)/boulder_policy_test integration: driver: mysql - open: boulder@tcp(localhost:3306)/boulder_pa_integration + open: boulder@tcp(localhost:3306)/boulder_policy_integration diff --git a/policy/policy-data.go b/policy/policy-authority-data.go similarity index 90% rename from policy/policy-data.go rename to policy/policy-authority-data.go index a1377fa89..b6a38f20b 100644 --- a/policy/policy-data.go +++ b/policy/policy-authority-data.go @@ -6,6 +6,7 @@ package policy import ( + "database/sql" "fmt" "strings" @@ -45,11 +46,6 @@ func NewPolicyAuthorityDatabaseImpl(dbMap *gorp.DbMap) (padb *PolicyAuthorityDat dbMap.AddTableWithName(DomainRule{}, "ruleList").SetKeys(false, "Host") - err = dbMap.CreateTablesIfNotExists() - if err != nil { - return - } - padb = &PolicyAuthorityDatabaseImpl{ dbMap: dbMap, log: logger, @@ -94,19 +90,19 @@ func (padb *PolicyAuthorityDatabaseImpl) DumpRules() ([]DomainRule, error) { } func (padb *PolicyAuthorityDatabaseImpl) checkBlacklist(host string) error { - var rule *DomainRule + var rule DomainRule // Use lexical odering to quickly find blacklisted root domains - _, err := padb.dbMap.Select( - rule, + err := padb.dbMap.SelectOne( + &rule, `SELECT * FROM ruleList WHERE :host >= host AND type = 'blacklist' ORDER BY host DESC LIMIT 1`, map[string]interface{}{"host": host}, ) if err != nil { + if err == sql.ErrNoRows { + return nil + } return err } - if rule == nil { - return nil - } if host == rule.Host || strings.HasPrefix(host, rule.Host+".") { return BlacklistedError{} @@ -115,23 +111,21 @@ func (padb *PolicyAuthorityDatabaseImpl) checkBlacklist(host string) error { } func (padb *PolicyAuthorityDatabaseImpl) checkWhitelist(host string) error { - var rule *DomainRule + var rule DomainRule // Because of how rules are sorted if there is a relevant whitelist AND blacklist // rule we will catch them both, this query will return a maximum of two rules - _, err := padb.dbMap.Select( - rule, - `SELECT * FROM ruleList WHERE :host = host AND type = 'whitelist' DESC LIMIT 1`, + err := padb.dbMap.SelectOne( + &rule, + `SELECT * FROM ruleList WHERE :host = host AND type = 'whitelist' LIMIT 1`, map[string]interface{}{"host": host}, ) if err != nil { - fmt.Println("BAD", err) + if err == sql.ErrNoRows { + return fmt.Errorf("Domain name is not whitelisted for issuance") + } return err } - if rule == nil { - return fmt.Errorf("Domain name is not whitelisted for issuance") - } - return nil } diff --git a/policy/policy-authority-data_test.go b/policy/policy-authority-data_test.go new file mode 100644 index 000000000..5db515e9d --- /dev/null +++ b/policy/policy-authority-data_test.go @@ -0,0 +1,88 @@ +// Copyright 2015 ISRG. All rights reserved +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +package policy + +import ( + "testing" + + "github.com/letsencrypt/boulder/sa" + "github.com/letsencrypt/boulder/test" +) + +func TestBlacklist(t *testing.T) { + p, cleanup := padbImpl(t) + defer cleanup() + + err := p.LoadRules([]DomainRule{ + DomainRule{ + Host: "bad.com", + Type: blacklisted, + }, + DomainRule{ + Host: "good.bad.com", + Type: whitelisted, + }, + }) + test.AssertNotError(t, err, "Couldn't load rules") + + err = p.CheckRules("bad.com", false) + test.AssertError(t, err, "Hostname should be blacklisted") + err = p.CheckRules("still.bad.com", false) + test.AssertError(t, err, "Hostname should be blacklisted") + err = p.CheckRules("badminton.com", false) + test.AssertNotError(t, err, "Hostname shouldn't be blacklisted") + // Whitelisted subdomain of blacklisted root should still be blacklsited + err = p.CheckRules("good.bad.com", false) + test.AssertError(t, err, "Blacklist should beat whitelist") + // Not blacklisted + err = p.CheckRules("good.com", false) + test.AssertNotError(t, err, "Hostname shouldn't be blacklisted") +} + +func TestWhitelist(t *testing.T) { + p, cleanup := padbImpl(t) + defer cleanup() + + err := p.LoadRules([]DomainRule{ + DomainRule{ + Host: "bad.com", + Type: blacklisted, + }, + DomainRule{ + Host: "good.bad.com", + Type: whitelisted, + }, + DomainRule{ + Host: "good.com", + Type: whitelisted, + }, + }) + test.AssertNotError(t, err, "Couldn't load rules") + + err = p.CheckRules("bad.com", true) + test.AssertError(t, err, "Hostname should be blacklisted") + // Whitelisted subdomain of blacklisted root should still be blacklsited + err = p.CheckRules("good.bad.com", true) + test.AssertError(t, err, "Blacklist should beat whitelist") + // Non-existent domain should fail + err = p.CheckRules("not-good.com", true) + test.AssertError(t, err, "Hostname isn't on whitelist") + // Whitelisted + err = p.CheckRules("good.com", true) + test.AssertNotError(t, err, "Hostname is on whitelist") +} + +func padbImpl(t *testing.T) (*PolicyAuthorityDatabaseImpl, func()) { + dbMap, err := sa.NewDbMap(dbConnStr) + test.AssertNotError(t, err, "Could not construct dbMap") + + padb, err := NewPolicyAuthorityDatabaseImpl(dbMap) + test.AssertNotError(t, err, "Couldn't create PADB") + + cleanUp := test.ResetTestDatabase(t, dbMap.Db) + + return padb, cleanUp +} diff --git a/policy/policy-authority_test.go b/policy/policy-authority_test.go index 636b56f4d..c47b1fd31 100644 --- a/policy/policy-authority_test.go +++ b/policy/policy-authority_test.go @@ -15,7 +15,7 @@ import ( ) var log = mocks.UseMockLog() -var dbConnStr = "mysql+tcp://boulder@localhost:3306/boulder_test" +var dbConnStr = "mysql+tcp://boulder@localhost:3306/boulder_policy_test" func paImpl(t *testing.T) (*PolicyAuthorityImpl, func()) { dbMap, err := sa.NewDbMap(dbConnStr) @@ -24,12 +24,7 @@ func paImpl(t *testing.T) (*PolicyAuthorityImpl, func()) { pa, err := NewPolicyAuthorityImpl(dbMap, false) test.AssertNotError(t, err, "Couldn't create PADB") - cleanUp := func() { - if err := dbMap.TruncateTables(); err != nil { - t.Fatalf("Could not truncate tables after the test: %s", err) - } - dbMap.Db.Close() - } + cleanUp := test.ResetTestDatabase(t, dbMap.Db) return pa, cleanUp } diff --git a/policy/policy-data_test.go b/policy/policy-data_test.go deleted file mode 100644 index 45a497246..000000000 --- a/policy/policy-data_test.go +++ /dev/null @@ -1,102 +0,0 @@ -// Copyright 2015 ISRG. All rights reserved -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at http://mozilla.org/MPL/2.0/. - -package policy - -import ( - "testing" - - "github.com/letsencrypt/boulder/sa" - "github.com/letsencrypt/boulder/test" -) - -var ( - rA = DomainRule{ - Host: "a.com", - Type: whitelisted, - } - rB = DomainRule{ - Host: "b.com", - Type: blacklisted, - } - rC = DomainRule{ - Host: "d.c.com", - Type: blacklisted, - } - rD = DomainRule{ - Host: "a.d.com", - Type: whitelisted, - } - rE = DomainRule{ - Host: "d.com", - Type: blacklisted, - } -) - -func TestLoadAndDump(t *testing.T) { - p, cleanup := padbImpl(t) - defer cleanup() - - err := p.LoadRules([]DomainRule{rA, rB, rC}) - test.AssertNotError(t, err, "Couldn't load rules") - - r, err := p.DumpRules() - test.AssertNotError(t, err, "Couldn't dump rules") - - test.AssertEquals(t, len(r), 3) -} - -func TestGet(t *testing.T) { - p, cleanup := padbImpl(t) - defer cleanup() - - err := p.LoadRules([]DomainRule{rA, rB, rC, rD, rE}) - test.AssertNotError(t, err, "Couldn't load rules") - - err = p.CheckRules("b.com", false) - test.AssertError(t, err, "Hostname should be blacklisted") - err = p.CheckRules("a.b.com", false) - test.AssertError(t, err, "Hostname should be blacklisted") - - err = p.CheckRules("a.com", false) - test.AssertNotError(t, err, "Hostname should be whitelisted") - err = p.CheckRules("a.a.com", false) - test.AssertNotError(t, err, "Hostname should be whitelisted") - - err = p.CheckRules("a.com", true) - test.AssertNotError(t, err, "Hostname should be whitelisted") - err = p.CheckRules("a.a.com", true) - test.AssertError(t, err, "Hostname isn't explicitly whitelisted") - - err = p.CheckRules("ab.com", false) - test.AssertNotError(t, err, "Hostname should not be blacklisted") - err = p.CheckRules(".b.com", false) - test.AssertError(t, err, "Hostname should be blacklisted") - - err = p.CheckRules("a.d.com", false) - test.AssertNotError(t, err, "Hostname shouldn't be blacklisted") - err = p.CheckRules("d.com", false) - test.AssertError(t, err, "Hostname should be blacklisted") - - err = p.CheckRules("e.d.c.com", false) - test.AssertError(t, err, "Hostname should be blacklisted") -} - -func padbImpl(t *testing.T) (*PolicyAuthorityDatabaseImpl, func()) { - dbMap, err := sa.NewDbMap(dbConnStr) - test.AssertNotError(t, err, "Could not construct dbMap") - - padb, err := NewPolicyAuthorityDatabaseImpl(dbMap) - test.AssertNotError(t, err, "Couldn't create PADB") - - cleanUp := func() { - if err := dbMap.TruncateTables(); err != nil { - t.Fatalf("Could not truncate tables after the test: %s", err) - } - dbMap.Db.Close() - } - - return padb, cleanUp -} diff --git a/ra/registration-authority_test.go b/ra/registration-authority_test.go index e705c2140..f9d434dc3 100644 --- a/ra/registration-authority_test.go +++ b/ra/registration-authority_test.go @@ -130,7 +130,7 @@ var ( ) const ( - paDBConnStr = "mysql+tcp://boulder@localhost:3306/boulder_pa_test" + paDBConnStr = "mysql+tcp://boulder@localhost:3306/boulder_policy_test" caDBConnStr = "mysql+tcp://boulder@localhost:3306/boulder_ca_test" saDBConnStr = "mysql+tcp://boulder@localhost:3306/boulder_sa_test" ) @@ -181,7 +181,12 @@ func initAuthorities(t *testing.T) (*DummyValidationAuthority, *sa.SQLStorageAut } signer, _ := local.NewSigner(caKey, caCert, x509.SHA256WithRSA, basicPolicy) ocspSigner, _ := ocsp.NewSigner(caCert, caCert, caKey, time.Hour) - pa, err := policy.NewPolicyAuthorityImpl(dbMap, false) + paDbMap, err := sa.NewDbMap(paDBConnStr) + if err != nil { + t.Fatalf("Failed to create dbMap: %s", err) + } + policyDBCleanUp := test.ResetTestDatabase(t, paDbMap.Db) + pa, err := policy.NewPolicyAuthorityImpl(paDbMap, false) test.AssertNotError(t, err, "Couldn't create PA") cadb, caDBCleanUp := caDBImpl(t) ca := ca.CertificateAuthorityImpl{ @@ -197,6 +202,7 @@ func initAuthorities(t *testing.T) (*DummyValidationAuthority, *sa.SQLStorageAut cleanUp := func() { saDBCleanUp() caDBCleanUp() + policyDBCleanUp() } csrDER, _ := hex.DecodeString(CSRhex) diff --git a/test/create_db.sh b/test/create_db.sh index 7814e6508..62dc0dfb9 100755 --- a/test/create_db.sh +++ b/test/create_db.sh @@ -9,7 +9,7 @@ function die() { SERVICES="ca sa -pa" +policy" DBENVS="development test integration" @@ -18,7 +18,7 @@ for svc in $SERVICES; do for dbenv in $DBENVS; do db="boulder_${svc}_${dbenv}" - mysql -u root -p -e "drop database if exists \`${db}\`; create database if not exists \`${db}\`; grant all privileges on ${db}.* to 'boulder'@'localhost'" || die "unable to create ${db}" + mysql -u root -e "drop database if exists \`${db}\`; create database if not exists \`${db}\`; grant all privileges on ${db}.* to 'boulder'@'localhost'" || die "unable to create ${db}" echo "created empty ${db} database" goose -path=./$svc/_db/ -env=$dbenv up || die "unable to migrate ${db}" diff --git a/wfe/web-front-end_test.go b/wfe/web-front-end_test.go index 1aa15f713..c0a48f820 100644 --- a/wfe/web-front-end_test.go +++ b/wfe/web-front-end_test.go @@ -128,8 +128,6 @@ wk6Oiadty3eQqSBJv0HnpmiEdQVffIK5Pg4M8Dd+aOBnEkbopAJOuA== "d616e2d6578616d706c652e636f6d300b06092a864886f70d01010b0341008cf8" + "f349efa6d2fadbaf8ed9ba67e5a9b98c3d5a13c06297c4cf36dc76f494e8887e3" + "5dd9c885526136d810fc7640f5ba56281e2b75fa3ff7c91a7d23bab7fd4" - - dbConnStr = "mysql+tcp://boulder@localhost:3306/boulder_test" ) type MockSA struct { @@ -516,7 +514,7 @@ func TestIssueCertificate(t *testing.T) { // TODO: Use a mock RA so we can test various conditions of authorized, not authorized, etc. common := cmd.PAConfig{ - DBConnect: dbConnStr, + DBConnect: "mysql+tcp://boulder@localhost:3306/boulder_policy_test", } ra, _ := ra.NewRegistrationAuthorityImpl(common) ra.SA = &MockSA{}