HADOOP-17311. ABFS: Logs should redact SAS signature #2422
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bilaharith
force-pushed
the
dev/HADOOP-17311-sassig
branch
from
5e7f550 to
0e4dbc8
Compare
bilaharith
force-pushed
the
dev/HADOOP-17311-sassig
branch
from
0e4dbc8 to
8883b59
Compare
|
💔 -1 overall
This message was automatically generated. |
steveloughran
requested changes
Oct 29, 2020
| @@ -36,6 +36,7 @@ | |||
| import org.codehaus.jackson.JsonParser; | |||
| import org.codehaus.jackson.JsonToken; | |||
| import org.codehaus.jackson.map.ObjectMapper; | |||
| import com.google.common.annotations.VisibleForTesting; | |||
There was a problem hiding this comment.
now all shaded I'm afraid. Making backporting harder already
| LOG.warn(String.format("Unknown host name: %s. Retrying to resolve the host name...", httpOperation.getUrl().getHost())); | ||
| LOG.warn(String.format( | ||
| "Unknown host name: %s. Retrying to resolve the host name...", | ||
| httpOperation.getHost())); |
There was a problem hiding this comment.
While you are there
- add a catch for UnknownHostException
- move from String.format to Log.warn("unknown host {}", httpOperation,getHost()
| final AzureBlobFileSystem fs; | ||
| String msg = null; | ||
| try { | ||
| fs = getFileSystem(); |
There was a problem hiding this comment.
use LambdaTestUtils.intercept(). Not only simpler, it will (correctly) fail if the rest operation didn't actually raise an exception
| if (this.maskedUrlStr != null) { | ||
| return this.maskedUrlStr; | ||
| } | ||
| final String urlStr = url.toString(); |
There was a problem hiding this comment.
This is complicated enough it could be pulled out into a static method, and so its handling fully tested in (new) Unit tests, as well as in the ITests.
snvijaya
suggested changes
Nov 2, 2020
| return this.maskedUrlStr; | ||
| } | ||
| final String urlStr = url.toString(); | ||
| final String qpStr = "sig="; |
There was a problem hiding this comment.
create a private static final string. - private static final String SIGNATURE_QUERY_PARAM_KEY = "sig=";
| int idx = urlStrSecondPart.indexOf("&"); | ||
| if (idx > -1) { | ||
| sb.append(urlStrSecondPart.substring(idx)); | ||
| } |
There was a problem hiding this comment.
Using string replace should be easier.
int sigStartIndex = urlStr.indexOf(SIGNATURE_QUERY_PARAM_KEY);
if (sigStartIndex == -1) {
// no signature query param in the url
return urlStr;
}
sigStartIndex += SIGNATURE_QUERY_PARAM_KEY.length();
int sigEndIndex = urlStr.indexOf("&", sigStartIndex);
String sigValue = (sigEndIndex == -1)
? urlStr.substring(sigStartIndex)
: urlStr.substring(sigStartIndex, sigEndIndex);
return urlStr.replace(sigValue, "XXXX");
| @@ -58,6 +58,9 @@ public static void dumpHeadersToDebugLog(final String origin, | |||
| if (key.contains("Cookie")) { | |||
| values = "*cookie info*"; | |||
| } | |||
| if (key.equals("sig")) { | |||
There was a problem hiding this comment.
Is a header called "sig" getting added when SAS ?
There was a problem hiding this comment.
The latency tracker sends all the query params through headers.
|
Driver test results using accounts in Central India HNS-OAuth[INFO] Results: HNS-SharedKey[INFO] Results: NonHNS-SharedKey[INFO] Results: |
bilaharith
commented
Nov 2, 2020
| @@ -61,6 +64,8 @@ | |||
|
|
|||
| private final String method; | |||
| private final URL url; | |||
| private String maskedUrlStr; | |||
There was a problem hiding this comment.
Remove Str suffix
bilaharith
commented
Nov 2, 2020
| ,"http://www.testurl.net?abc=xyz&sig=abcd" | ||
| ,"http://www.testurl.net?abc=xyz&sig=XXXX"); | ||
|
|
||
| testIfMaskedSuccessfully("Where sig query param is not present" |
There was a problem hiding this comment.
query params ending mysig/*sig.
There was a problem hiding this comment.
sig in other cases, like caps,
sig as suffix in the param names and values
|
💔 -1 overall
This message was automatically generated. |
|
findbugs is pretty unhappy |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The yetus run error is due to tests failing on the WASB side, which is not affected by the changes proposed here. The findbugs reported is also unrelated to the change in this PR. |
|
where did the findbugs error come from? do we need to fix/roll back that change? |
This comment has been minimized.
This comment has been minimized.
This is fixed. |
|
thx. Trying to work out where that regression failing tests is coming from. as far as I'm concerned, consistent unit test failures are a block on anything new going in. All too easy to ignore and then accidentally add more regressions |
steveloughran
changed the title
asfgit
pushed a commit
that referenced
this pull request
Nov 25, 2020
Contributed by bilaharith. Change-Id: Iff0ed4303ac5ce41b62bfda8150ee983dafa40be
jojochuang
pushed a commit
to jojochuang/hadoop
that referenced
this pull request
May 23, 2023
…che#2422) Contributed by bilaharith. Change-Id: Iff0ed4303ac5ce41b62bfda8150ee983dafa40be
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Masking SAS signatures from logs
HNS-OAuth
[INFO] Results:
[INFO]
[INFO] Tests run: 88, Failures: 0, Errors: 0, Skipped: 0
[INFO] Results:
[INFO]
[WARNING] Tests run: 459, Failures: 0, Errors: 0, Skipped: 66
[INFO] Results:
[INFO]
[WARNING] Tests run: 208, Failures: 0, Errors: 0, Skipped: 24
HNS-SharedKey
[INFO] Results:
[INFO]
[INFO] Tests run: 88, Failures: 0, Errors: 0, Skipped: 0
[INFO] Results:
[INFO]
[WARNING] Tests run: 459, Failures: 0, Errors: 0, Skipped: 24
[INFO] Results:
[INFO]
[WARNING] Tests run: 208, Failures: 0, Errors: 0, Skipped: 16
NonHNS-SharedKey
[INFO] Results:
[INFO]
[INFO] Tests run: 88, Failures: 0, Errors: 0, Skipped: 0
[INFO] Results:
[INFO]
[WARNING] Tests run: 459, Failures: 0, Errors: 0, Skipped: 247
[INFO] Results:
[INFO]
[WARNING] Tests run: 208, Failures: 0, Errors: 0, Skipped: 16