From f978076cdc2b093e2b6a3205636845ada54e8ff8 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Wed, 11 Dec 2019 16:59:31 +0100 Subject: [PATCH] *: allow periods (.) in usernames Requested by a user: > Currently, there is a restriction for the database username which > will limit the certificate-based authentication. It's very common to > include .local (e.g.: internal-service2.local) in the CN (Common Name) > of a certificate. The AWS Certificate Manager (ACM) won't even issue > a certificate if the "dot" (.) is not present. Release note (sql change): Usernames can now contain periods, for compatibility with certificate managers that require domain names to be used as usernames. Release note (security update): The validation rule for principal names was extended to support periods and thus allow domainname-like principals. For reference, the validation rule is currently the regular expression `^[\p{Ll}0-9_][---\p{Ll}0-9_.]+$` and a size limit of 63 UTF-8 bytes (larger usernames are rejected with an error); for comparison, PostgreSQL allows many more characters and truncates at 63 chacters silently. --- pkg/cli/cli_test.go | 2 +- pkg/sql/create_user.go | 13 ++-- pkg/sql/create_user_test.go | 63 +++++++++++++++++++ .../logictest/testdata/logic_test/drop_user | 2 +- pkg/sql/logictest/testdata/logic_test/user | 4 +- 5 files changed, 75 insertions(+), 9 deletions(-) create mode 100644 pkg/sql/create_user_test.go diff --git a/pkg/cli/cli_test.go b/pkg/cli/cli_test.go index 712368207321..9fdcd89f90f9 100644 --- a/pkg/cli/cli_test.go +++ b/pkg/cli/cli_test.go @@ -1339,7 +1339,7 @@ func Example_cert() { // cert create-client Ομηρος // cert create-client 0foo // cert create-client ,foo - // failed to generate client certificate and key: username ",foo" invalid; usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters + // failed to generate client certificate and key: username ",foo" invalid } // TestFlagUsage is a basic test to make sure the fragile diff --git a/pkg/sql/create_user.go b/pkg/sql/create_user.go index a6767a242756..0d7e6df93368 100644 --- a/pkg/sql/create_user.go +++ b/pkg/sql/create_user.go @@ -196,10 +196,10 @@ func (*CreateUserNode) Close(context.Context) {} // FastPathResults implements the planNodeFastPath interface. func (n *CreateUserNode) FastPathResults() (int, bool) { return n.run.rowsAffected, true } -const usernameHelp = "usernames are case insensitive, must start with a letter, " + - "digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters" +const usernameHelp = "Usernames are case insensitive, must start with a letter, " + + "digit or underscore, may contain letters, digits, dashes, periods, or underscores, and must not exceed 63 characters." -var usernameRE = regexp.MustCompile(`^[\p{Ll}0-9_][\p{Ll}0-9_-]{0,62}$`) +var usernameRE = regexp.MustCompile(`^[\p{Ll}0-9_][---\p{Ll}0-9_.]+$`) var blacklistedUsernames = map[string]struct{}{ security.NodeUser: {}, @@ -214,7 +214,7 @@ func NormalizeAndValidateUsername(username string) (string, error) { return "", err } if _, ok := blacklistedUsernames[username]; ok { - return "", errors.Errorf("username %q reserved", username) + return "", pgerror.Newf(pgcode.ReservedName, "username %q reserved", username) } return username, nil } @@ -224,7 +224,10 @@ func NormalizeAndValidateUsername(username string) (string, error) { func NormalizeAndValidateUsernameNoBlacklist(username string) (string, error) { username = tree.Name(username).Normalize() if !usernameRE.MatchString(username) { - return "", errors.Errorf("username %q invalid; %s", username, usernameHelp) + return "", errors.WithHint(pgerror.Newf(pgcode.InvalidName, "username %q invalid", username), usernameHelp) + } + if len(username) > 63 { + return "", errors.WithHint(pgerror.Newf(pgcode.NameTooLong, "username %q is too long", username), usernameHelp) } return username, nil } diff --git a/pkg/sql/create_user_test.go b/pkg/sql/create_user_test.go new file mode 100644 index 000000000000..48c3b66069f0 --- /dev/null +++ b/pkg/sql/create_user_test.go @@ -0,0 +1,63 @@ +// Copyright 2020 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package sql_test + +import ( + "testing" + + "github.com/cockroachdb/cockroach/pkg/sql" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/testutils" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" +) + +func TestUserName(t *testing.T) { + defer leaktest.AfterTest(t)() + + testCases := []struct { + username string + normalized string + err string + sqlstate string + }{ + {"Abc123", "abc123", "", ""}, + {"0123121132", "0123121132", "", ""}, + {"HeLlO", "hello", "", ""}, + {"Ομηρος", "ομηρος", "", ""}, + {"_HeLlO", "_hello", "", ""}, + {"a-BC-d", "a-bc-d", "", ""}, + {"A.Bcd", "a.bcd", "", ""}, + {"WWW.BIGSITE.NET", "www.bigsite.net", "", ""}, + {"", "", `username "" invalid`, pgcode.InvalidName}, + {"-ABC", "", `username "-abc" invalid`, pgcode.InvalidName}, + {".ABC", "", `username ".abc" invalid`, pgcode.InvalidName}, + {"*.wildcard", "", `username "\*.wildcard" invalid`, pgcode.InvalidName}, + {"foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoof", "", `username "foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoof" is too long`, pgcode.NameTooLong}, + } + + for _, tc := range testCases { + normalized, err := sql.NormalizeAndValidateUsername(tc.username) + if !testutils.IsError(err, tc.err) { + t.Errorf("%q: expected %q, got %v", tc.username, tc.err, err) + continue + } + if err != nil { + if pgcode := pgerror.GetPGCode(err); pgcode != tc.sqlstate { + t.Errorf("%q: expected SQLSTATE %s, got %s", tc.username, tc.sqlstate, pgcode) + continue + } + } + if normalized != tc.normalized { + t.Errorf("%q: expected %q, got %q", tc.username, tc.normalized, normalized) + } + } +} diff --git a/pkg/sql/logictest/testdata/logic_test/drop_user b/pkg/sql/logictest/testdata/logic_test/drop_user index ccf09dc9cb8a..8e739cd55fea 100644 --- a/pkg/sql/logictest/testdata/logic_test/drop_user +++ b/pkg/sql/logictest/testdata/logic_test/drop_user @@ -54,7 +54,7 @@ DROP USER IF EXISTS user1 statement error username "node" reserved DROP USER node -statement error pq: username "foo☂" invalid; usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters +statement error pq: username "foo☂" invalid DROP USER foo☂ statement ok diff --git a/pkg/sql/logictest/testdata/logic_test/user b/pkg/sql/logictest/testdata/logic_test/user index d444a72e36e9..3db05ac9ad58 100644 --- a/pkg/sql/logictest/testdata/logic_test/user +++ b/pkg/sql/logictest/testdata/logic_test/user @@ -48,10 +48,10 @@ CREATE USER uSEr2 WITH PASSWORD 'cockroach' statement ok CREATE USER user3 WITH PASSWORD '蟑螂' -statement error pq: username "foo☂" invalid; usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters +statement error pq: username "foo☂" invalid CREATE USER foo☂ -statement error pq: username "-foo" invalid; usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters +statement error pq: username "-foo" invalid CREATE USER "-foo" statement error at or near "-": syntax error