SEPURI-SAI-KRISHNA opened a new pull request, #12595:
URL: https://github.com/apache/seatunnel/pull/12595
## Purpose of this pull request
`BasicAuthFilter` compares the credentials from an incoming request against
the configured ones with `String.equals`, joined by `&&`:
```java
if (username.equals(httpConfig.getBasicAuthUsername())
&& password.equals(httpConfig.getBasicAuthPassword())) {
```
Two separate properties of that line are worth changing, and they are
independent of each other.
`String.equals` returns at the first differing character, so the work it
does grows with how many leading characters already match. `&&` short-circuits,
so when the username does not match, the password is never compared at all, and
the amount of work the filter does separates a wrong username from a correct
username with a wrong password. That is CWE-208, an observable discrepancy in a
credential check.
This is hardening, not a security fix. I raised it with ASF Security first
and they reviewed it and concluded it is not a vulnerability, because the
published threat model treats the REST API and Web UI as inside the
administrative boundary and network reachability, not Basic auth, as the
security boundary. I agree with that, and I am noting it so the change is read
as ordinary code quality rather than an embargoed disclosure.
The case for it is simply that the difference should not be there: the fix
is small, local, and does not change which credentials are accepted.
Closes #12594
## What this changes
Both comparisons are computed into locals before the branch, so neither is
skipped, and each one goes through `MessageDigest.isEqual` on the UTF-8 bytes
rather than `String.equals`.
`MessageDigest.isEqual` accumulates the difference across bytes with `xor`
and `or` instead of returning at the first mismatch. I checked this against the
JDK 17 bytecode rather than taking the javadoc on trust: the loop bound is the
length of the **first** argument and there is no early return inside it. The
request's own value is therefore passed first, so the number of bytes read is
something the caller already knows and the length of the configured credential
is not what drives it.
A `credentialMatches` helper carries this, with the argument-order reasoning
in its javadoc so a later edit does not silently swap the arguments and
reintroduce the length dependency.
## Null safety
The configured credentials default to `admin`, but they are plain `String`
fields and can be unset. `String.equals(null)` is safely `false`; reading bytes
off a null is an NPE, which would turn a 401 into a 500. The helper therefore
rejects an unset credential explicitly, and there is a test for each of the
three unset combinations.
## Behaviour equivalence
Every accept-or-reject decision is unchanged. Checked directly, old
expression against new, on the cases that tend to be where a rewrite like this
goes wrong:
| provided | configured | before | after |
| --- | --- | --- | --- |
| `admin` | `admin` | true | true |
| `admin` | `root` | false | false |
| `s3cre` | `s3cret` (prefix) | false | false |
| `s3crets` | `s3cret` (longer) | false | false |
| `` | `` | true | true |
| `` | `admin` | false | false |
| `admin` | `` | false | false |
| `é` (multi-byte UTF-8) | `é` | true | true |
| `admin` | unset (null) | false | false |
No option, default or public signature changes. Nothing that authenticates
today stops authenticating.
## Tests
`BasicAuthFilterTest` is new, because `BasicAuthFilter` had no unit test. It
follows `ExceptionHandlingFilterTest`, the existing per-filter test sitting in
the same `rest/filter` test package, and uses the same Mockito shape. It covers
correct credentials passing the chain, wrong password, wrong username, a
credential that is a proper prefix of the configured one, all three
unset-credential combinations asserting no exception escapes, and
authentication being disabled.
`BasicAuthenticationIT` gains two cases. Its existing negative case sends
`wronguser:wrongpassword`, which gets both halves wrong and so cannot tell
whether the password is examined once the username has already failed. The new
cases send a correct username with a wrong password, and a proper prefix of
both, and expect 401 from each.
One thing to flag: the unit tests are verified locally, but the two IT cases
are not. I have no Docker available on this machine, so CI will be the first
thing to execute them. They mirror the existing
`testAccessWithIncorrectCredentials` method in the same file, use no identifier
that file does not already use, and add no import, but I would rather say so
than have it discovered in a red build.
## What the tests do and do not catch
Stating this rather than leaving it to be discovered, because it is the
honest shape of a behaviour-preserving change.
Reverting this patch entirely, back to `String.equals` joined by `&&`,
leaves **all six unit tests passing**. I ran that mutation rather than assuming
it. That is the expected result: the accept-or-reject decisions are identical
either way, so no black-box test can separate the two implementations. A timing
assertion could in principle, but it would be flaky on CI and would not earn
its place, so there is not one here.
What the tests do catch is the risk this rewrite actually introduces.
Removing the null guard while keeping the byte comparison fails
`testUnsetCredentialIsRejectedAndDoesNotThrow` with exactly the regression it
exists to prevent:
```
java.lang.NullPointerException: Cannot invoke
"String.getBytes(java.nio.charset.Charset)"
because "expected" is null
```
So the suite is a regression guard for the rewrite, not evidence of the
timing property. The timing property rests on `MessageDigest.isEqual` and on
both comparisons being evaluated unconditionally, both of which are visible in
the diff and neither of which needs a test to be checked by a reviewer.
--
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]
