Do not log sensitive data #2083

kziemianek · 2023-08-30T11:16:46Z

No sensitive data should be present in logs for production mode workers.

Removes for example Identity content from the logs for production build
Hardcodes enclave logger to INFO - it doesn't read configuration from env for production build anymore as third party libraries might log sensitive data on various levels
Changes several log statements to not log sensitive data in production build

Example of hidding Identity contents

Non Production

[2023-08-30T10:59:48Z DEBUG itp_top_pool_author::api] [Pool] creating hash of direct_call(TrustedCallSigned { call: set_user_shielding_key(Substrate(Address32([212, 53, 147, 199, 21, 253, 211, 28, 97, 20, 26, 189, 4, 169, 159, 214, 130, 44, 133, 88, 133, 76, 205, 227, 154, 86, 132, 231, 165, 109, 162, 125])), Substrate(Address32([212, 53, 147, 199, 21, 253, 211, 28, 97, 20, 26, 189, 4, 169, 159, 214, 130, 44, 133, 88, 133, 76, 205, 227, 154, 86, 132, 231, 165, 109, 162, 125])), [34, 252, 130, 219, 91, 96, 105, 152, 173, 69, 9, 155, 121, 120, 181, 180, 249, 221, 78, 166, 1, 126, 87, 55, 10, 197, 97, 65, 202, 170, 189, 18], 0xf457c8d9ec835e2ec0cbdb2c03ad77e33416ccc1e91939a3863fa57e55eb09a8), nonce: 0, signature: Sr25519() })

[2023-08-30T11:00:36Z DEBUG itp_top_pool_author::api] [Pool] creating hash of direct_call(TrustedCallSigned { call: link_identity(Substrate(Address32([212, 53, 147, 199, 21, 253, 211, 28, 97, 20, 26, 189, 4, 169, 159, 214, 130, 44, 133, 88, 133, 76, 205, 227, 154, 86, 132, 231, 165, 109, 162, 125])), Substrate(Address32([212, 53, 147, 199, 21, 253, 211, 28, 97, 20, 26, 189, 4, 169, 159, 214, 130, 44, 133, 88, 133, 76, 205, 227, 154, 86, 132, 231, 165, 109, 162, 125])), Twitter(IdentityString { inner: BoundedVec([109, 111, 99, 107, 95, 117, 115, 101, 114], 64) }), Web2(Twitter(TwitterValidationData { tweet_id: BoundedVec([49], 64) })), [], [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1], 0x4b9cf34a5837837847974a92246bbfe1f5c8ab8d36ef2d013d465ca36e168723), nonce: 1, signature: Sr25519() })

[2023-08-30T11:00:36Z DEBUG itp_top_pool_author::api] [Pool] creating hash of direct_call(TrustedCallSigned { call: link_identity_callback(Substrate(Address32([35, 112, 254, 25, 116, 130, 30, 154, 197, 190, 131, 101, 191, 1, 240, 119, 210, 117, 129, 225, 240, 223, 148, 35, 184, 162, 98, 205, 235, 187, 238, 211])), Substrate(Address32([212, 53, 147, 199, 21, 253, 211, 28, 97, 20, 26, 189, 4, 169, 159, 214, 130, 44, 133, 88, 133, 76, 205, 227, 154, 86, 132, 231, 165, 109, 162, 125])), Twitter(IdentityString { inner: BoundedVec([109, 111, 99, 107, 95, 117, 115, 101, 114], 64) }), [], 0x4b9cf34a5837837847974a92246bbfe1f5c8ab8d36ef2d013d465ca36e168723), nonce: 0, signature: Ed25519() })

Production

[2023-08-30T10:46:24Z DEBUG itp_top_pool_author::api] [Pool] creating hash of direct_call(TrustedCallSigned { call: set_user_shielding_key(Substrate(Address32), Substrate(Address32), [34, 252, 130, 219, 91, 96, 105, 152, 173, 69, 9, 155, 121, 120, 181, 180, 249, 221, 78, 166, 1, 126, 87, 55, 10, 197, 97, 65, 202, 170, 189, 18], 0x4ed294ad4725d4c3904ddf15dd7934cf0f1f6f2fd749c0c6a0600c279cb17813), nonce: 0, signature: Sr25519() })

[2023-08-30T10:46:48Z DEBUG itp_top_pool_author::api] [Pool] creating hash of direct_call(TrustedCallSigned { call: link_identity(Substrate(Address32), Substrate(Address32), Twitter(IdentityString), Web2(Twitter(TwitterValidationData { tweet_id: BoundedVec([49], 64) })), [], [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1], 0x3d8c863948ca59c3be8d2804e53c56c2e151ddec369ea4af43e4d707d489cf2f), nonce: 1, signature: Sr25519() })

[2023-08-30T10:49:00Z DEBUG itp_top_pool_author::api] [Pool] creating hash of direct_call(TrustedCallSigned { call: link_identity_callback(Substrate(Address32), Substrate(Address32), Twitter(IdentityString), [], 0x53551a9cbaa903110d2037c1b97bc0dd28d2e7a4eb1aa64e6a23814529b17f3a), nonce: 1, signature: Ed25519() })

relates: #2040
fixes: #1514

BillyWooo · 2023-09-11T06:25:14Z

Hi @kziemianek , do you think it's better to design a local wrapper of crate log ? Then only change the toml dependency and keep the coding style like, debug!(), info!(), etc. That would be much easier. Otherwise all the log info needs to add extra if_production_or or if_not_production.
My original idea would be: we can have log crate local override and switch between production and non-production mode.
How do you think so?

kziemianek · 2023-09-11T08:02:31Z

Hi @kziemianek , do you think it's better to design a local wrapper of crate log ? Then only change the toml dependency and keep the coding style like, debug!(), info!(), etc. That would be much easier. Otherwise all the log info needs to add extra if_production_or or if_not_production. My original idea would be: we can have log crate local override and switch between production and non-production mode. How do you think so?

Good idea, i can wrap it with another macro.

BillyWooo · 2023-09-11T10:03:31Z

tee-worker/core/direct-rpc-server/src/rpc_ws_handler.rs

@@ -20,8 +20,8 @@ use crate::sgx_reexport_prelude::*;

 use crate::{DetermineWatch, RpcConnectionRegistry, RpcHash};
 use itc_tls_websocket_server::{error::WebSocketResult, ConnectionToken, WebSocketMessageHandler};
+use itp_utils::debug as lit_debug;


can we just use debug, without prefix lit_ ? Will it introduce any conflict?
If no conflict, I would suggest only debug is sufficient.
so we will only change:
use log::* to use itp_utils::*

We may need to take care of sgx vs std.

Yes we can but I prefer different names from log crate macros.
What do you think about renaming our macros, example debug -> lit_debug ?

I actually prefer renaming the macros to lit_ prefixed, to clearly differ from the known log macros.

And it seems we are using alias anyway in most of the cases

@kziemianek let's renaming our macros directly to lit_***. And then merge this PR.

I'll remove them as they seems to be redundant since we locked log level for production build at INFO.

kziemianek · 2023-09-11T12:53:48Z

I set enclave logger's base filter level to INFO for production build.
It might be worth to consider disabling the logging for 3rd crates.

tee-worker/enclave-runtime/src/lib.rs

Kailai-Wang

Looks good from my side, thanks :)

Kailai-Wang · 2023-09-18T21:47:48Z

tee-worker/litentry/primitives/src/identity.rs

+
+#[derive(Eq, PartialEq, Clone, MaxEncodedLen, Default)]
+#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
+pub struct IdentityString {


Do I understand it correctly that we need such a flattened struct because of prod/non-prod Debug implementation?

Yes, the actual reason is just to not introduce any breaking changes and to not pollute api contract with this implementation detail.
This struct will be serialized/deserialized the same as it was before so for the consumers there would be no change.

Kailai-Wang · 2023-09-18T21:49:52Z

tee-worker/core/direct-rpc-server/src/rpc_ws_handler.rs

@@ -20,8 +20,8 @@ use crate::sgx_reexport_prelude::*;

 use crate::{DetermineWatch, RpcConnectionRegistry, RpcHash};
 use itc_tls_websocket_server::{error::WebSocketResult, ConnectionToken, WebSocketMessageHandler};
+use itp_utils::debug as lit_debug;


I actually prefer renaming the macros to lit_ prefixed, to clearly differ from the known log macros.

And it seems we are using alias anyway in most of the cases

kziemianek · 2023-09-19T08:45:05Z

@BillyWooo , @Kailai-Wang I've removed log macros because they seem to be redundant since we locked log level for production build at INFO.

BillyWooo · 2023-09-20T07:19:27Z

@BillyWooo , @Kailai-Wang I've removed log macros because they seem to be redundant since we locked log level for production build at INFO.

Good idea. Mindset was trapped in before.
If so, we need to apply to all of our logs. enclave_runtime and worker in general. Then we check all INFO level logs to make sure no sensitive info leak.

BillyWooo

Thanks. LGTM

Hide Identity content in logs

6f582f2

kziemianek requested a review from a team August 30, 2023 11:16

kziemianek changed the title ~~Hide Identity content in logs~~ Hide Identity content in logs Aug 30, 2023

kziemianek added 6 commits August 31, 2023 10:35

Merge branch 'dev' into 2040-anonymyze-data

6b725c3

Merge branch 'dev' into 2040-anonymyze-data

63b1a3c

remove deref

c5ba15e

add macro

df27180

Merge branch 'dev' into 2040-anonymyze-data

19e7ac7

data providers

707b611

kziemianek changed the title ~~Hide Identity content in logs~~ [WIP] do not log sensitive data Sep 1, 2023

kziemianek added 6 commits September 1, 2023 15:25

rest client

ce11b09

tls websocket server

3e3cab3

direct client

5f7a3b1

direct rpc server

297f415

assertions

afcd2f7

web2 verificaiton

94fa4fe

kziemianek changed the title ~~[WIP] do not log sensitive data~~ Do not log sensitive data Sep 4, 2023

kziemianek added 2 commits September 4, 2023 15:57

Merge branch 'dev' into 2040-anonymyze-data

a48eb1c

Merge branch 'dev' into 2040-anonymyze-data

87081d0

Merge branch 'dev' into 2040-anonymyze-data

6417946

add log macros

748920a

BillyWooo reviewed Sep 11, 2023

View reviewed changes

do not init logger from env for prod

9c065bd

Merge branch 'dev' into 2040-anonymyze-data

2e244a6

kziemianek commented Sep 12, 2023

View reviewed changes

tee-worker/enclave-runtime/src/lib.rs Show resolved Hide resolved

kziemianek requested a review from BillyWooo September 12, 2023 07:10

Kailai-Wang reviewed Sep 18, 2023

View reviewed changes

kziemianek added 3 commits September 19, 2023 10:45

remove log macros

4b8fdcd

Merge branch 'dev' into 2040-anonymyze-data

dc43257

cleanup deps

324bf39

Merge branch 'dev' into 2040-anonymyze-data

bf1022a

BillyWooo approved these changes Sep 21, 2023

View reviewed changes

kziemianek merged commit 1bec274 into dev Sep 21, 2023
28 checks passed

kziemianek deleted the 2040-anonymyze-data branch September 27, 2023 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not log sensitive data #2083

Do not log sensitive data #2083

kziemianek commented Aug 30, 2023 •

edited

Loading

BillyWooo commented Sep 11, 2023

kziemianek commented Sep 11, 2023

BillyWooo Sep 11, 2023

kziemianek Sep 11, 2023

Kailai-Wang Sep 18, 2023

BillyWooo Sep 18, 2023

kziemianek Sep 19, 2023

kziemianek commented Sep 11, 2023 •

edited

Loading

Kailai-Wang left a comment

Kailai-Wang Sep 18, 2023

kziemianek Sep 19, 2023

Kailai-Wang Sep 18, 2023

kziemianek commented Sep 19, 2023

BillyWooo commented Sep 20, 2023

BillyWooo left a comment

Do not log sensitive data #2083

Do not log sensitive data #2083

Conversation

kziemianek commented Aug 30, 2023 • edited Loading

BillyWooo commented Sep 11, 2023

kziemianek commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kziemianek commented Sep 11, 2023 • edited Loading

Kailai-Wang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kziemianek commented Sep 19, 2023

BillyWooo commented Sep 20, 2023

BillyWooo left a comment

Choose a reason for hiding this comment

kziemianek commented Aug 30, 2023 •

edited

Loading

kziemianek commented Sep 11, 2023 •

edited

Loading