Skip to content

Commit

Permalink
Revert "check file download path for security"
Browse files Browse the repository at this point in the history 
This reverts commit 6a421d5.
  • Loading branch information
Derek Dagit committed Aug 21, 2013
1 parent 6a421d5 commit 935e295
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 54 deletions.
12 changes: 0 additions & 12 deletions storm-core/src/clj/backtype/storm/daemon/nimbus.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -881,17 +881,6 @@
(throw (InvalidTopologyException.
(str "Topology name cannot contain any of the following: " (pr-str DISALLOWED-TOPOLOGY-NAME-STRS))))))

(defn check-file-location! [conf file-path]
(log-debug "check file location:" file-path)
(if (clojure.string/blank? file-path)
(throw (AuthorizationException. (str "Invalid file path:" file-path))))
(let [stormdist-dir (str (master-stormdist-root conf) "/")
stormdist-dir-len (.length stormdist-dir)
file-path-len (.length file-path)
file-path-prefix (if (> file-path-len stormdist-dir-len) (.substring file-path 0 stormdist-dir-len))]
(if (not= (compare stormdist-dir file-path-prefix) 0)
(throw (AuthorizationException. (str "Invalid file path:" file-path))))))

(defn- try-read-storm-conf [conf storm-id]
(try-cause
(read-storm-conf conf storm-id)
Expand Down Expand Up @@ -1095,7 +1084,6 @@
))

(^String beginFileDownload [this ^String file]
(check-file-location! (:conf nimbus) file)
(check-authorization! nimbus nil nil "fileDownload")
(let [is (BufferFileInputStream. file)
id (uuid)]
Expand Down
78 changes: 36 additions & 42 deletions storm-core/test/clj/backtype/storm/nimbus_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,13 +146,7 @@
(is (not-nil? ((:executor->start-time-secs assignment) e))))
))

(deftest test-file-bogus-bogus-download
(with-local-cluster [cluster :daemon-conf {SUPERVISOR-ENABLE false TOPOLOGY-ACKER-EXECUTORS 0}]
(let [nimbus (:nimbus cluster)]
(is (thrown? AuthorizationException (.beginFileDownload nimbus nil)))
(is (thrown? AuthorizationException (.beginFileDownload nimbus "")))
(is (thrown? AuthorizationException (.beginFileDownload nimbus "/bogus-path/foo")))
)))


(deftest test-bogusId
(with-local-cluster [cluster :supervisors 4 :ports-per-supervisor 3 :daemon-conf {SUPERVISOR-ENABLE false TOPOLOGY-ACKER-EXECUTORS 0}]
Expand Down Expand Up @@ -234,7 +228,7 @@
{"1" (thrift/mk-spout-spec (TestPlannerSpout. false) :parallelism-hint 3)}
{"2" (thrift/mk-bolt-spec {"1" :none} (TestPlannerBolt.) :parallelism-hint 5)
"3" (thrift/mk-bolt-spec {"2" :none} (TestPlannerBolt.))}))

(submit-local-topology nimbus "noniso" {TOPOLOGY-OPTIMIZE false TOPOLOGY-WORKERS 4} topology)
(advance-cluster-time cluster 1)
(is (= 4 (topology-num-nodes state "noniso")))
Expand All @@ -243,32 +237,32 @@
(submit-local-topology nimbus "tester1" {TOPOLOGY-OPTIMIZE false TOPOLOGY-WORKERS 6} topology)
(submit-local-topology nimbus "tester2" {TOPOLOGY-OPTIMIZE false TOPOLOGY-WORKERS 6} topology)
(advance-cluster-time cluster 1)

(bind task-info-tester1 (storm-component->task-info cluster "tester1"))
(bind task-info-tester2 (storm-component->task-info cluster "tester2"))


(is (= 1 (topology-num-nodes state "noniso")))
(is (= 3 (storm-num-workers state "noniso")))

(is (= {2 3} (topology-node-distribution state "tester1")))
(is (= {3 2} (topology-node-distribution state "tester2")))

(is (apply disjoint? (map (partial topology-nodes state) ["noniso" "tester1" "tester2"])))

(check-consistency cluster "tester1")
(check-consistency cluster "tester2")
(check-consistency cluster "noniso")

;;check that nothing gets reassigned
(bind tester1-slots (topology-slots state "tester1"))
(bind tester2-slots (topology-slots state "tester2"))
(bind noniso-slots (topology-slots state "noniso"))
(bind noniso-slots (topology-slots state "noniso"))
(advance-cluster-time cluster 20)
(is (= tester1-slots (topology-slots state "tester1")))
(is (= tester2-slots (topology-slots state "tester2")))
(is (= noniso-slots (topology-slots state "noniso")))

)))

(deftest test-zero-executor-or-tasks
Expand Down Expand Up @@ -302,7 +296,7 @@
(check-consistency cluster "mystorm")
(is (= 5 (count (task-info "1"))))
(check-distribution (executor-info "1") [2 2 1])

(is (= 2 (count (task-info "2"))))
(check-distribution (executor-info "2") [1 1])

Expand Down Expand Up @@ -375,13 +369,13 @@
(is (thrown? AlreadyAliveException (submit-local-topology (:nimbus cluster) "2test" {} topology)))
(advance-cluster-time cluster 5)
(is (= 1 (count (.heartbeat-storms state))))

(advance-cluster-time cluster 6)
(is (nil? (.storm-base state storm-id nil)))
(is (nil? (.assignment-info state storm-id nil)))
(advance-cluster-time cluster 11)
(is (= 0 (count (.heartbeat-storms state))))

(submit-local-topology (:nimbus cluster) "test3" {TOPOLOGY-MESSAGE-TIMEOUT-SECS 5} topology)
(bind storm-id3 (get-storm-id state "test3"))
(advance-cluster-time cluster 1)
Expand All @@ -395,12 +389,12 @@
;; this guarantees that monitor thread won't trigger for 10 more seconds
(advance-time-secs! 11)
(wait-until-cluster-waiting cluster)

(submit-local-topology (:nimbus cluster) "test3" {TOPOLOGY-MESSAGE-TIMEOUT-SECS 5} topology)
(bind storm-id3 (get-storm-id state "test3"))

(bind executor-id (first (topology-executors cluster storm-id3)))

(do-executor-heartbeat cluster storm-id3 executor-id)

(.killTopology (:nimbus cluster) "test3")
Expand All @@ -416,7 +410,7 @@
(advance-cluster-time cluster 9)
(is (not-nil? (.assignment-info state storm-id4 nil)))
(advance-cluster-time cluster 2)
(is (nil? (.assignment-info state storm-id4 nil)))
(is (nil? (.assignment-info state storm-id4 nil)))
)))

(deftest test-reassignment
Expand All @@ -440,7 +434,7 @@
(bind [executor-id1 executor-id2] (topology-executors cluster storm-id))
(bind ass1 (executor-assignment cluster storm-id executor-id1))
(bind ass2 (executor-assignment cluster storm-id executor-id2))

(advance-cluster-time cluster 59)
(do-executor-heartbeat cluster storm-id executor-id1)
(do-executor-heartbeat cluster storm-id executor-id2)
Expand All @@ -461,7 +455,7 @@
(do-executor-heartbeat cluster storm-id executor-id1)
(is (= ass1 (executor-assignment cluster storm-id executor-id1)))
(check-consistency cluster "test")

(advance-cluster-time cluster 11)
(is (= ass1 (executor-assignment cluster storm-id executor-id1)))
(is (not= ass2 (executor-assignment cluster storm-id executor-id2)))
Expand Down Expand Up @@ -537,7 +531,7 @@
(bind [executor-id1 executor-id2] (topology-executors cluster storm-id))
(bind ass1 (executor-assignment cluster storm-id executor-id1))
(bind ass2 (executor-assignment cluster storm-id executor-id2))

(advance-cluster-time cluster 59)
(do-executor-heartbeat cluster storm-id executor-id1)
(do-executor-heartbeat cluster storm-id executor-id2)
Expand Down Expand Up @@ -609,7 +603,7 @@
(bind common (first (find-first (fn [[k v]] (= 3 (count v))) slot-executors2)))
(is (not-nil? common))
(is (= (slot-executors2 common) (slot-executors common)))

;; check that start times are changed for everything but the common one
(bind same-executors (slot-executors2 common))
(bind changed-executors (apply concat (vals (dissoc slot-executors2 common))))
Expand Down Expand Up @@ -655,7 +649,7 @@
(bind slot-executors (slot-assignments cluster storm-id))
(check-executor-distribution slot-executors [1 1 1])
(check-num-nodes slot-executors 3)

(is (thrown? InvalidTopologyException
(.rebalance (:nimbus cluster) "test"
(doto (RebalanceOptions.)
Expand Down Expand Up @@ -685,7 +679,7 @@
(slot-assignments cluster storm-id)
distribution)))
(checker [2 2 2])

(.rebalance (:nimbus cluster) "test"
(doto (RebalanceOptions.)
(.set_num_workers 6)
Expand All @@ -694,7 +688,7 @@
(checker [2 2 2])
(advance-cluster-time cluster 3)
(checker [1 1 1 1 1 1])

(.rebalance (:nimbus cluster) "test"
(doto (RebalanceOptions.)
(.set_num_executors {"1" 1})
Expand All @@ -712,11 +706,11 @@
(advance-cluster-time cluster 32)
(checker [2 2 2 2])
(check-consistency cluster "test")

(bind executor-info (->> (storm-component->executor-info cluster "test")
(map-val #(map executor-id->tasks %))))
(check-distribution (executor-info "1") [2 2 2 2 1 1 1 1])

)))

(deftest test-submit-invalid
Expand All @@ -729,13 +723,13 @@
(bind topology (thrift/mk-topology
{"1" (thrift/mk-spout-spec (TestPlannerSpout. true) :parallelism-hint 0 :conf {TOPOLOGY-TASKS 1})}
{}))

(is (thrown? InvalidTopologyException
(submit-local-topology (:nimbus cluster)
"test"
{}
topology)))

(bind topology (thrift/mk-topology
{"1" (thrift/mk-spout-spec (TestPlannerSpout. true) :parallelism-hint 1 :conf {TOPOLOGY-TASKS 1})}
{}))
Expand All @@ -753,7 +747,7 @@
(is (thrown? InvalidTopologyException
(submit-local-topology (:nimbus cluster)
"test"
{TOPOLOGY-WORKERS 3}
{TOPOLOGY-WORKERS 3}
topology)))
(bind topology (thrift/mk-topology
{"1" (thrift/mk-spout-spec (TestPlannerSpout. true)
Expand Down Expand Up @@ -838,32 +832,32 @@
))))

(deftest test-nimbus-iface-submitTopologyWithOpts-checks-authorization
(with-local-cluster [cluster
:daemon-conf {NIMBUS-AUTHORIZER
(with-local-cluster [cluster
:daemon-conf {NIMBUS-AUTHORIZER
"backtype.storm.security.auth.authorizer.DenyAuthorizer"}]
(let [
nimbus (:nimbus cluster)
topology (thrift/mk-topology {} {})
]
(is (thrown? AuthorizationException
(submit-local-topology-with-opts nimbus "mystorm" {} topology
(submit-local-topology-with-opts nimbus "mystorm" {} topology
(SubmitOptions. TopologyInitialStatus/INACTIVE))
))
)
)
)

(deftest test-nimbus-iface-methods-check-authorization
(with-local-cluster [cluster
:daemon-conf {NIMBUS-AUTHORIZER
(with-local-cluster [cluster
:daemon-conf {NIMBUS-AUTHORIZER
"backtype.storm.security.auth.authorizer.DenyAuthorizer"}]
(let [
nimbus (:nimbus cluster)
topology (thrift/mk-topology {} {})
]
; Fake good authorization as part of setup.
(mocking [nimbus/check-authorization!]
(submit-local-topology-with-opts nimbus "test" {} topology
(submit-local-topology-with-opts nimbus "test" {} topology
(SubmitOptions. TopologyInitialStatus/INACTIVE))
)
(stubbing [nimbus/storm-active? true]
Expand Down Expand Up @@ -939,16 +933,16 @@
(stubbing [topology-bases bogus-bases]
(let [topos (.get_topologies (.getClusterInfo nimbus))]
; The number of topologies in the summary is correct.
(is (= (count
(is (= (count
(filter (fn [b] (second b)) bogus-bases)) (count topos)))
; Each topology present has a valid name.
(is (empty?
(filter (fn [t] (or (nil? t) (nil? (.get_name t)))) topos)))
; The topologies are those with valid bases.
(is (empty?
(filter (fn [t]
(or
(nil? t)
(filter (fn [t]
(or
(nil? t)
(not (number? (read-string (.get_id t))))
(odd? (read-string (.get_id t)))
)) topos)))
Expand Down

0 comments on commit 935e295

Please sign in to comment.