exceptionfactory commented on a change in pull request #4988:
URL: https://github.com/apache/nifi/pull/4988#discussion_r611632508
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/AccessResource.java
##########
@@ -1456,10 +1464,12 @@ public Response createAccessToken(
// generate JWT for response
final String token =
jwtService.generateSignedToken(loginAuthenticationToken);
+ // currently there is no way to use javax.servlet-api to set
SameSite=Strict, so we do this using Jetty
+ HttpCookie jwtCookie = new
HttpCookie(JwtAuthenticationFilter.JWT_COOKIE_NAME, token, null, "/",
TWELVE_HOURS, true, true, null, 0, HttpCookie.SameSite.STRICT);
// build the response
final URI uri = URI.create(generateResourceUri("access", "token"));
- return generateCreatedResponse(uri, token).build();
+ return generateCreatedResponse(uri,
token).header(HttpHeader.SET_COOKIE.asString(),
jwtCookie.getRFC6265SetCookie()).build();
Review comment:
Wrapping the `header()` calls for setting the cookie in a single method
would help indicate which methods set the cookie. Something along the lines
for `generateTokenResponse(ResponseBuilder builder, String jwt)` would provide
an opportunity for streamlining cookie response generation.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/AccessResource.java
##########
@@ -847,8 +850,10 @@ public Response oidcExchange(@Context HttpServletRequest
httpServletRequest, @Co
throw new IllegalArgumentException("A JWT for this login request
identifier could not be found. Unable to continue.");
}
+ HttpCookie jwtCookie = new
HttpCookie(JwtAuthenticationFilter.JWT_COOKIE_NAME, jwt, null, "/",
TWELVE_HOURS, true, true, null, 0, HttpCookie.SameSite.STRICT);
Review comment:
Instead of instantiating `HttpCookie` in multiple methods, it would be
helpful to encapsulate the cookie generation using an interface and
implementation class. Having an interface method such as `String
CookieProvider.getJwtCookie(String jwt)` would streamline references here in
`AccessResource`, and also provide opportunity for introducing a unit test that
ensures certain fields are set as intended.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/AccessResource.java
##########
@@ -1812,4 +1826,25 @@ public void setLogoutRequestManager(LogoutRequestManager
logoutRequestManager) {
this.logoutRequestManager = logoutRequestManager;
}
+ private void logOutUser(HttpServletRequest httpServletRequest) throws
LogoutException {
+ final String jwt = getJwtFromRequest(httpServletRequest);
+ jwtService.logOut(jwt);
+ }
+
+ private String getJwtFromRequest(HttpServletRequest httpServletRequest) {
+ final String authCookie =
getCookieValue(httpServletRequest.getCookies(),
JwtAuthenticationFilter.JWT_COOKIE_NAME);
+ final String authHeader =
httpServletRequest.getHeader(httpServletRequest.getHeader(JwtAuthenticationFilter.AUTHORIZATION));
+
+ String jwt = null;
+
+ if (authCookie != null && !authCookie.isEmpty()) {
+ jwt = authCookie;
+ }
+
+ if (authHeader != null && !authHeader.isEmpty()) {
+ jwt = JwtAuthenticationFilter.getTokenFromHeader(authHeader);
Review comment:
Are the separate conditionals intentional? As implemented, request may
contain different values in the `Cookie` and `Authorization` headers, and the
`Authorization` header will override whatever was provided in the `Cookie`.
It seems like the `Authorization` header should be checked first, following
the current implementation.
Looking at the rest of the PR, this method also has some logic from the
retrieval used in the `JwtAuthenticationFilter`. It seems like this should be
centralized in a common place to avoid confusion. The Spring Security OAuth2
library includes the concept of a `BearerTokenResolver` interface and
implementation. We should either consider using that library, or at least
implementation a similar approach.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/AccessResource.java
##########
@@ -1812,4 +1826,25 @@ public void setLogoutRequestManager(LogoutRequestManager
logoutRequestManager) {
this.logoutRequestManager = logoutRequestManager;
}
+ private void logOutUser(HttpServletRequest httpServletRequest) throws
LogoutException {
+ final String jwt = getJwtFromRequest(httpServletRequest);
+ jwtService.logOut(jwt);
+ }
+
+ private String getJwtFromRequest(HttpServletRequest httpServletRequest) {
+ final String authCookie =
getCookieValue(httpServletRequest.getCookies(),
JwtAuthenticationFilter.JWT_COOKIE_NAME);
+ final String authHeader =
httpServletRequest.getHeader(httpServletRequest.getHeader(JwtAuthenticationFilter.AUTHORIZATION));
+
+ String jwt = null;
+
+ if (authCookie != null && !authCookie.isEmpty()) {
+ jwt = authCookie;
+ }
+
+ if (authHeader != null && !authHeader.isEmpty()) {
Review comment:
This could be streamlined using `StringUtils.isNotBlank()`:
```suggestion
if (StringUtils.isNotBlank(authHeader)) {
```
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/jwt/JwtAuthenticationFilter.java
##########
@@ -43,33 +51,63 @@ public Authentication attemptAuthentication(final
HttpServletRequest request) {
return null;
}
- // TODO: Refactor request header extraction logic to shared utility as
it is duplicated in AccessResource
+ // Check for JWT in cookie and header
+ final String cookieToken = getTokenFromCookie(request);
+ final String headerToken = getTokenFromHeader(request);
Review comment:
As mentioned in the `TODO` comment, recommend refactor this logic to a
shared interface and implementation class named `BearerTokenResolver` or
`AuthorizationTokenResolver`.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/AccessResource.java
##########
@@ -143,6 +146,7 @@
private static final Pattern REVOKE_ACCESS_TOKEN_LOGOUT_FORMAT =
Pattern.compile("(\\.google\\.com)");
private static final Pattern ID_TOKEN_LOGOUT_FORMAT =
Pattern.compile("(\\.okta)");
private static final int msTimeout = 30_000;
+ private static final int TWELVE_HOURS = 43200;
Review comment:
Hard-coding cookie maximum age does not seem like the best approach.
Using a Session Cookie, with a maximum age of `-1` will remove the cookie when
closing the browser. Multiple tabs and windows should continue to send the
session cookie, but closing the browser will remove the cookie and require the
user to login again. This seems like a better approach that follows the
current implementation more closely.
The other option is to base the cookie expiration using the JWT expiration,
but making this a session cookie seems like the better option.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/nf-common.js
##########
@@ -102,7 +102,6 @@
type: 'DELETE',
url: '../nifi-api/access/logout',
}).done(function () {
- nfStorage.removeItem("jwt");
Review comment:
Is there a reason for not the JWT token after a successful logout? It
seems like that should still occur.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/AccessResource.java
##########
@@ -1456,10 +1464,12 @@ public Response createAccessToken(
// generate JWT for response
final String token =
jwtService.generateSignedToken(loginAuthenticationToken);
+ // currently there is no way to use javax.servlet-api to set
SameSite=Strict, so we do this using Jetty
Review comment:
Refactoring cookie generation and rendering would also provide the
opportunity to include this comment in one location.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/AccessResource.java
##########
@@ -868,6 +873,10 @@ public void oidcLogout(@Context HttpServletRequest
httpServletRequest, @Context
throw new
IllegalStateException(OPEN_ID_CONNECT_SUPPORT_IS_NOT_CONFIGURED_MSG);
}
+ final String mappedUserIdentity = NiFiUserUtils.getNiFiUserIdentity();
+ removeCookie(httpServletResponse,
JwtAuthenticationFilter.JWT_COOKIE_NAME);
+ logger.info("Successfully invalidated JWT for " + mappedUserIdentity);
Review comment:
It seems like this should be a debug message, and also recommend using
placeholder log statements with reference that this is related to OIDC.
Removing this statement would also remove the need for retrieving the NiFi User
Identity from `NiFiUserUtils`, so it this necessary?
```suggestion
logger.debug("OIDC User [{}] Logout Completed", mappedUserIdentity);
```
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/AccessResource.java
##########
@@ -1507,7 +1518,10 @@ public Response logOut(@Context HttpServletRequest
httpServletRequest, @Context
return generateOkResponse().build();
} catch (final JwtException e) {
- logger.error("Logout of user " + mappedUserIdentity + " failed due
to: " + e.getMessage(), e);
+ logger.error("There was a problem with [" + mappedUserIdentity +
"]'s JWT, failed due to: " + e.getMessage(), e);
Review comment:
Recommend taking the opportunity to change the logging statement to use
placeholders:
```suggestion
logger.error("JWT Processing Failed for User [{}]: {}",
mappedUserIdentity, e.getMessage(), e);
```
##########
File path:
nifi-framework-api/src/main/java/org/apache/nifi/authorization/user/NiFiUser.java
##########
@@ -61,4 +61,9 @@
*/
String getClientAddress();
+ /**
+ * @return <code>true</code> if the user can use the log out button
+ */
+ boolean canLogOut();
Review comment:
What do you think about renaming this to logOutEnabled? It is minor,
but it reads better for method names.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/AccessResource.java
##########
@@ -1812,4 +1826,25 @@ public void setLogoutRequestManager(LogoutRequestManager
logoutRequestManager) {
this.logoutRequestManager = logoutRequestManager;
}
+ private void logOutUser(HttpServletRequest httpServletRequest) throws
LogoutException {
+ final String jwt = getJwtFromRequest(httpServletRequest);
+ jwtService.logOut(jwt);
+ }
+
+ private String getJwtFromRequest(HttpServletRequest httpServletRequest) {
+ final String authCookie =
getCookieValue(httpServletRequest.getCookies(),
JwtAuthenticationFilter.JWT_COOKIE_NAME);
+ final String authHeader =
httpServletRequest.getHeader(httpServletRequest.getHeader(JwtAuthenticationFilter.AUTHORIZATION));
+
+ String jwt = null;
+
+ if (authCookie != null && !authCookie.isEmpty()) {
Review comment:
This could be streamlined using `StringUtils.isNotBlank()`:
```suggestion
if (StringUtils.isNotBlank(authCookie)) {
```
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/jwt/JwtAuthenticationFilter.java
##########
@@ -34,7 +38,11 @@
// The Authorization header contains authentication credentials
public static final String AUTHORIZATION = "Authorization";
- private static final Pattern tokenPattern = Pattern.compile("^Bearer
(\\S*\\.\\S*\\.\\S*)$");
+ public static final String JWT_COOKIE_NAME = "jwt-auth-cookie";
+ private static final Pattern BEARER_HEADER_PATTERN =
Pattern.compile("^Bearer (\\S*\\.\\S*\\.\\S*)$");
+ private static final Pattern JWT_PATTERN =
Pattern.compile("^(\\S*\\.\\S*\\.\\S*)$");
+ private static final List<String> UNAUTHENTICATED_METHODS =
Arrays.asList("GET", "HEAD", "TRACE", "OPTIONS");
Review comment:
Recommend renaming this variable for clarity, since these methods still
require authentication, just not CSRF protection.
```suggestion
private static final List<String> IDEMPOTENT_METHODS =
Arrays.asList("GET", "HEAD", "TRACE", "OPTIONS");
```
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/jwt/JwtAuthenticationFilter.java
##########
@@ -43,33 +51,63 @@ public Authentication attemptAuthentication(final
HttpServletRequest request) {
return null;
}
- // TODO: Refactor request header extraction logic to shared utility as
it is duplicated in AccessResource
+ // Check for JWT in cookie and header
+ final String cookieToken = getTokenFromCookie(request);
+ final String headerToken = getTokenFromHeader(request);
+ if (cookieToken != null && !cookieToken.isEmpty()) {
+ if
(!UNAUTHENTICATED_METHODS.contains(request.getMethod().toUpperCase())) {
+ // To protect against CSRF when using a cookie, if the request
method requires authentication the request must have a matching Authorization
header JWT
+ if (headerToken.equals(cookieToken)) {
+ return new JwtAuthenticationRequestToken(headerToken,
request.getRemoteAddr());
+ } else {
+ throw new InvalidAuthenticationException("Authorization
HTTP header and authentication cookie did not match.");
+ }
+ } else {
+ return new JwtAuthenticationRequestToken(cookieToken,
request.getRemoteAddr());
+ }
+ } else if (headerToken != null && !headerToken.isEmpty()) {
Review comment:
This could be refactored to use `StringUtils.isNotBlank()`
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/jwt/JwtAuthenticationFilter.java
##########
@@ -34,7 +38,11 @@
// The Authorization header contains authentication credentials
public static final String AUTHORIZATION = "Authorization";
- private static final Pattern tokenPattern = Pattern.compile("^Bearer
(\\S*\\.\\S*\\.\\S*)$");
+ public static final String JWT_COOKIE_NAME = "jwt-auth-cookie";
Review comment:
As an added security measure, the `__Host-` prefix should be added to
this cookie name so that browsers will not allow the potential for compromised
subdomains to override the cookie value. The [Mozilla Developer Network
Set-Cookie](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie)
documentation describes the details.
```suggestion
public static final String JWT_COOKIE_NAME = "__Host-jwt-auth-cookie";
```
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/jwt/JwtService.java
##########
@@ -186,7 +187,7 @@ public String generateSignedToken(final
LoginAuthenticationToken authenticationT
* @throws JwtException if there is a problem with the token input
* @throws Exception if there is an issue logging the user out
*/
- public void logOut(String token) {
+ public void logOut(String token) throws LogoutException {
Review comment:
As a subclass of `RuntimeException`, `LogoutException` does not need to
be declared.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/jwt/JwtAuthenticationFilter.java
##########
@@ -43,33 +51,63 @@ public Authentication attemptAuthentication(final
HttpServletRequest request) {
return null;
}
- // TODO: Refactor request header extraction logic to shared utility as
it is duplicated in AccessResource
+ // Check for JWT in cookie and header
+ final String cookieToken = getTokenFromCookie(request);
+ final String headerToken = getTokenFromHeader(request);
+ if (cookieToken != null && !cookieToken.isEmpty()) {
+ if
(!UNAUTHENTICATED_METHODS.contains(request.getMethod().toUpperCase())) {
+ // To protect against CSRF when using a cookie, if the request
method requires authentication the request must have a matching Authorization
header JWT
+ if (headerToken.equals(cookieToken)) {
+ return new JwtAuthenticationRequestToken(headerToken,
request.getRemoteAddr());
+ } else {
+ throw new InvalidAuthenticationException("Authorization
HTTP header and authentication cookie did not match.");
+ }
+ } else {
+ return new JwtAuthenticationRequestToken(cookieToken,
request.getRemoteAddr());
+ }
+ } else if (headerToken != null && !headerToken.isEmpty()) {
+ return new JwtAuthenticationRequestToken(headerToken,
request.getRemoteAddr());
+ } else {
+ return null;
+ }
+ }
+
+ private boolean validJwtFormat(String authenticationHeader) {
+ Matcher matcher = BEARER_HEADER_PATTERN.matcher(authenticationHeader);
+ return matcher.matches();
+ }
- // get the principal out of the user token
+ private String getTokenFromHeader(final HttpServletRequest request) {
final String authorizationHeader = request.getHeader(AUTHORIZATION);
// if there is no authorization header, we don't know the user
if (authorizationHeader == null ||
!validJwtFormat(authorizationHeader)) {
return null;
} else {
// Extract the Base64 encoded token from the Authorization header
- final String token = getTokenFromHeader(authorizationHeader);
- return new JwtAuthenticationRequestToken(token,
request.getRemoteAddr());
+ return getTokenFromHeader(authorizationHeader);
}
}
- private boolean validJwtFormat(String authenticationHeader) {
- Matcher matcher = tokenPattern.matcher(authenticationHeader);
- return matcher.matches();
- }
-
public static String getTokenFromHeader(String authenticationHeader) {
- Matcher matcher = tokenPattern.matcher(authenticationHeader);
+ Matcher matcher = BEARER_HEADER_PATTERN.matcher(authenticationHeader);
if(matcher.matches()) {
return matcher.group(1);
} else {
throw new InvalidAuthenticationException("JWT did not match
expected pattern.");
}
}
+ private String getTokenFromCookie(final HttpServletRequest request) {
Review comment:
Spring `WebUtils` provides a `getCookie` method that could be used to
simplify this method.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/AccessResource.java
##########
@@ -1507,7 +1518,10 @@ public Response logOut(@Context HttpServletRequest
httpServletRequest, @Context
return generateOkResponse().build();
} catch (final JwtException e) {
- logger.error("Logout of user " + mappedUserIdentity + " failed due
to: " + e.getMessage(), e);
+ logger.error("There was a problem with [" + mappedUserIdentity +
"]'s JWT, failed due to: " + e.getMessage(), e);
+ return Response.serverError().build();
+ } catch (final LogoutException e) {
+ logger.error("There was a problem logging out user [%s] due to: ",
mappedUserIdentity, e);
Review comment:
The `%s` placeholder is valid for String formatting, but not logging
statements:
```suggestion
logger.error("Logout Failed for User [{}]", mappedUserIdentity,
e);
```
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/jwt/JwtAuthenticationFilter.java
##########
@@ -43,33 +51,63 @@ public Authentication attemptAuthentication(final
HttpServletRequest request) {
return null;
}
- // TODO: Refactor request header extraction logic to shared utility as
it is duplicated in AccessResource
+ // Check for JWT in cookie and header
+ final String cookieToken = getTokenFromCookie(request);
+ final String headerToken = getTokenFromHeader(request);
+ if (cookieToken != null && !cookieToken.isEmpty()) {
+ if
(!UNAUTHENTICATED_METHODS.contains(request.getMethod().toUpperCase())) {
+ // To protect against CSRF when using a cookie, if the request
method requires authentication the request must have a matching Authorization
header JWT
+ if (headerToken.equals(cookieToken)) {
+ return new JwtAuthenticationRequestToken(headerToken,
request.getRemoteAddr());
+ } else {
+ throw new InvalidAuthenticationException("Authorization
HTTP header and authentication cookie did not match.");
+ }
+ } else {
+ return new JwtAuthenticationRequestToken(cookieToken,
request.getRemoteAddr());
+ }
+ } else if (headerToken != null && !headerToken.isEmpty()) {
Review comment:
It would be helpful to refactor this set of conditionals to make the
logic easier to follow. Part of the challenge here seems to be attempting to
accomplish multiple goals in a single method. It would be easier to follow if
CSRF protection were handled separate from JWT request token retrieval, even
though they are related.
Leveraging Spring Security's CsrfFilter and customizing token retrieval and
comparison seems like a better way to enforce CSRF protection. The CsrfFilter
default configuration already handled skipping idempotent HTTP methods, so it
would be a better of customizing the CsrfTokenRepository implementation to
retrieve the appropriate header, and then compare it against the same value
provided in the `Authorization` header.
Separating the implementation will require some more, but it should help
avoid confusion, and avoid potential pitfalls by using less custom code.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
