Skip to content
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

Sanitize transport config and include in report (Alt Approach) #152

Merged
merged 20 commits into from
Jan 13, 2024

Conversation

amircybersec
Copy link
Contributor

@amircybersec amircybersec commented Jan 2, 2024

This PR scoped changed since opening it. Currently, it proposes a standalone sanitizer method in config.go that redacts sensitive user info.

Copy link
Contributor

@fortuna fortuna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is happening at a lower level than it should be. It should be in the caller of the connectivity library instead, like it's done in test-connectivity. Please revert.

x/connectivity/connectivity.go Outdated Show resolved Hide resolved
x/connectivity/connectivity.go Outdated Show resolved Hide resolved
x/config/config.go Outdated Show resolved Hide resolved
@amircybersec amircybersec changed the title Add StartTime & Duration to connectivity error for connect/send/receive operations Sanitize transport config and include in report Jan 3, 2024
x/config/config.go Outdated Show resolved Hide resolved
@fortuna fortuna marked this pull request as ready for review January 5, 2024 22:30
x/config/config.go Outdated Show resolved Hide resolved
x/config/config.go Show resolved Hide resolved
@amircybersec
Copy link
Contributor Author

We should either merge this PR or PR #159, not both. SanitizeConfig in this PR does not handle each transport explicitly though and has a potential for leak if transport does not encode sensitive user data in UserInfo part of the URL.

@amircybersec amircybersec changed the title Sanitize transport config and include in report Sanitize transport config and include in report (Alt Approach) Jan 8, 2024
x/config/config.go Outdated Show resolved Hide resolved
x/config/config.go Outdated Show resolved Hide resolved
x/config/config.go Outdated Show resolved Hide resolved
x/config/config.go Show resolved Hide resolved
x/config/config.go Outdated Show resolved Hide resolved
x/config/config_test.go Show resolved Hide resolved
x/config/config.go Outdated Show resolved Hide resolved
x/config/config.go Outdated Show resolved Hide resolved
} else {
return u, nil
}
// If no user info is found, return the scheme and redacted placeholder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Perhaps just drop the path or search params?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sanitizeURLGeneric is a bit conservative in terms of not leaking any sensitive info. Right now if no UserInfo is detected, I redact everything.

Perhaps, I should further dissect the URL and keep url.host if it has the ip:port format and keep path, query params and fragments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my other comment here

x/config/shadowsocks.go Outdated Show resolved Hide resolved
x/config/shadowsocks.go Outdated Show resolved Hide resolved
x/config/shadowsocks.go Show resolved Hide resolved
x/config/shadowsocks.go Show resolved Hide resolved
@amircybersec amircybersec requested a review from fortuna January 12, 2024 17:50
x/config/shadowsocks.go Outdated Show resolved Hide resolved
x/config/shadowsocks.go Outdated Show resolved Hide resolved
x/config/config.go Outdated Show resolved Hide resolved
@amircybersec amircybersec merged commit bb5a759 into Jigsaw-Code:main Jan 13, 2024
4 checks passed
@amircybersec
Copy link
Contributor Author

@fortuna Thanks for all the feedback! I applied the last requested changes, resolved merge conflicts and merged with main :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants