Backport to branch(3) : Fix NullPointerException when a client is misconfigured with digital signature #318
Conversation
…l signature for HMAC (#302)
Summary of ChangesHello @feeblefakie, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request backports a critical fix to prevent Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a backport that fixes a NullPointerException when a client is misconfigured for digital signatures. The core change is modifying SecretRegistry.lookup to throw a MissingSecretException instead of returning null when a secret is not found. This is a good improvement for robustness. The related code, including tests, has been updated accordingly.
I've added two comments:
- A high-severity comment on
SecretManager.registerpointing out a pre-existing race condition and recommending an atomic operation for registration as a follow-up. - A medium-severity comment on
ScalarSecretRegistry.lookupwith a suggestion to make the code more concise and idiomatic.
| public void register(SecretEntry entry) { | ||
| SecretEntry existing = registry.lookup(entry.getKey()); | ||
| if (existing != null) { | ||
| try { | ||
| registry.lookup(entry.getKey()); | ||
| throw new DatabaseException(CommonError.SECRET_ALREADY_REGISTERED); | ||
| } catch (MissingSecretException e) { | ||
| registry.bind(entry); | ||
| } | ||
| registry.bind(entry); | ||
| } |
There was a problem hiding this comment.
This implementation of register uses a "Look-Before-You-Leap" (LBYL) approach by first calling lookup and then bind. This pattern is vulnerable to a race condition in a concurrent environment. Two threads could both check for the existence of a secret, find it missing, and then both attempt to bind it, with the second one overwriting the first.
A more robust approach would be to use an atomic 'put-if-absent' operation in the underlying SecretRegistry. The bind method in SecretRegistry could be modified to use a conditional Put (e.g., with PutIfNotExists in ScalarDB) and throw an exception if the secret already exists. This would make the registration atomic.
The register method here would then simplify to just calling registry.bind(entry) and handling the potential exception.
Since this race condition existed before this change, and fixing it would require changes outside the scope of this file, I recommend creating a follow-up task to address this atomicity issue.
| Result result; | ||
| try { | ||
| return storage.get(get).map(this::toSecretEntry).orElse(null); | ||
| result = | ||
| storage | ||
| .get(get) | ||
| .orElseThrow(() -> new MissingSecretException(CommonError.SECRET_NOT_FOUND)); | ||
| } catch (ExecutionException e) { | ||
| throw new DatabaseException(CommonError.GETTING_SECRET_KEY_FAILED, e, e.getMessage()); | ||
| } | ||
|
|
||
| return toSecretEntry(result); |
There was a problem hiding this comment.
The logic in this try-catch block can be made more concise and idiomatic by better leveraging the Optional API. You can chain map and orElseThrow to achieve the same result with less code.
try {
return storage
.get(get)
.map(this::toSecretEntry)
.orElseThrow(() -> new MissingSecretException(CommonError.SECRET_NOT_FOUND));
} catch (ExecutionException e) {
throw new DatabaseException(CommonError.GETTING_SECRET_KEY_FAILED, e, e.getMessage());
}
2 participants
This is an automated backport of the following:
Please merge this PR after all checks have passed.