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

FE print token in FE log #52535

Open
xyllq999 opened this issue Nov 1, 2024 · 11 comments
Open

FE print token in FE log #52535

xyllq999 opened this issue Nov 1, 2024 · 11 comments
Labels
type/bug Something isn't working

Comments

@xyllq999
Copy link
Contributor

xyllq999 commented Nov 1, 2024

FE will print token in fe log。
image
image

I have two options to solve this problem, either by including a function at each location where the uri is printed and masking the token field. Or change GET to POST to solve the problem completely, but this UT is not very easy to rewrite. Would you like to ask what you think of this? @kevincai

StarRocks version (Required)

3.2.0+ and more

@xyllq999 xyllq999 added the type/bug Something isn't working label Nov 1, 2024
@xyllq999
Copy link
Contributor Author

xyllq999 commented Nov 1, 2024

I've done the modification to POST in the internal code, and it works perfectly. But I don't seem to be able to fulfill the community's requirements for UT. Which way do you think is better? If you adopt POST, please help solve the UT problem, thank you very much. @kevincai

@kevincai
Copy link
Contributor

kevincai commented Nov 1, 2024

please connect the PR this issue

@xyllq999
Copy link
Contributor Author

xyllq999 commented Nov 1, 2024

please connect the PR this issue

So is it the way to mention POST or which way? Personally, I prefer the modification of the POST mode.

@kevincai
Copy link
Contributor

kevincai commented Nov 1, 2024

it is better to eliminate the token, turn to signature validation, no matter it is a get or post.

That is:

client:
sign the url with the token, generate a signature and append the signature as a query parameter.
server:
use the token to generate the signature again, if the signature matches the one in query url, the validation passed, otherwise reject the request.

This way the token will be never transfterred in wire in plaintext.

@xyllq999
Copy link
Contributor Author

xyllq999 commented Nov 1, 2024

it is better to eliminate the token, turn to signature validation, no matter it is a get or post.

That is:

client: sign the url with the token, generate a signature and append the signature as a query parameter. server: use the token to generate the signature again, if the signature matches the one in query url, the validation passed, otherwise reject the request.

This way the token will be never transfterred in wire in plaintext.

You have a good point. However, I think that such a change is too big for a PR, and I may not be able to complete the change perfectly and ensure the compatibility of upgrades and downgrades. Therefore, I wonder if I can change the mode to POST to prevent tokens from being printed in plaintext in logs.
As for the plaintext transmission problem, we have transformed HTTP into HTTPS internally. However, this code cannot be contributed to the community. We use our own encryption and decryption.

@kevincai
Copy link
Contributor

kevincai commented Nov 1, 2024

It will also introduce compatibility between upgrade and downgrade by changing from GET to POST. The compatibility issue can't be avoided either way.

@xyllq999
Copy link
Contributor Author

xyllq999 commented Nov 1, 2024

It will also introduce compatibility between upgrade and downgrade by changing from GET to POST. The compatibility issue can't be avoided either way.

Internal heartbeat interfaces are not sensed by external invoking. Why is there a compatibility problem?

@xyllq999
Copy link
Contributor Author

xyllq999 commented Nov 1, 2024

It will also introduce compatibility between upgrade and downgrade by changing from GET to POST. The compatibility issue can't be avoided either way.

So do you prefer to call a function that shields tokens where the URL is printed?

@kevincai
Copy link
Contributor

kevincai commented Nov 1, 2024

It will also introduce compatibility between upgrade and downgrade by changing from GET to POST. The compatibility issue can't be avoided either way.

Internal heartbeat interfaces are not sensed by external invoking. Why is there a compatibility problem?

during the upgrade/downgrade in a HA cluster, the new leader can't probe the follower in older version, which leads to wrong state/aliveness judgement.

@kevincai
Copy link
Contributor

kevincai commented Nov 1, 2024

It will also introduce compatibility between upgrade and downgrade by changing from GET to POST. The compatibility issue can't be avoided either way.

So do you prefer to call a function that shields tokens where the URL is printed?

If it can't be eliminated at all for now, just mask it as a lazy workaround.

@xyllq999
Copy link
Contributor Author

xyllq999 commented Nov 1, 2024

It will also introduce compatibility between upgrade and downgrade by changing from GET to POST. The compatibility issue can't be avoided either way.

So do you prefer to call a function that shields tokens where the URL is printed?

If it can't be eliminated at all for now, just mask it as a lazy workaround.

Know what you mean, thanks for answering my questions, and I'll submit my code as soon as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants