[Feature] Support mysql client OpenID Connect Pluggable Authentication #55846
Conversation
![starrocks-cr[bot]](https://avatars.githubusercontent.com/in/404334?s=60&v=4){kind=small}
| * The aud field in the JWT identifies the recipients that the JWT is intended for. | ||
| */ | ||
| @ConfField(mutable = false) | ||
| public static String oidc_required_audience = ""; | ||
| } |
There was a problem hiding this comment.
The most risky bug in this code is:
Using an empty string for critical JWT configuration values can lead to insecure defaults and potential security vulnerabilities if not handled properly when validating JWTs.
You can modify the code like this:
@ConfField(mutable = false)
public static String oidc_jwks_url = "default_value_here"; // Set a sensible default or validate it programmatically
@ConfField(mutable = false)
public static String oidc_required_issuer = "default_value_here"; // Set a sensible default or validate prior to use
@ConfField(mutable = false)
public static String oidc_required_audience = "default_value_here"; // As above, set or validate as necessaryEnsure that each configuration has a valid default or check them during application initialization/configuration loading to avoid using insecure defaults.
| byte[] openIdConnect = MysqlProto.readLenEncodedString(authBuffer); | ||
| OpenIdConnectVerifier.verify(new String(openIdConnect), user, jwksUrl, principalFiled, requireIssuer, requireAudience); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
The most risky bug in this code is:
The typo in the variable name principalFiled should be principalField.
You can modify the code like this:
package com.starrocks.authentication;
//... other imports ...
public class OpenIdConnectAuthenticationProvider implements AuthenticationProvider {
public static final String PLUGIN_NAME = AuthPlugin.AUTHENTICATION_OPENID_CONNECT.name();
private final String jwksUrl;
private final String principalField; // Corrected variable name
private final String requireIssuer;
private final String requireAudience;
public OpenIdConnectAuthenticationProvider(String jwksUrl, String principalField, // Corrected parameter name
String requireIssuer, String requireAudience) {
this.jwksUrl = jwksUrl;
this.principalField = principalField; // Use corrected variable name
this.requireIssuer = requireIssuer;
this.requireAudience = requireAudience;
}
//... rest of the code ...
}
|
|
||
| return JWKSet.load(inputStream); | ||
| } | ||
| } |
There was a problem hiding this comment.
The most risky bug in this code is:
The verifyJWT method lacks a null check for the return value of jwkSet.getKeyByKeyId(kid), which can lead to a NullPointerException.
You can modify the code like this:
private static void verifyJWT(String jwt, JWKSet jwkSet) throws AuthenticationException, ParseException, JOSEException {
SignedJWT signedJWT = SignedJWT.parse(jwt);
String kid = signedJWT.getHeader().getKeyID();
var key = jwkSet.getKeyByKeyId(kid);
if (key == null) {
throw new AuthenticationException("Cannot find public key for kid: " + kid);
}
RSAPublicKey publicKey = key.toRSAKey().toRSAPublicKey();
if (publicKey == null) {
throw new AuthenticationException("Public key conversion failed for kid: " + kid);
}
RSASSAVerifier verifier = new RSASSAVerifier(publicKey);
if (!signedJWT.verify(verifier)) {
throw new AuthenticationException("JWT with kid " + kid + " is invalid!");
}
}
HangyuanLiu
force-pushed
the
0212-oidc
branch
from
3407b84 to
f3179bd
Compare
| @@ -3474,4 +3474,32 @@ public class Config extends ConfigBase { | |||
|
|
|||
| @ConfField(mutable = false) | |||
| public static int max_historical_automated_cluster_snapshot_jobs = 100; | |||
|
|
|||
| /** | |||
| * The URL to a JWKS service or a local file in the conf dir | |||
There was a problem hiding this comment.
The comments can be added in to the comment field like
@ConfField(mutable = fase, comment = "xxx");
So that admin show config will see the usage of this config param.
| if (jwksUrl.startsWith("http://") || jwksUrl.startsWith("https://")) { | ||
| jwksInputStream = new URL(jwksUrl).openStream(); | ||
| } else { | ||
| String filePath = StarRocksFE.STARROCKS_HOME_DIR + "/conf/" + jwksUrl; |
There was a problem hiding this comment.
Does this file have to be added to the conf/ directory? If so, the comments of this param should be marked.
HangyuanLiu
force-pushed
the
0212-oidc
branch
from
c8ed522 to
1b5390f
Compare
|
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 64 / 72 (88.89%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
Why I'm doing:
Compatible with openIdConnect authentication method in MySQL 9.2
What I'm doing:
Fixes #55847
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: