-
Notifications
You must be signed in to change notification settings - Fork 33
ssl: check no sensitive ssl data in logs #599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
35c48fd
to
6fdf404
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the patch! Great work, just some nits
P.S. I checked the tests manually, they seem ok, not flaky on my laptop. We don't have CI for EE, and I'm not sure, we can do that politically
3c0d105
to
f6149a6
Compare
f6149a6
to
22db719
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch already looks great and I don't have any new comments regarding the code, let's just properly split the commits and call it a day. Good work
02fb16a
to
5241202
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the patches. Looks good!
5241202
to
f1a5e46
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 💪! Top work 🔥! I like how you've used this ticket as a chance to do a bit of cleanup around the related code.
t.run_only_if(vutil.feature.ssl) | ||
vtest.cluster_new(g, global_cfg) | ||
vtest.cluster_bootstrap(g, global_cfg) | ||
vtest.cluster_rebalancer_disable(g) | ||
|
||
local new_cfg_template = table.deepcopy(cfg_template) | ||
new_cfg_template.sharding[1].is_ssl = true | ||
local new_global_cfg = vtest.config_new(new_cfg_template) | ||
|
||
g.router = vtest.router_new(g, 'router', new_global_cfg) | ||
vtest.cluster_cfg(g, new_global_cfg) | ||
vtest.router_cfg(g.router, new_global_cfg) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to start it without SSL and then configure it with SSL right away? Can you update the original cfg_template
to simply already contain all the SSL params?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that we cannot simply pass is_ssl = true
into cfg_template
. The storages cannot be configured correctly.
Part of the test:
local g = t.group('router_ssl')
local cfg_template = {
sharding = {
{
replicas = {
replica_1_a = {
master = true,
},
replica_1_b = {},
},
is_ssl = true,
},
{
replicas = {
replica_2_a = {
master = true,
},
replica_2_b = {},
},
is_ssl = true,
},
},
bucket_count = 100,
}
local global_cfg = vtest.config_new(cfg_template)
g.before_all(function()
t.run_only_if(vutil.feature.ssl)
vtest.cluster_new(g, global_cfg)
vtest.cluster_bootstrap(g, global_cfg)
vtest.cluster_rebalancer_disable(g)
g.router = vtest.router_new(g, 'router', global_cfg)
vtest.router_cfg(g.router, global_cfg)
vtest.cluster_wait_fullsync(g)
end)
The logs:
replica_1_b | Incorrect value for option 'replication': Invalid URI table: expected {uri = string, params = table} or {string, params = table} {"type":"ClientError","code":59,"name":"CFG","o
ption":"replication","details":"Invalid URI table: expected {uri = string, params = table} or {string, params = table}","trace":[{"file":"[C]","line":4294967295}]}
replica_1_b | fatal error, exiting the event loop
replica_1_a | Incorrect value for option 'replication': Invalid URI table: expected {uri = string, params = table} or {string, params = table} {"type":"ClientError","code":59,"name":"CFG","o
ption":"replication","details":"Invalid URI table: expected {uri = string, params = table} or {string, params = table}","trace":[{"file":"[C]","line":4294967295}]}
replica_1_a | fatal error, exiting the event loop
replica_2_b | Incorrect value for option 'replication': Invalid URI table: expected {uri = string, params = table} or {string, params = table} {"type":"ClientError","code":59,"name":"CFG","o
ption":"replication","details":"Invalid URI table: expected {uri = string, params = table} or {string, params = table}","trace":[{"file":"[C]","line":4294967295}]}
replica_2_b | fatal error, exiting the event loop
replica_2_a | Incorrect value for option 'replication': Invalid URI table: expected {uri = string, params = table} or {string, params = table} {"type":"ClientError","code":59,"name":"CFG","o
ption":"replication","details":"Invalid URI table: expected {uri = string, params = table} or {string, params = table}","trace":[{"file":"[C]","line":4294967295}]}
replica_2_a | fatal error, exiting the event loop
not ok 1 router_ssl.test_no_ssl_sensitive_data_in_storage_logs
# fiber is cancelled
# stack traceback:
# ...e/mrforza/Desktop/vshard/test/luatest_helpers/server.lua:107: in function 'wait_for_readiness'
# /home/mrforza/Desktop/vshard/test/luatest_helpers/vtest.lua:228: in function 'cluster_new'
# ...sktop/vshard/test/router-luatest/router_2_2_ssl_test.lua:34: in function <...sktop/vshard/test/router-luatest/router_2_2_ssl_test.lua:32>
# ...
# [C]: in function 'xpcall'
# Ran 1 tests in 15.300 seconds, 0 succeeded, 1 errored
AFAIU, we firstly should configure the cluster without ssl and then reconfigure it with ssl options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you investigate that? That doesn't look right. Probably something simple is missing in vtest.
Before this patch outdating the replicasets led to broken replicas in logs. It happened because the outdated replicaset and replica didn't have the correct tostring method. As a result, all replica's json table is printed into logs which means that some sensitive data could leak into it (e.g. ssl data, uri password and e.t.c.). Now, we fix this issue by introducing custom tostring functions for replicasets and replicas. Closes tarantool#597 Needed for tarantool#593 NO_DOC=bugfix
This patch adds `(outdated)` prefix into tostring of outdated replicasets and replicas in order to inform user about outdatedness. Also we change the format of replica's tostring in order for it be consistent with replicaset tostring. Since the `failover/failover.test.lua` and `misc/check_uuid_on_connect.test.lua` tests didn't expect another format of replica's tostring, we accordingly changed them. NO_DOC=minor
f1a5e46
to
bf615e1
Compare
The `safe_uri` function is used in vshard to get a purged uri. Before tarantool/tarantool#11810 patch sensitive ssl data may leaked into logs as `uri.format` function didn't handle ssl params of uri. As a result we don't fix this issue in vshard for those tarantool versions where `uri.format` works incorrectly, because we will certainly get data leaks in tarantool logs. This patch explicitly set `write_sensitive` option of `uri.format` into `false` in `replica_safe_uri` and add some tests which check that sensitive ssl data doesn't leak into logs of vshard storage and router. Also we move all ssl related testcases into the `router_2_2_ssl_test`. Closes tarantool#593 NO_DOC=bugfix
bf615e1
to
08a813b
Compare
ssl_password = 'P4ssw0rd', | ||
ssl_password_file = 'PATH_TO_PASSWORD_FILE', | ||
ssl_ciphers = {'TLS_AES_256_GCM_SHA384', 'TLS_RSA_POLY1305_SHA256'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are these 3 keys coming from?
ssl_cert_file = fio.pathjoin(cert_dir, 'server.crt'), | ||
ssl_key_file = fio.pathjoin(cert_dir, 'server.key'), | ||
ssl_ca_file = fio.pathjoin(cert_dir, 'ca.crt'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you not hardcode these values and instead reuse them being returned from vtest? From what I see, config_new()
returns the replica objects with these parameters available in replica.listen
tables. Alternatively they could be just exported from vtest explicitly.
Their hardcoding here leads to a problem that if I stop using these files in vtest in the future, your test will still pass. But already regardless of whether SSL data is actually printed. Because you are only looking for specifically these hardcoded values.
The
safe_uri
function is used in vshard to get a purged uri. Before #11810 patch sensitive ssl data may leaked into logs asuri.format
function didn't handle ssl params of uri.This patch explicitly sets
write_sensitive
parameter ofuri.format
intofalse
insafe_uri
function and add some tests which check that sensitive ssl data doesn't leak into logs of vshard storage and router.NO_DOC=bugfix
Closes #593
Closes #597