SEPURI-SAI-KRISHNA commented on PR #12595:
URL: https://github.com/apache/seatunnel/pull/12595#issuecomment-5969862462
Thanks for the review, @DanielLeens. You flagged Issue 1 as an assumption to
check, so I checked it, and it does not hold: JDK 8 has the same time-constant
implementation.
From `openjdk/jdk8u`,
`jdk/src/share/classes/java/security/MessageDigest.java`:
```java
int result = 0;
result |= lenA - lenB;
// time-constant comparison
for (int i = 0; i < lenA; i++) {
// If i >= lenB, indexB is 0; otherwise, i.
int indexB = ((i - lenB) >>> 31) * i;
result |= digesta[i] ^ digestb[indexB];
}
return result == 0;
```
There is no early return on a length mismatch. The difference is folded into
`result` and the loop still runs `lenA` times, with a branchless index so a
shorter `digestb` is read at index 0 rather than skipped. That is the same
shape I verified in the JDK 17 bytecode, so the javadoc claim holds on the Java
8 target and does not need softening.
Two honest caveats. The early returns that do exist are reference identity,
either argument null, and `lenB == 0`, so an empty configured credential is
distinguishable. That is a misconfiguration rather than a credential to
protect, and it is the same on both JDKs. And I read current `jdk8u`; I cannot
rule out that a very early 8u build predates the time-constant version, though
the comment in the source suggests it was written that way deliberately.
On the SHA-256-then-compare variant: agreed it is optional, and I would
rather not. It would hide the length on any implementation, but it also adds a
digest per request and a dependency on the hash for something the comparison
already gives us on both JDKs in question.
Happy to add a line to the javadoc recording that this was verified on 8u
and 17 if you think it is worth pinning.
**On the CI question**, you asked whether the Windows unit-test is green or
fails outside `BasicAuthFilterTest`. It is the latter, and I can be specific:
`BasicAuthFilterTest` ran and passed 6 of 6 on that job before it died, and the
job then failed on `Could not resolve dependencies for project
org.apache.seatunnel:seatunnel-flink-starter-common`. An infrastructure
failure, not a test. The same class passed 6 of 6 on all four unit-test legs,
JDK 8 and 11 on both Linux and Windows. `BasicAuthenticationIT` also ran in
`engine-v2-it (8)` and reported `Tests run: 9, Failures: 0, Errors: 0`, which
is the seven existing cases plus the two added here. Rerunning the failed jobs
now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
