gnodet-bot commented on code in PR #29749:
URL: https://github.com/apache/camel/pull/29749#discussion_r4555224111
##########
components/camel-knative/camel-knative-http/src/main/java/org/apache/camel/component/knative/http/KnativeOidcServiceOptions.java:
##########
@@ -81,26 +87,58 @@ public void configureOptions(CamelContext camelContext) {
* Read OIDC token from given path. Caches the token for future usage.
*/
public String retrieveOidcToken() {
- if (oidcToken == null || !cacheTokens) {
- try {
- oidcToken = IOHelper
-
.loadText(ResourceHelper.resolveMandatoryResourceAsInputStream(getCamelContext(),
oidcTokenPath))
- .trim();
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
+ String token = oidcToken;
+ if (token == null || !cacheTokens) {
+ token = loadOidcToken();
+ oidcToken = token;
}
- return oidcToken;
+ return token;
}
/**
* Force renew of token by reading again from token path. Token may have
been renewed already via volume mount
* update.
*/
public String renewOidcToken() {
- oidcToken = null;
- return retrieveOidcToken();
+ String token = loadOidcToken();
+ oidcToken = token;
+ return token;
+ }
+
+ /**
+ * Whether the given value of the <code>Authorization</code> header is
<code>Bearer</code> followed by the token at
+ * the token path. The comparison takes the same time wherever the first
difference is. When the cached token does
Review Comment:
⚠️ **Inaccurate constant-time claim:** `MessageDigest.isEqual`
short-circuits on length difference (`if (digesta.length != digestb.length)
return false;` in the JDK source), so the comparison is only constant-time for
equal-length inputs. An attacker could discover the byte-length of `"Bearer " +
token` by timing responses to Authorization headers of varying lengths. In
Knative deployments the practical risk is low (service-account JWTs have a
predictable length), but the Javadoc should not overstate the guarantee.
```suggestion
* the token path. The comparison is constant-time for equal-length
inputs (`MessageDigest.isEqual`
* short-circuits when the array lengths differ). When the cached token
does
```
##########
components/camel-knative/camel-knative-http/src/main/java/org/apache/camel/component/knative/http/KnativeOidcServiceOptions.java:
##########
@@ -81,26 +87,58 @@ public void configureOptions(CamelContext camelContext) {
* Read OIDC token from given path. Caches the token for future usage.
*/
public String retrieveOidcToken() {
- if (oidcToken == null || !cacheTokens) {
- try {
- oidcToken = IOHelper
-
.loadText(ResourceHelper.resolveMandatoryResourceAsInputStream(getCamelContext(),
oidcTokenPath))
- .trim();
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
+ String token = oidcToken;
+ if (token == null || !cacheTokens) {
+ token = loadOidcToken();
+ oidcToken = token;
}
- return oidcToken;
+ return token;
}
/**
* Force renew of token by reading again from token path. Token may have
been renewed already via volume mount
* update.
*/
public String renewOidcToken() {
- oidcToken = null;
- return retrieveOidcToken();
+ String token = loadOidcToken();
+ oidcToken = token;
+ return token;
+ }
+
+ /**
+ * Whether the given value of the <code>Authorization</code> header is
<code>Bearer</code> followed by the token at
+ * the token path. The comparison takes the same time wherever the first
difference is. When the cached token does
+ * not match, the token is read again, as Knative may have renewed the
mounted token since it was read: the renewed
+ * token is then accepted, and the token it replaces is no longer accepted.
+ *
+ * @param authorization the value of the <code>Authorization</code>
header of the request, may be null
+ * @return true when the request carries the token at the
token path
+ */
+ public boolean isAuthorized(String authorization) {
+ if (authorization == null) {
+ return false;
+ }
+ if (matches(authorization, retrieveOidcToken())) {
+ return true;
+ }
+ // without the cache the token was just read
+ return cacheTokens && matches(authorization, renewOidcToken());
+ }
+
+ private static boolean matches(String authorization, String token) {
+ byte[] expected = ("Bearer " + token).getBytes(StandardCharsets.UTF_8);
+ return MessageDigest.isEqual(expected,
authorization.getBytes(StandardCharsets.UTF_8));
+ }
+
+ private String loadOidcToken() {
+ try {
+ return IOHelper
+
.loadText(ResourceHelper.resolveMandatoryResourceAsInputStream(getCamelContext(),
oidcTokenPath))
+ .trim();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
Review Comment:
🔍 **Missing context in exception message:** When the token file is missing
or unreadable, the IOException message alone ("No such file or directory")
doesn't tell you *which* path failed. Since `loadOidcToken()` was freshly
extracted, it's the right time to add context.
```suggestion
throw new RuntimeException("Failed to load OIDC token from: " +
oidcTokenPath, e);
```
##########
components/camel-knative/camel-knative-http/src/main/docs/knative-http-component.adoc:
##########
@@ -245,3 +245,36 @@ camel.knative.client.oidc.token.path=oidc/token
camel.knative.client.oidc.renew.tokens.on.forbidden=false
camel.knative.client.oidc.cache.tokens=false
----
+
+== Knative Http consumer factory
+
+=== OIDC request verification
+
+With OIDC enabled on the service side, the Knative Http consumer accepts a
request only when its `Authorization` header is `Bearer` followed by the token
mounted at the token path. Any other request, also one without the header, gets
a `401` response and does not reach the route.
Review Comment:
📝 **Missing RFC 6750 space:** Per RFC 6750, the `Authorization` value is
`Bearer {token}` with a mandatory space. The current wording — `` `Bearer`
followed by the token `` — could mislead implementors into omitting it. The
code correctly uses `"Bearer " + token`.
```suggestion
With OIDC enabled on the service side, the Knative Http consumer accepts a
request only when its `Authorization` header is `Bearer ` (note the trailing
space) followed by the token mounted at the token path. Any other request, also
one without the header, gets a `401` response and does not reach the route.
```
--
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]
