Copilot commented on code in PR #293:
URL: https://github.com/apache/bigtop-manager/pull/293#discussion_r3005903088
##########
dev-support/docker/containers/build.sh:
##########
@@ -112,6 +129,11 @@ create_container() {
docker exec ${container} bash -c "PGPASSWORD=postgres psql -h
bm-postgres -p5432 -U postgres -d bigtop_manager -f
/opt/bigtop-manager-server/ddl/PostgreSQL-DDL-CREATE.sql"
docker exec ${container} bash -c "sed -i
's/localhost:5432/bm-postgres:5432/'
/opt/bigtop-manager-server/conf/application.yml"
fi
+
+ # Inject JWT secret into server config inside container (for
environments where env vars are not propagated)
+ docker exec ${container} bash -c "grep -q '^\s*secret:'
/opt/bigtop-manager-server/conf/application.yml || true"
+ docker exec ${container} bash -c "sed -i 's/^\(\s*secret:\).*/\1
\${BIGTOP_MANAGER_JWT_SECRET}/' /opt/bigtop-manager-server/conf/application.yml
|| true"
Review Comment:
The `grep`/`sed` patterns use `\s` to match whitespace, but POSIX
`grep`/`sed` don’t treat `\s` as whitespace. As a result, these commands won’t
match `secret:` lines and the “inject JWT secret” step is effectively a no-op.
Use a portable whitespace class (e.g., `[[:space:]]`) and make the logic
actually update/ensure the intended YAML key.
```suggestion
docker exec ${container} bash -c "if grep -q '^[[:space:]]*secret:'
/opt/bigtop-manager-server/conf/application.yml; then sed -i
's/^\([[:space:]]*secret:\).*/\1 \${BIGTOP_MANAGER_JWT_SECRET}/'
/opt/bigtop-manager-server/conf/application.yml; else printf '\nsecret: %s\n'
\"\${BIGTOP_MANAGER_JWT_SECRET}\" >>
/opt/bigtop-manager-server/conf/application.yml; fi || true"
```
##########
bigtop-manager-server/src/main/java/org/apache/bigtop/manager/server/utils/JWTUtils.java:
##########
@@ -33,23 +41,69 @@ public class JWTUtils {
public static final String CLAIM_TOKEN_VERSION = "token_version";
- protected static final String SIGN = "r0PGVyvjKOxUBwGt";
+ /**
+ * Dev-only fallback secret to preserve local boot for contributors.
+ * <p>
+ * In production, configure `bigtop-manager.security.jwt.secret`.
+ */
+ static final String DEFAULT_DEV_SECRET = "r0PGVyvjKOxUBwGt";
+
+ private final JwtProperties jwtProperties;
- // Token validity period (days)
- private static final int TOKEN_EXPIRATION_DAYS = 7;
+ public JWTUtils(JwtProperties jwtProperties) {
+ this.jwtProperties = jwtProperties;
+ }
- public static String generateToken(Long userId, String username, Integer
tokenVersion) {
- Instant expireTime = Instant.now().plus(TOKEN_EXPIRATION_DAYS,
ChronoUnit.DAYS);
+ public String generateToken(Long userId, String username, Integer
tokenVersion) {
+ Instant now = Instant.now();
+ Instant expireTime = now.plus(jwtProperties.getExpirationDays(),
ChronoUnit.DAYS);
return JWT.create()
+ .withIssuer(jwtProperties.getIssuer())
+ .withAudience(jwtProperties.getAudience())
+ .withIssuedAt(Date.from(now))
.withClaim(CLAIM_ID, userId)
.withClaim(CLAIM_USERNAME, username)
.withClaim(CLAIM_TOKEN_VERSION, tokenVersion)
- .withExpiresAt(expireTime)
- .sign(Algorithm.HMAC256(SIGN));
+ .withExpiresAt(Date.from(expireTime))
+ .sign(Algorithm.HMAC256(getSigningSecret()));
+ }
+
+ public DecodedJWT resolveToken(String token) throws
JWTVerificationException {
+ Algorithm algorithm = Algorithm.HMAC256(getSigningSecret());
+ JWTVerifier verifier = JWT.require(algorithm)
+ .withIssuer(jwtProperties.getIssuer())
+ .withAudience(jwtProperties.getAudience())
+ .build();
+
+ DecodedJWT decodedJWT = verifier.verify(token);
+
+ // Enforce issued-at to mitigate tokens without freshness metadata.
+ Date issuedAt = decodedJWT.getIssuedAt();
+ if (issuedAt == null) {
+ throw new JWTVerificationException("Missing iat");
+ }
Review Comment:
`resolveToken()` now requires issuer/audience and enforces presence of
`iat`. Tokens minted by the previous implementation (no issuer/audience/iat)
will start failing verification immediately after upgrade, forcing all clients
to re-authenticate. If that’s intended, it should be explicitly documented as a
breaking change; otherwise consider a compatibility mode/migration window
(e.g., optionally accept missing fields for a period).
##########
bigtop-manager-server/src/main/resources/application.yml:
##########
@@ -67,4 +67,15 @@ springdoc:
pagehelper:
reasonable: false
params: count=countSql
- support-methods-arguments: true
\ No newline at end of file
+ support-methods-arguments: true
+
+bigtop-manager:
+ security:
+ jwt:
+ # IMPORTANT: Set a strong, random secret in production (e.g. via env var
BIGTOP_MANAGER_JWT_SECRET)
+ secret: ${BIGTOP_MANAGER_JWT_SECRET:}
+ issuer: bigtop-manager
+ audience: bigtop-manager
+ expiration-days: 7
+ # Dev-only compatibility switch. Keep false in production.
+ allow-default-secret: false
Review Comment:
With `secret: ${BIGTOP_MANAGER_JWT_SECRET:}` and `allow-default-secret:
false`, a default install where the env var isn’t set will allow the app to
start but will later fail token generation/verification at runtime (typically
as 500s). If the intention is to require an explicit secret everywhere,
consider failing fast during startup or providing a clear dev/default profile
so contributors don’t hit opaque runtime errors.
##########
dev-support/docker/containers/build.sh:
##########
@@ -31,6 +31,18 @@ log() {
echo -e "\033[32m[LOG] $1\033[0m"
}
+# Generate a random JWT secret for local dev if user didn't provide one.
+init_jwt_secret() {
+ if [ -z "${BIGTOP_MANAGER_JWT_SECRET}" ]; then
+ # Dev-support default. Override by exporting BIGTOP_MANAGER_JWT_SECRET
before running this script.
+ BIGTOP_MANAGER_JWT_SECRET="r0PGVyvjKOxUBwGt"
+ export BIGTOP_MANAGER_JWT_SECRET
+ log "Defaulted BIGTOP_MANAGER_JWT_SECRET for dev-support"
Review Comment:
`init_jwt_secret` says it "Generate[s] a random JWT secret", but it actually
sets a hard-coded, well-known value. This is misleading and also makes local
environments share the same signing key, which weakens token integrity even in
dev. Either generate a per-run/per-dev random secret (and optionally persist
it) or update the comment/behavior to clearly indicate it’s a fixed dev default.
```suggestion
# Set a fixed default JWT secret for local dev if user didn't provide one.
init_jwt_secret() {
if [ -z "${BIGTOP_MANAGER_JWT_SECRET}" ]; then
# Fixed dev-support default. Override by exporting
BIGTOP_MANAGER_JWT_SECRET before running this script.
BIGTOP_MANAGER_JWT_SECRET="r0PGVyvjKOxUBwGt"
export BIGTOP_MANAGER_JWT_SECRET
log "Using fixed default BIGTOP_MANAGER_JWT_SECRET for dev-support"
```
##########
bigtop-manager-server/src/main/java/org/apache/bigtop/manager/server/utils/JWTUtils.java:
##########
@@ -33,23 +41,69 @@ public class JWTUtils {
public static final String CLAIM_TOKEN_VERSION = "token_version";
- protected static final String SIGN = "r0PGVyvjKOxUBwGt";
+ /**
+ * Dev-only fallback secret to preserve local boot for contributors.
+ * <p>
+ * In production, configure `bigtop-manager.security.jwt.secret`.
+ */
+ static final String DEFAULT_DEV_SECRET = "r0PGVyvjKOxUBwGt";
+
+ private final JwtProperties jwtProperties;
- // Token validity period (days)
- private static final int TOKEN_EXPIRATION_DAYS = 7;
+ public JWTUtils(JwtProperties jwtProperties) {
+ this.jwtProperties = jwtProperties;
+ }
- public static String generateToken(Long userId, String username, Integer
tokenVersion) {
- Instant expireTime = Instant.now().plus(TOKEN_EXPIRATION_DAYS,
ChronoUnit.DAYS);
+ public String generateToken(Long userId, String username, Integer
tokenVersion) {
+ Instant now = Instant.now();
+ Instant expireTime = now.plus(jwtProperties.getExpirationDays(),
ChronoUnit.DAYS);
return JWT.create()
+ .withIssuer(jwtProperties.getIssuer())
+ .withAudience(jwtProperties.getAudience())
+ .withIssuedAt(Date.from(now))
.withClaim(CLAIM_ID, userId)
.withClaim(CLAIM_USERNAME, username)
.withClaim(CLAIM_TOKEN_VERSION, tokenVersion)
- .withExpiresAt(expireTime)
- .sign(Algorithm.HMAC256(SIGN));
+ .withExpiresAt(Date.from(expireTime))
+ .sign(Algorithm.HMAC256(getSigningSecret()));
+ }
+
+ public DecodedJWT resolveToken(String token) throws
JWTVerificationException {
+ Algorithm algorithm = Algorithm.HMAC256(getSigningSecret());
+ JWTVerifier verifier = JWT.require(algorithm)
+ .withIssuer(jwtProperties.getIssuer())
+ .withAudience(jwtProperties.getAudience())
+ .build();
+
+ DecodedJWT decodedJWT = verifier.verify(token);
+
+ // Enforce issued-at to mitigate tokens without freshness metadata.
+ Date issuedAt = decodedJWT.getIssuedAt();
+ if (issuedAt == null) {
+ throw new JWTVerificationException("Missing iat");
+ }
+
+ // Reject tokens issued too far in the future (clock skew).
+ Instant now = Instant.now();
+ if (issuedAt.toInstant().isAfter(now.plus(5, ChronoUnit.MINUTES))) {
+ throw new JWTVerificationException("iat is in the future");
+ }
+
+ return decodedJWT;
}
- public static DecodedJWT resolveToken(String token) {
- return JWT.require(Algorithm.HMAC256(SIGN)).build().verify(token);
+ private String getSigningSecret() {
+ String secret = jwtProperties.getSecret();
+ if (secret != null && !secret.isBlank()) {
+ return secret;
+ }
+
+ if (jwtProperties.isAllowDefaultSecret()) {
+ return DEFAULT_DEV_SECRET;
+ }
+
+ throw new IllegalStateException(
+ "JWT secret is not configured. Please set
bigtop-manager.security.jwt.secret (or enable allowDefaultSecret for dev
only).");
}
Review Comment:
`getSigningSecret()` throws `IllegalStateException` when the secret is
missing, which will be handled by the global exception handler as a generic
500. That makes a common misconfiguration hard to diagnose and can surface as
an opaque runtime failure on login/auth. Consider failing fast at startup
(validation) or throwing/handling a domain-specific exception that returns a
clearer configuration error response.
```suggestion
throw new JwtConfigurationException(
"JWT secret is not configured. Please set
bigtop-manager.security.jwt.secret (or enable allowDefaultSecret for dev
only).");
}
/**
* Exception indicating that the JWT subsystem is misconfigured, for
example
* when the signing secret is missing in a non-development environment.
*/
static class JwtConfigurationException extends RuntimeException {
JwtConfigurationException(String message) {
super(message);
}
}
```
--
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]
