Merge pull request #8388 from rabbitmq/rabbitmq-server-8372

michaelklishin · web-flow · commit 60e074d78903 · 2023-05-29T01:11:10.000+04:00
Fail boot if definition file is invalid JSON
diff --git a/deps/rabbit/src/rabbit_definitions.erl b/deps/rabbit/src/rabbit_definitions.erl
@@ -44,7 +44,8 @@
 -export([import_raw/1, import_raw/2, import_parsed/1, import_parsed/2,
          import_parsed_with_hashing/1, import_parsed_with_hashing/2,
          apply_defs/2, apply_defs/3,
-         should_skip_if_unchanged/0]).
+         should_skip_if_unchanged/0
+        ]).
 
 -export([all_definitions/0]).
 -export([
@@ -53,7 +54,13 @@
   list_exchanges/0, list_queues/0, list_bindings/0,
   is_internal_parameter/1
 ]).
--export([decode/1, decode/2, args/1]).
+-export([decode/1, decode/2, args/1, validate_definitions/1]).
+
+%% for tests
+-export([
+    maybe_load_definitions_from_local_filesystem_if_unchanged/3,
+    maybe_load_definitions_from_pluggable_source_if_unchanged/2
+]).
 
 -import(rabbit_misc, [pget/2, pget/3]).
 -import(rabbit_data_coercion, [to_binary/1]).
@@ -97,6 +104,21 @@ maybe_load_definitions() ->
         {error, E} -> {error, E}
     end.
 
+validate_definitions(Defs) when is_list(Defs) ->
+    lists:foldl(fun(_Body, false) ->
+                     false;
+                   (Body, true) ->
+                       case decode(Body) of
+                           {ok, _Map}    -> true;
+                           {error, _Err} -> false
+                       end
+                end, true, Defs);
+validate_definitions(Body) when is_binary(Body) ->
+    case decode(Body) of
+        {ok, _Map}    -> true;
+        {error, _Err} -> false
+    end.
+
 -spec import_raw(Body :: binary() | iolist()) -> ok | {error, term()}.
 import_raw(Body) ->
     rabbit_log:info("Asked to import definitions. Acting user: ~ts", [?INTERNAL_USER]),
@@ -271,20 +293,25 @@ maybe_load_definitions_from_local_filesystem(App, Key) ->
                     rabbit_log:debug("Will re-import definitions even if they have not changed"),
                     Mod:load(IsDir, Path);
                 true ->
-                    Algo = rabbit_definitions_hashing:hashing_algorithm(),
-                    rabbit_log:debug("Will import definitions only if definition file/directory has changed, hashing algo: ~ts", [Algo]),
-                    CurrentHash = rabbit_definitions_hashing:stored_global_hash(),
-                    rabbit_log:debug("Previously stored hash value of imported definitions: ~ts...", [binary:part(rabbit_misc:hexify(CurrentHash), 0, 12)]),
-                    case Mod:load_with_hashing(IsDir, Path, CurrentHash, Algo) of
-                        CurrentHash ->
-                            rabbit_log:info("Hash value of imported definitions matches current contents");
-                        UpdatedHash ->
-                            rabbit_log:debug("Hash value of imported definitions has changed to ~ts", [binary:part(rabbit_misc:hexify(UpdatedHash), 0, 12)]),
-                            rabbit_definitions_hashing:store_global_hash(UpdatedHash)
-                    end
+                    maybe_load_definitions_from_local_filesystem_if_unchanged(Mod, IsDir, Path)
             end
     end.
 
+maybe_load_definitions_from_local_filesystem_if_unchanged(Mod, IsDir, Path) ->
+    Algo = rabbit_definitions_hashing:hashing_algorithm(),
+    rabbit_log:debug("Will import definitions only if definition file/directory has changed, hashing algo: ~ts", [Algo]),
+    CurrentHash = rabbit_definitions_hashing:stored_global_hash(),
+    rabbit_log:debug("Previously stored hash value of imported definitions: ~ts...", [binary:part(rabbit_misc:hexify(CurrentHash), 0, 12)]),
+    case Mod:load_with_hashing(IsDir, Path, CurrentHash, Algo) of
+        {error, Err} ->
+            {error, Err};
+        CurrentHash ->
+            rabbit_log:info("Hash value of imported definitions matches current contents");
+        UpdatedHash ->
+            rabbit_log:debug("Hash value of imported definitions has changed to ~ts", [binary:part(rabbit_misc:hexify(UpdatedHash), 0, 12)]),
+            rabbit_definitions_hashing:store_global_hash(UpdatedHash)
+    end.
+
 maybe_load_definitions_from_pluggable_source(App, Key) ->
     case application:get_env(App, Key) of
         undefined  -> ok;
@@ -296,26 +323,33 @@ maybe_load_definitions_from_pluggable_source(App, Key) ->
                     {error, "definition import source is configured but definitions.import_backend is not set"};
                 ModOrAlias ->
                     Mod = normalize_backend_module(ModOrAlias),
-                    case should_skip_if_unchanged() of
-                        false ->
-                            rabbit_log:debug("Will use module ~ts to import definitions", [Mod]),
-                            Mod:load(Proplist);
-                        true ->
-                            rabbit_log:debug("Will use module ~ts to import definitions (if definition file/directory/source has changed)", [Mod]),
-                            CurrentHash = rabbit_definitions_hashing:stored_global_hash(),
-                            rabbit_log:debug("Previously stored hash value of imported definitions: ~ts...", [binary:part(rabbit_misc:hexify(CurrentHash), 0, 12)]),
-                            Algo = rabbit_definitions_hashing:hashing_algorithm(),
-                            case Mod:load_with_hashing(Proplist, CurrentHash, Algo) of
-                                CurrentHash ->
-                                    rabbit_log:info("Hash value of imported definitions matches current contents");
-                                UpdatedHash ->
-                                    rabbit_log:debug("Hash value of imported definitions has changed to ~ts...", [binary:part(rabbit_misc:hexify(CurrentHash), 0, 12)]),
-                                    rabbit_definitions_hashing:store_global_hash(UpdatedHash)
-                            end
-                    end
+                    maybe_load_definitions_from_pluggable_source_if_unchanged(Mod, Proplist)
             end
     end.
 
+maybe_load_definitions_from_pluggable_source_if_unchanged(Mod, Proplist) ->
+    case should_skip_if_unchanged() of
+        false ->
+            rabbit_log:debug("Will use module ~ts to import definitions", [Mod]),
+            Mod:load(Proplist);
+        true ->
+            rabbit_log:debug("Will use module ~ts to import definitions (if definition file/directory/source has changed)", [Mod]),
+            CurrentHash = rabbit_definitions_hashing:stored_global_hash(),
+            rabbit_log:debug("Previously stored hash value of imported definitions: ~ts...", [binary:part(rabbit_misc:hexify(CurrentHash), 0, 12)]),
+            Algo = rabbit_definitions_hashing:hashing_algorithm(),
+            case Mod:load_with_hashing(Proplist, CurrentHash, Algo) of
+                {error, Err} ->
+                    {error, Err};
+                CurrentHash ->
+                    rabbit_log:info("Hash value of imported definitions matches current contents");
+                UpdatedHash ->
+                    rabbit_log:debug("Hash value of imported definitions has changed to ~ts...", [binary:part(rabbit_misc:hexify(CurrentHash), 0, 12)]),
+                    rabbit_definitions_hashing:store_global_hash(UpdatedHash)
+            end
+    end.
+
+
+
 normalize_backend_module(local_filesystem) ->
     rabbit_definitions_import_local_filesystem;
 normalize_backend_module(local) ->
diff --git a/deps/rabbit/src/rabbit_definitions_import_local_filesystem.erl b/deps/rabbit/src/rabbit_definitions_import_local_filesystem.erl
@@ -35,7 +35,7 @@
 
 -import(rabbit_misc, [pget/2, pget/3]).
 -import(rabbit_data_coercion, [to_binary/1]).
--import(rabbit_definitions, [import_raw/1]).
+-import(rabbit_definitions, [import_raw/1, decode/1, validate_definitions/1]).
 
 %%
 %% API
@@ -84,12 +84,18 @@ load_with_hashing(IsDir, Path, PreviousHash, Algo) when is_boolean(IsDir) ->
         [] ->
             rabbit_definitions_hashing:hash(Algo, undefined);
         Defs ->
-            case rabbit_definitions_hashing:hash(Algo, Defs) of
-                PreviousHash -> PreviousHash;
-                Other        ->
-                    rabbit_log:debug("New hash: ~ts", [rabbit_misc:hexify(Other)]),
-                    _ = load_from_local_path(IsDir, Path),
-                    Other
+            case validate_definitions(Defs) of
+                true ->
+                    case rabbit_definitions_hashing:hash(Algo, Defs) of
+                        PreviousHash -> PreviousHash;
+                        Other        ->
+                            rabbit_log:debug("New hash: ~ts", [rabbit_misc:hexify(Other)]),
+                            _ = load_from_local_path(IsDir, Path),
+                            Other
+                    end;
+                false ->
+                    rabbit_log:error("Failed to parse a definition file, path: ~p", [Path]),
+                    {error, not_json}
             end
     end.
 
diff --git a/deps/rabbit/test/definition_import_SUITE.erl b/deps/rabbit/test/definition_import_SUITE.erl
@@ -51,7 +51,8 @@ groups() ->
                                import_case16,
                                import_case17,
                                import_case18,
-                               import_case19
+                               import_case19,
+                               import_case20
                               ]},
         
         {boot_time_import_using_classic_source, [], [
@@ -295,6 +296,15 @@ import_case19(Config) ->
             {skip, "Should not run in mixed version environments"}
     end.
 
+import_case20(Config) ->
+    case rabbit_ct_helpers:is_mixed_versions() of
+        false ->
+            import_invalid_file_case_if_unchanged(Config, "failing_case20");
+        true ->
+            %% skip the test in mixed version mode
+            {skip, "Should not run in mixed version environments"}
+    end.
+
 export_import_round_trip_case1(Config) ->
     case rabbit_ct_helpers:is_mixed_versions() of
       false ->
@@ -379,6 +389,11 @@ import_invalid_file_case(Config, CaseName) ->
     rabbit_ct_broker_helpers:rpc(Config, 0, ?MODULE, run_invalid_import_case, [CasePath]),
     ok.
 
+import_invalid_file_case_if_unchanged(Config, CaseName) ->
+    CasePath = filename:join(?config(data_dir, Config), CaseName ++ ".json"),
+    rabbit_ct_broker_helpers:rpc(Config, 0, ?MODULE, run_invalid_import_case_if_unchanged, [CasePath]),
+    ok.
+
 import_from_directory_case(Config, CaseName) ->
     import_from_directory_case_expect(Config, CaseName, ok).
 
@@ -398,15 +413,15 @@ import_raw(Config, Body) ->
         ok -> ok;
         {error, E} ->
             ct:pal("Import of JSON definitions ~tp failed: ~tp~n", [Body, E]),
-            ct:fail({failure, Body, E})
+            ct:fail({expected_failure, Body, E})
     end.
 
 import_parsed(Config, Body) ->
     case rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_definitions, import_parsed, [Body]) of
         ok -> ok;
         {error, E} ->
             ct:pal("Import of parsed definitions ~tp failed: ~tp~n", [Body, E]),
-            ct:fail({failure, Body, E})
+            ct:fail({expected_failure, Body, E})
     end.
 
 export(Config) ->
@@ -432,19 +447,30 @@ run_import_case(Path) ->
      ok -> ok;
      {error, E} ->
        ct:pal("Import case ~tp failed: ~tp~n", [Path, E]),
-       ct:fail({failure, Path, E})
+       ct:fail({expected_failure, Path, E})
    end.
 
 run_invalid_import_case(Path) ->
    {ok, Body} = file:read_file(Path),
-   ct:pal("Successfully loaded a definition to import from ~tp~n", [Path]),
+   ct:pal("Successfully loaded a definition file at ~tp~n", [Path]),
    case rabbit_definitions:import_raw(Body) of
      ok ->
        ct:pal("Expected import case ~tp to fail~n", [Path]),
-       ct:fail({failure, Path});
+       ct:fail({expected_failure, Path});
      {error, _E} -> ok
    end.
 
+run_invalid_import_case_if_unchanged(Path) ->
+    Mod = rabbit_definitions_import_local_filesystem,
+    ct:pal("Successfully loaded a definition to import from ~tp~n", [Path]),
+    case rabbit_definitions:maybe_load_definitions_from_local_filesystem_if_unchanged(Mod, false, Path) of
+        ok ->
+            ct:pal("Expected import case ~tp to fail~n", [Path]),
+            ct:fail({expected_failure, Path});
+        {error, _E} -> ok
+    end.
+
+
 queue_lookup(Config, VHost, Name) ->
     rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_amqqueue, lookup, [rabbit_misc:r(VHost, queue, Name)]).
 
diff --git a/deps/rabbit/test/definition_import_SUITE_data/failing_case20.json b/deps/rabbit/test/definition_import_SUITE_data/failing_case20.json
@@ -0,0 +1,46 @@
+{
+  "bindings": [],
+  "exchanges": [],
+  "global_parameters": [
+    {
+      "name": "cluster_name"""""{{{,
+      "value": "rabbitmq@localhost",{{{{
+    }
+  ],
+  "parameters": [],
+  "permissions": [
+    {
+      "configure": ".*",
+      "read": ".*",
+      "user": "guest",
+      "vhost": "/",
+      "write"$ ".*"
+    }
+  ],
+  "policies": [],
+  "queues": [],
+  "rabbit_version": "3.9.1",
+  "rabbitmq_version": "3.9.1",
+  "topic_permissions": [],
+  "users": [
+    {
+      "hashing_algorithm": "rabbit_password_hashing_sha256",
+      "limits": {"max-connections" : 2},
+      "name": "limited_guest",
+      "password_hash": "wS4AT3B4Z5RpWlFn1FA30osf2C75D7WA3gem591ACDZ6saO6",
+      "tags": [
+        "administrator"
+      ]
+    }
+  ],
+  "vhosts": [
+    {
+      "limits": [],
+      "name": "/"
+    },
+    {
+      "limits": [],
+      "name": "tagged"
+    }
+  ]
+}
