600: Cached Our Private Key Only After Successful Auth with RS #604
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jcrichlake
reviewed
Oct 25, 2023
...main/java/gov/hhs/cdc/trustedintermediary/external/reportstream/ReportStreamOrderSender.java
Show resolved
Hide resolved
jcrichlake
reviewed
Oct 25, 2023
...main/java/gov/hhs/cdc/trustedintermediary/external/reportstream/ReportStreamOrderSender.java
Show resolved
Hide resolved
halprin
commented
Oct 25, 2023
...main/java/gov/hhs/cdc/trustedintermediary/external/reportstream/ReportStreamOrderSender.java
Outdated
Show resolved
Hide resolved
jcrichlake
approved these changes
Oct 25, 2023
jcrichlake
approved these changes
Oct 25, 2023
jorg3lopez
reviewed
Oct 25, 2023
| var senderPrivateKey = | ||
| "trusted-intermediary-private-key-" + ApplicationContext.getEnvironment(); | ||
| String key = this.keyCache.get(senderPrivateKey); | ||
| String key = keyCache.get(OUR_PRIVATE_KEY_ID); | ||
| if (key != null) { | ||
| return key; |
There was a problem hiding this comment.
For record keeping. We still need to have a system in place that will change our private key in the cache, when it expires or when it is not valid.
There was a problem hiding this comment.
Exactly! Thanks for calling that out and adding that item to our engineering task list channel.
jorg3lopez
reviewed
Oct 25, 2023
| token = extractToken(rsResponse); | ||
| } catch (Exception e) { | ||
| throw new UnableToSendOrderException( | ||
| "Error getting the API token from ReportStream", e); | ||
| } | ||
|
|
||
| // only cache our private key if we successfully authenticate to RS | ||
| cachePrivateKeyIfNotCachedAlready(ourPrivateKey); |
There was a problem hiding this comment.
Great work here. I wonder since cachePrivateKeyIfNotCachedAlready() deals with only our private key, what are your thoughts on renaming it cacheOurPrivateKeyIfNotCachedAlready()?
There was a problem hiding this comment.
Great idea. I'll do that!
jorg3lopez
approved these changes
Oct 25, 2023
|
Kudos, SonarCloud Quality Gate passed! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cached Our Private Key Only After Successful Auth with RS
Before this PR, we cached our private key after retrieving it no matter what happened. This prevented us from having to continually call Azure for our secret each time we need to login to ReportStream (which is about every 5 minutes). But, if the key was bad for some reason, we would fail to login to RS in perpetuity until we restarted our application (which would clear the cache). This happened because once we retrieved the secret, we cached it without ever clearing it.
Now, we don't even cache our private key in the first place if we fail to login to RS. We also only cache the key if it wasn't already cached. This prevents us from continuously caching the key whenever we successfully authenticate with a key that was already cached.
Issue
#600.
Checklist