Kimirock/metabase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Add deny list of common passwords (#15912)

Browse Source 
* Add deny list of common passwords

* replace filtered pw list with full list

* switch to using a FileReader to check password line-by-line

* bypass deny list if complexity is set to easy

* fix tests

* more test fixes and remove unneeded ns

* fix typos

* fix reflection warning

* needed to import File

* fix final tests, and update docs to reflect change

* address initial review comments

* fix build after removing import statement

* address more review comments

* use InputStreamReader to read password file in JARs
This commit is contained in:
Noah Moss
2021-05-10 14:10:04 -07:00
committed by GitHub
parent ddae0bc923
commit dea5093096
16 changed files with 47106 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
docs/api-documentation.md
View File

@@ -2663,7 +2663,7 @@ Reset password with a reset token.
* **`token`** value must be a non-blank string.
* **`password`** Insufficient password strength
* **`password`** Password is insufficiently complex, or is too common
* **`request`**
@@ -2741,7 +2741,7 @@ Special endpoint for creating the first user during setup. This endpoint both cr
* **`auto_run_queries`** value may be nil, or if non-nil, value must be a boolean.
* **`password`** Insufficient password strength
* **`password`** Password is insufficiently complex, or is too common
* **`name`**
@@ -3106,7 +3106,7 @@ Update a user's password.
* **`id`**
* **`password`** Insufficient password strength
* **`password`** Password is insufficiently complex, or is too common
* **`old_password`**
@@ -3165,4 +3165,4 @@ Endpoint that checks if the supplied password meets the currently configured pas
##### PARAMS:
* **`password`** Insufficient password strength
* **`password`** Password is insufficiently complex, or is too common

3
docs/operations-guide/changing-password-complexity.md
View File

@@ -10,3 +10,6 @@ The settings above can be used independently, so it's fine to use only one or th
* `weak` = no character constraints
* `normal` = at least 1 digit
* `strong` = minimum 8 characters w/ 2 lowercase, 2 uppercase, 1 digit, and 1 special character
By default, Metabase also prevents users from setting passwords that are in a list of common passwords (like `qwerty123` and
`passw0rd`). Changing the complexity requirement to `weak` disables this behavior.

47023
resources/common_passwords.txt Normal file
View File

File diff suppressed because it is too large Load Diff

2
src/metabase/api/session.clj
View File

@@ -218,7 +218,7 @@
"Reset password with a reset token."
[:as {{:keys [token password]} :body, :as request}]
{token su/NonBlankString
password su/ComplexPassword}
password su/ValidPassword}
(or (when-let [{user-id :id, :as user} (valid-reset-token->user token)]
(user/set-password! user-id password)
;; if this is the first time the user has logged in it means that they're just accepted their Metabase invite.

2
src/metabase/api/setup.clj
View File

@@ -95,7 +95,7 @@
first_name su/NonBlankString
last_name su/NonBlankString
email su/Email
password su/ComplexPassword
password su/ValidPassword
allow_tracking (s/maybe (s/cond-pre s/Bool su/BooleanString))
schedules (s/maybe sync.schedules/ExpandedSchedulesMap)
auto_run_queries (s/maybe s/Bool)}

2
src/metabase/api/user.clj
View File

@@ -278,7 +278,7 @@
(api/defendpoint PUT "/:id/password"
"Update a user's password."
[id :as {{:keys [password old_password]} :body}]
{password su/ComplexPassword}
{password su/ValidPassword}
(check-self-or-superuser id)
(api/let-404 [user (db/select-one [User :password_salt :password], :id id, :is_active true)]
;; admins are allowed to reset anyone's password (in the admin people list) so no need to check the value of

2
src/metabase/api/util.clj
View File

@@ -13,7 +13,7 @@
(api/defendpoint POST "/password_check"
"Endpoint that checks if the supplied password meets the currently configured password complexity rules."
[:as {{:keys [password]} :body}]
{password su/ComplexPassword} ;; if we pass the su/ComplexPassword test we're g2g
{password su/ValidPassword} ;; if we pass the su/ValidPassword test we're g2g
{:valid true})
(api/defendpoint GET "/logs"

2
src/metabase/public_settings.clj
View File

@@ -341,7 +341,7 @@
"Current password complexity requirements"
:visibility :public
:setter :none
:getter (constantly password/active-password-complexity))
:getter password/active-password-complexity)
(defsetting report-timezone-short
"Current report timezone abbreviation"

32
src/metabase/util/password.clj
View File

@@ -1,6 +1,8 @@
(ns metabase.util.password
"Utility functions for checking passwords against hashes and for making sure passwords match complexity requirements."
(:require [cemerick.friend.credentials :as creds]
[clojure.java.io :as io]
[clojure.string :as str]
[metabase.config :as config]
[metabase.util :as u]))
@@ -47,17 +49,41 @@
(when (>= (occurances char-type) min-count)
(recur more)))))))
(def active-password-complexity
(defn active-password-complexity
"The currently configured description of the password complexity rules being enforced"
[]
(merge (complexity->char-type->min (config/config-kw :mb-password-complexity))
;; Setting MB_PASSWORD_LENGTH overrides the default :total for a given password complexity class
(when-let [min-len (config/config-int :mb-password-length)]
{:total min-len})))
(def ^{:arglists '([password])} is-complex?
(defn- is-complex?
"Check if a given password meets complexity standards for the application."
(partial password-has-char-counts? active-password-complexity))
[password]
(password-has-char-counts? (active-password-complexity) password))
(def ^java.net.URL common-passwords-url
"A set of ~12k common passwords to reject, that otherwise meet Metabase's default complexity requirements.
Sourced from Dropbox's zxcvbn repo: https://github.com/dropbox/zxcvbn/blob/master/data/passwords.txt"
(io/resource "common_passwords.txt"))
(defn- is-uncommon?
"Check if a given password is not present in the common passwords set. Case-insensitive search since
the list only contains lower-case passwords."
[password]
(with-open [is (.openStream common-passwords-url)
reader (java.io.BufferedReader. (java.io.InputStreamReader. is))]
(not-any?
(partial = (str/lower-case password))
(iterator-seq (.. reader lines iterator)))))
(defn is-valid?
"Check that a password both meets complexity standards, and is not present in the common passwords list.
Common password list is ignored if minimum password complexity is set to :weak"
[password]
(and (is-complex? password)
(or (= (config/config-kw :mb-password-complexity) :weak)
(is-uncommon? password))))
(defn verify-password
"Verify if a given unhashed password + salt matches the supplied hashed-password. Returns `true` if matched, `false`

8
src/metabase/util/schema.clj
View File

@@ -232,10 +232,10 @@
(with-api-error-message (s/constrained s/Str u/email? (deferred-tru "Valid email address"))
(deferred-tru "value must be a valid email address.")))
(def ComplexPassword
"Schema for a valid password of sufficient complexity."
(with-api-error-message (s/constrained s/Str password/is-complex?)
(deferred-tru "Insufficient password strength")))
(def ValidPassword
"Schema for a valid password of sufficient complexity which is not found on a common password list."
(with-api-error-message (s/constrained s/Str password/is-valid?)
(deferred-tru "Password is insufficiently complex, or is too common")))
(def IntString
"Schema for a string that can be parsed as an integer.

2
test/metabase/api/session_test.clj
View File

@@ -290,7 +290,7 @@
(testing "Test that token and password are required"
(is (= {:errors {:token "value must be a non-blank string."}}
(mt/client :post 400 "session/reset_password" {})))
(is (= {:errors {:password "Insufficient password strength"}}
(is (= {:errors {:password "Password is insufficiently complex, or is too common"}}
(mt/client :post 400 "session/reset_password" {:token "anything"}))))
(testing "Test that malformed token returns 400"

6
test/metabase/api/setup_test.clj
View File

@@ -225,11 +225,11 @@
(testing "password"
(testing "missing"
(is (= {:errors {:password "Insufficient password strength"}}
(is (= {:errors {:password "Password is insufficiently complex, or is too common"}}
(setup! m/dissoc-in [:user :password]))))
(testing "invalid"
(is (= {:errors {:password "Insufficient password strength"}}
(is (= {:errors {:password "Password is insufficiently complex, or is too common"}}
(setup! assoc-in [:user :password] "anything"))))))))
(deftest setup-with-empty-cache-test
@@ -257,7 +257,7 @@
:user {:first_name (mt/random-name)
:last_name (mt/random-name)
:email user-email
:password "p@ssw0rd"}}]
:password "p@ssword1"}}]
(do-with-setup*
body
(fn []

2
test/metabase/api/user_test.clj
View File

@@ -804,7 +804,7 @@
(deftest reset-password-input-validation-test
(testing "PUT /api/user/:id/password"
(testing "Test input validations on password change"
(is (= {:errors {:password "Insufficient password strength"}}
(is (= {:errors {:password "Password is insufficiently complex, or is too common"}}
(mt/user-http-request :rasta :put 400 (format "user/%d/password" (mt/user->id :rasta)) {}))))
(testing "Make sure that if current password doesn't match we get a 400"

6
test/metabase/api/util_test.clj
View File

@@ -6,13 +6,13 @@
(deftest password-check-test
(testing "POST /api/util/password_check"
(testing "Test for required params"
(is (= {:errors {:password "Insufficient password strength"}}
(is (= {:errors {:password "Password is insufficiently complex, or is too common"}}
(mt/client :post 400 "util/password_check" {}))))
(testing "Test complexity check"
(is (= {:errors {:password "Insufficient password strength"}}
(is (= {:errors {:password "Password is insufficiently complex, or is too common"}}
(mt/client :post 400 "util/password_check" {:password "blah"}))))
(testing "Should be a valid password"
(is (= {:valid true}
(mt/client :post 200 "util/password_check" {:password "something1"}))))))
(mt/client :post 200 "util/password_check" {:password "something123"}))))))

3
test/metabase/models/user_test.clj
View File

@@ -83,7 +83,7 @@
(when-let [[{[{invite-email :content}] :body}] (get @mt/inbox new-user-email-address)]
(let [[_ reset-token] (re-find #"/auth/reset_password/(\d+_[\w_-]+)#new" invite-email)]
(http/client :post 200 "session/reset_password" {:token reset-token
:password "ABC123"}))))
:password "p@ssword1"}))))
(defn sent-emails
"Fetch the emails that have been sent in the form of a map of email address -> sequence of email subjects.
@@ -96,7 +96,6 @@
[address (for [{subject :subject} emails]
(str/replace subject (str new-user-first-name " " new-user-last-name) "<New User>"))])))
(defn- invite-user-accept-and-check-inboxes!
"Create user by passing `invite-user-args` to `create-and-invite-user!` or `create-new-google-auth-user!`,
and return a map of addresses emails were sent to to the email subjects."

36
test/metabase/util/password_test.clj
View File

@@ -1,7 +1,12 @@
(ns metabase.util.password-test
(:require [clojure.test :refer :all]
[environ.core :as environ]
[metabase.test :as mt]
[metabase.test.fixtures :as fixtures]
[metabase.util.password :as pwu]))
(use-fixtures :once (fixtures/initialize :db))
;; Password Complexity testing
(deftest count-occurrences-test
@@ -47,12 +52,29 @@
(is (= expected
(apply #'pwu/password-has-char-counts? input))))))))
(deftest is-complex?-test
(deftest is-valid?-normal-test
(testing "Do some tests with the default (:normal) password requirements"
(doseq [[input expected] {"ABC" false
"ABCDEF" false
"ABCDE1" true
"123456" true}]
(testing (pr-str (list 'is-complex? input))
(doseq [[input expected] {"ABC" false
"ABCDEF" false
"ABCDE1" false
"123456" false
"passw0rd" false
"PASSW0RD" false
"unc0mmonpw" true
"pa$$w0®∂" true
"s6n!8z-6.gcJe" true}]
(testing (pr-str (list 'is-valid? input))
(is (= expected
(pwu/is-complex? input)))))))
(pwu/is-valid? input)))))))
(deftest is-valid?-weak-test
(testing "Do some tests with password complexity requirements set to :weak.
Common password list should not be checked."
(mt/with-temp-env-var-value [:mb-password-complexity "weak"]
(doseq [[input expected] {"ABC" false
"ABCDEF" true
"ABCDE1" true
"passw0rd" true}]
(testing (pr-str (list 'is-valid? input))
(is (= expected
(pwu/is-valid? input))))))))