Skip to content

Commit 547e2d2

Browse files
StekPerepolnennfrmtk
StekPerepolnen
and
nfrmtk
authored
EXT-1523 Write audit records always except for denylist (#25387) (#25642)
Co-authored-by: nfrmtk <[email protected]>
1 parent 8e18d44 commit 547e2d2

File tree

9 files changed

+116
-135
lines changed

9 files changed

+116
-135
lines changed

‎ydb/core/mon/audit/audit.cpp‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
#include "audit.h"
2-
#include "auditable_actions.cpp"
2+
#include "audit_denylist.cpp"
33

44
#include <ydb/core/audit/audit_log.h>
55
#include <ydb/core/audit/audit_config/audit_config.h>
@@ -50,9 +50,9 @@ namespace {
5050
return reason;
5151
}
5252

53-
inline TUrlMatcher CreateAuditableActionsMatcher() {
53+
inline TUrlMatcher CreateDenylistMatcher() {
5454
TUrlMatcher policy;
55-
for (const auto& pattern : AUDITABLE_ACTIONS) {
55+
for (const auto& pattern : AUDIT_DENYLIST) {
5656
policy.AddPattern(pattern);
5757
}
5858
return policy;
@@ -73,8 +73,8 @@ void TAuditCtx::AddAuditLogPart(TStringBuf name, const TString& value) {
7373
Parts.emplace_back(name, value);
7474
}
7575

76-
bool TAuditCtx::AuditableRequest(const NHttp::THttpIncomingRequestPtr& request) {
77-
// only modifying methods are audited
76+
bool TAuditCtx::AuditableRequest(const NHttp::THttpIncomingRequestPtr& request) const {
77+
// modifying methods are always audited
7878
const TString method(request->Method);
7979
static const THashSet<TString> MODIFYING_METHODS = {"POST", "PUT", "DELETE"};
8080
if (MODIFYING_METHODS.contains(method)) {
@@ -86,13 +86,13 @@ bool TAuditCtx::AuditableRequest(const NHttp::THttpIncomingRequestPtr& request)
8686
return false;
8787
}
8888

89-
// force audit for specific URLs
90-
static auto FORCE_AUDIT_MATCHER = CreateAuditableActionsMatcher();
91-
if (FORCE_AUDIT_MATCHER.Match(request->URL)) {
92-
return true;
89+
// skip audit for URLs from denylist
90+
static auto DENYLIST_MATCHER = CreateDenylistMatcher();
91+
if (DENYLIST_MATCHER.Match(request->URL)) {
92+
return false;
9393
}
9494

95-
return false;
95+
return true;
9696
}
9797

9898
void TAuditCtx::InitAudit(const NHttp::TEvHttpProxy::TEvHttpIncomingRequest::TPtr& ev) {

‎ydb/core/mon/audit/audit.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,10 @@ class TAuditCtx {
2727
void LogOnCompleted(const NHttp::THttpOutgoingResponsePtr& response);
2828
void SetSubjectType(NACLibProto::ESubjectType subjectType);
2929
static bool AuditEnabled(NKikimrConfig::TAuditConfig::TLogClassConfig::ELogPhase logPhase, NACLibProto::ESubjectType subjectType);
30+
bool AuditableRequest(const NHttp::THttpIncomingRequestPtr& request) const;
3031

3132
private:
3233
void AddAuditLogPart(TStringBuf name, const TString& value);
33-
bool AuditableRequest(const NHttp::THttpIncomingRequestPtr& request);
3434

3535
TAuditParts Parts;
3636
bool Auditable = false;

‎ydb/core/mon/audit/audit_denylist.cpp‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
#include "url_matcher.h"
2+
3+
#include <util/generic/string.h>
4+
#include <util/generic/vector.h>
5+
6+
namespace NMonitoring::NAudit {
7+
8+
// Audit logging is enabled for all requests that either
9+
// 1) use modifying HTTP methods (POST, PUT, DELETE), or
10+
// 2) target endpoints not listed in AUDIT_DENYLIST.
11+
// The denylist excludes frequently queried read-only endpoints with no security value.
12+
const TVector<TUrlPattern> AUDIT_DENYLIST = {
13+
{ .Path = "/" },
14+
{ .Path = "/internal"},
15+
{ .Path = "/ver"},
16+
{ .Path = "/counters", .Recursive = true },
17+
18+
// viewer endpoints
19+
{ .Path = "/viewer", .Recursive = true },
20+
{ .Path = "/vdisk", .Recursive = true },
21+
{ .Path = "/pdisk", .Recursive = true },
22+
{ .Path = "/monitoring", .Recursive = true },
23+
{ .Path = "/healthcheck", .Recursive = true },
24+
{ .Path = "/operation", .Recursive = true },
25+
{ .Path = "/query", .Recursive = true },
26+
{ .Path = "/scheme", .Recursive = true },
27+
{ .Path = "/storage", .Recursive = true },
28+
29+
// static resources such as JS, CSS, images
30+
{ .Path = "/static", .Recursive = true },
31+
{ .Path = "/jquery.tablesorter.js" },
32+
{ .Path = "/jquery.tablesorter.css" },
33+
{ .Path = "/lwtrace/mon/static", .Recursive = true },
34+
};
35+
36+
}

‎ydb/core/mon/audit/audit_ut.cpp‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,57 @@
55

66
using namespace NMonitoring::NAudit;
77

8+
namespace {
9+
10+
struct TRequestHolder : public NHttp::THttpIncomingRequest {
11+
TStringBuf Store(TString value) {
12+
return Storage.emplace_back(std::move(value));
13+
}
14+
private:
15+
TVector<TString> Storage;
16+
};
17+
18+
NHttp::THttpIncomingRequestPtr MakeRequest(TString method, TString url) {
19+
auto request = MakeIntrusive<TRequestHolder>();
20+
request->Method = request->Store(std::move(method));
21+
request->URL = request->Store(std::move(url));
22+
return std::move(request);
23+
}
24+
25+
} // namespace
26+
827
Y_UNIT_TEST_SUITE(TAuditTest) {
928
Y_UNIT_TEST(AuditDisabledWithoutAppData) {
1029
UNIT_ASSERT(!TAuditCtx::AuditEnabled(NKikimrConfig::TAuditConfig::TLogClassConfig::Completed, NACLibProto::SUBJECT_TYPE_ANONYMOUS));
1130
}
31+
32+
Y_UNIT_TEST(ModifyingMethodsAlwaysAuditable) {
33+
TAuditCtx ctx;
34+
UNIT_ASSERT(ctx.AuditableRequest(MakeRequest("POST", "/path")));
35+
UNIT_ASSERT(ctx.AuditableRequest(MakeRequest("PUT", "/path")));
36+
UNIT_ASSERT(ctx.AuditableRequest(MakeRequest("DELETE", "/path")));
37+
38+
UNIT_ASSERT(ctx.AuditableRequest(MakeRequest("POST", "/counters")));
39+
UNIT_ASSERT(ctx.AuditableRequest(MakeRequest("PUT", "/counters")));
40+
UNIT_ASSERT(ctx.AuditableRequest(MakeRequest("DELETE", "/counters")));
41+
}
42+
43+
Y_UNIT_TEST(OptionsRequestsAreNotAudited) {
44+
TAuditCtx ctx;
45+
UNIT_ASSERT(!ctx.AuditableRequest(MakeRequest("OPTIONS", "/path")));
46+
}
47+
48+
Y_UNIT_TEST(BlacklistedPathsAreNotAudited) {
49+
TAuditCtx ctx;
50+
UNIT_ASSERT(!ctx.AuditableRequest(MakeRequest("GET", "/counters")));
51+
UNIT_ASSERT(!ctx.AuditableRequest(MakeRequest("GET", "/viewer/subpage")));
52+
UNIT_ASSERT(!ctx.AuditableRequest(MakeRequest("GET", "/viewer?mode=overview")));
53+
UNIT_ASSERT(!ctx.AuditableRequest(MakeRequest("GET", "/monitoring/cluster/static/js/24615.12b53f26.chunk.js")));
54+
}
55+
56+
Y_UNIT_TEST(OtherGetRequestsAreAudited) {
57+
TAuditCtx ctx;
58+
UNIT_ASSERT(ctx.AuditableRequest(MakeRequest("GET", "/other")));
59+
UNIT_ASSERT(ctx.AuditableRequest(MakeRequest("GET", "/viewerstats?mode=overview")));
60+
}
1261
}

‎ydb/core/mon/audit/auditable_actions.cpp‎

Lines changed: 0 additions & 52 deletions
This file was deleted.

‎ydb/core/mon/audit/url_matcher.cpp‎

Lines changed: 7 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,6 @@
33

44
namespace NMonitoring::NAudit {
55

6-
const TString ANY_PATH = "*";
7-
86
void TUrlMatcher::AddPattern(const TUrlPattern& pattern) {
97
TStringBuf path = pattern.Path;
108
TNode* node = &Root;
@@ -18,17 +16,11 @@ void TUrlMatcher::AddPattern(const TUrlPattern& pattern) {
1816
node = &node->Children[part];
1917
}
2018

21-
if (pattern.ParamName.empty()) {
22-
node->MatchWithoutParams = true;
23-
} else {
24-
TParamCondition param;
25-
param.Name = pattern.ParamName;
26-
param.ExpectedValue = pattern.ParamValue;
27-
node->MatchedParams.push_back(std::move(param));
28-
}
19+
node->PatternEnd = true;
20+
node->Recursive = pattern.Recursive;
2921
}
3022

31-
bool TUrlMatcher::Match(const TStringBuf originalPath, const TCgiParameters& params) const {
23+
bool TUrlMatcher::MatchPath(const TStringBuf originalPath) const {
3224
TStringBuf path(originalPath);
3325
if (path.StartsWith('/')) {
3426
path = path.Skip(1);
@@ -44,33 +36,24 @@ bool TUrlMatcher::Match(const TStringBuf originalPath, const TCgiParameters& par
4436
}
4537

4638
auto it = node->Children.find(part);
47-
if (it == node->Children.end()) {
48-
it = node->Children.find(ANY_PATH);
49-
}
5039
if (it == node->Children.end()) {
5140
return false;
5241
}
5342
node = &it->second;
54-
};
55-
56-
if (node->MatchWithoutParams) {
57-
return true;
58-
}
59-
for (const auto& p : node->MatchedParams) {
60-
if (params.Has(p.Name) && (p.ExpectedValue.empty() || params.Get(p.Name) == p.ExpectedValue)) {
43+
if (node->Recursive) {
6144
return true;
6245
}
63-
}
46+
};
6447

65-
return false;
48+
return node->PatternEnd;
6649
}
6750

6851
bool TUrlMatcher::Match(const TStringBuf url) const {
6952
const auto path = url.Before('?');
7053
const auto params = url.After('?');
7154
const auto cgiParams = TCgiParameters(params);
7255

73-
return Match(path, cgiParams);
56+
return MatchPath(path);
7457
}
7558

7659
}

‎ydb/core/mon/audit/url_matcher.h‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12,26 +12,20 @@ namespace NMonitoring::NAudit {
1212

1313
struct TUrlPattern {
1414
TString Path;
15-
TString ParamName;
16-
TString ParamValue;
15+
bool Recursive = false;
1716
};
1817

1918
class TUrlMatcher {
2019
public:
21-
void AddPattern(const TUrlPattern& rule);
22-
bool Match(const TStringBuf path, const TCgiParameters& params) const;
20+
void AddPattern(const TUrlPattern& pattern);
21+
bool MatchPath(const TStringBuf path) const;
2322
bool Match(const TStringBuf url) const;
2423

2524
private:
26-
struct TParamCondition {
27-
TString Name;
28-
TString ExpectedValue;
29-
};
30-
3125
struct TNode {
3226
THashMap<TString, TNode> Children;
33-
bool MatchWithoutParams = false;
34-
TVector<TParamCondition> MatchedParams;
27+
bool PatternEnd = false;
28+
bool Recursive = false;
3529
};
3630

3731
TNode Root;

‎ydb/core/mon/audit/url_matcher_ut.cpp‎

Lines changed: 7 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ using namespace NMonitoring::NAudit;
88
Y_UNIT_TEST_SUITE(TUrlMatcherTest) {
99
Y_UNIT_TEST(MatchExactPathOnly) {
1010
NMonitoring::NAudit::TUrlMatcher matcher;
11-
matcher.AddPattern({.Path = "/a/b/c"});
11+
matcher.AddPattern({.Path = "/a/b/c", .Recursive = false});
1212

1313
UNIT_ASSERT(matcher.Match("/a/b/c"));
1414
UNIT_ASSERT(matcher.Match("a/b/c"));
@@ -24,48 +24,19 @@ Y_UNIT_TEST_SUITE(TUrlMatcherTest) {
2424
UNIT_ASSERT(!matcher.Match("//a/b/c"));
2525
UNIT_ASSERT(!matcher.Match("/a/b///c"));
2626
UNIT_ASSERT(!matcher.Match("/a/b/c/d"));
27+
UNIT_ASSERT(!matcher.Match("/a/b/c/d/e/f"));
2728
}
2829

29-
Y_UNIT_TEST(MatchWithParamNameOnly) {
30+
Y_UNIT_TEST(MatchRecursive) {
3031
NMonitoring::NAudit::TUrlMatcher matcher;
31-
matcher.AddPattern({.Path = "/a/b", .ParamName = "mode"});
32-
33-
UNIT_ASSERT(matcher.Match("/a/b?mode="));
34-
UNIT_ASSERT(matcher.Match("/a/b?mode="));
35-
UNIT_ASSERT(matcher.Match("/a/b?mode=1"));
36-
UNIT_ASSERT(matcher.Match("/a/b?other=1&mode=1"));
37-
38-
UNIT_ASSERT(!matcher.Match("/a/b"));
39-
UNIT_ASSERT(!matcher.Match("/a?mode=1"));
40-
UNIT_ASSERT(!matcher.Match("/a/b?other=1"));
41-
UNIT_ASSERT(!matcher.Match("/a/b/c?mode=1"));
42-
}
43-
44-
Y_UNIT_TEST(MatchWithParamNameAndValue) {
45-
NMonitoring::NAudit::TUrlMatcher matcher;
46-
matcher.AddPattern({.Path = "/a/b", .ParamName = "action", .ParamValue = "start"});
47-
matcher.AddPattern({.Path = "/a/b", .ParamName = "action", .ParamValue = "stop"});
48-
49-
UNIT_ASSERT(matcher.Match("/a/b?action=start"));
50-
UNIT_ASSERT(matcher.Match("/a/b?action=stop"));
51-
UNIT_ASSERT(matcher.Match("/a/b?k=stop&action=start"));
52-
53-
UNIT_ASSERT(!matcher.Match("/a/b"));
54-
UNIT_ASSERT(!matcher.Match("/a/b?action=restart"));
55-
UNIT_ASSERT(!matcher.Match("/a/b?k=stop"));
56-
UNIT_ASSERT(!matcher.Match("/a/b/c?action=start"));
57-
}
58-
59-
Y_UNIT_TEST(MatchWithWildcardPath) {
60-
NMonitoring::NAudit::TUrlMatcher matcher;
61-
matcher.AddPattern({.Path = "/actors/blobstorageproxies/*", .ParamName = "PutSamplingRate"});
32+
matcher.AddPattern({.Path = "/actors/blobstorageproxies", .Recursive = true});
6233

34+
UNIT_ASSERT(matcher.Match("/actors/blobstorageproxies"));
35+
UNIT_ASSERT(matcher.Match("/actors/blobstorageproxies/blobstorageproxy2181038080"));
6336
UNIT_ASSERT(matcher.Match("/actors/blobstorageproxies/blobstorageproxy2181038080?PutSamplingRate=1"));
6437
UNIT_ASSERT(matcher.Match("/actors/blobstorageproxies/somethingelse?PutSamplingRate=123"));
6538

66-
UNIT_ASSERT(!matcher.Match("/actors/blobstorageproxies"));
67-
UNIT_ASSERT(!matcher.Match("/actors/blobstorageproxies/blobstorageproxy2181038080"));
68-
UNIT_ASSERT(!matcher.Match("/actors/blobstorageproxies/blobstorageproxy2181038080?OtherParam=1"));
39+
UNIT_ASSERT(!matcher.Match("/actors/blobstorageproxies123"));
6940
UNIT_ASSERT(!matcher.Match("/actors/otherproxy/blobstorageproxy2181038080?PutSamplingRate=1"));
7041
}
7142
}

‎ydb/core/mon/audit/ya.make‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ RECURSE_FOR_TESTS(
55
LIBRARY()
66

77
SRCS(
8-
auditable_actions.cpp
8+
audit_denylist.cpp
99
audit.cpp
1010
url_matcher.cpp
1111
)

0 commit comments

Comments
 (0)