BFD-3723: Determine SAMHSA authorization based on certificate identity #2486
+928
−91
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
aschey-forpeople
commented
Nov 8, 2024
| fi | ||
|
|
||
| # Skip binary formats | ||
| case "$extension" in |
There was a problem hiding this comment.
Shellcheck was blowing up when trying to parse the certificate formats
| @@ -160,12 +160,13 @@ if lsof -Pi :4566 -sTCP:LISTEN -t >/dev/null ; then | |||
| export AWS_ACCESS_KEY_ID=$AWS_ACCESS_KEY | |||
| export AWS_SECRET_ACCESS_KEY=$AWS_SECRET_KEY | |||
| SSM_PARAMETER_PATH=/bfd-server | |||
| export CONFIG_SETTINGS_JSON=$(cat <<EOF | |||
| CONFIG_SETTINGS_JSON=$(cat <<EOF | |||
There was a problem hiding this comment.
Shellcheck wanted the export on a separate line
| # If debug_suspend is y the pipeline app will wait for a debugger to connect before doing any work. | ||
| if [[ $debug_enabled = "true" ]] ; then | ||
| ARGS="$ARGS -agentlib:jdwp=transport=dt_socket,server=y,suspend=${debug_suspend},address=*:5005" | ||
| ARGS="$ARGS -agentlib:jdwp=transport=dt_socket,server=y,suspend=${debug_suspend},address=*:${debug_port}" |
There was a problem hiding this comment.
The debug_port variable already existed, but wasn't used, so shellcheck was complaining. Figure we might as well use it since it already exists.
| @@ -294,7 +296,7 @@ if [[ x$kubernetes = xtrue ]] ; then | |||
| elif [[ x$image_tag = x ]] ; then | |||
| echo running application directly | |||
| export CLASSPATH="$classpath" | |||
| exec $java $properties $ARGS $mainClass | |||
| exec $java $ARGS $mainClass | |||
There was a problem hiding this comment.
$properties wasn't being set anywhere so it didn't do anything.
| /*** | ||
| * test. | ||
| */ | ||
| public class ClientCertificateUtils { |
There was a problem hiding this comment.
Most of this was moved here from RequestResponseMdcFilter so it can be reused.
aschey-forpeople
requested review from
mjburling,
dondevun and
malessi
as code owners
dondevun
reviewed
Nov 15, 2024
apps/bfd-server/bfd-server-war/src/main/java/gov/cms/bfd/server/war/AllowSamhsaFilter.java
Outdated
Show resolved
Hide resolved
dondevun
previously approved these changes
Nov 19, 2024
MahiFentaye
previously approved these changes
Nov 19, 2024
MahiFentaye
previously approved these changes
Nov 26, 2024
timothy-ellis-ky
previously approved these changes
Nov 26, 2024
apps/bfd-server/bfd-server-war/src/test/java/gov/cms/bfd/server/war/utils/RDATestUtils.java
Outdated
Show resolved
Hide resolved
…r/war/utils/RDATestUtils.java
...-server/bfd-server-war/src/test/java/gov/cms/bfd/server/war/ExplanationOfBenefitE2EBase.java
Outdated
Show resolved
Hide resolved
…r/war/ExplanationOfBenefitE2EBase.java
apps/bfd-server/bfd-server-war/src/main/java/gov/cms/bfd/server/war/AllowSamhsaFilter.java
Outdated
Show resolved
Hide resolved
…r/war/AllowSamhsaFilter.java
...rver/bfd-server-war/src/main/java/gov/cms/bfd/server/war/commons/ClientCertificateUtils.java
Outdated
Show resolved
Hide resolved
…r/war/commons/ClientCertificateUtils.java
...rver/bfd-server-war/src/main/java/gov/cms/bfd/server/war/commons/CommonTransformerUtils.java
Outdated
Show resolved
Hide resolved
…r/war/commons/CommonTransformerUtils.java
apps/bfd-server/bfd-server-war/src/main/java/gov/cms/bfd/server/war/AllowSamhsaFilter.java
Outdated
Show resolved
Hide resolved
…r/war/AllowSamhsaFilter.java
...rver/bfd-server-war/src/main/java/gov/cms/bfd/server/war/commons/ClientCertificateUtils.java
Outdated
Show resolved
Hide resolved
…r/war/commons/ClientCertificateUtils.java
MahiFentaye
approved these changes
Nov 27, 2024
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.
JIRA Ticket:
BFD-3723
What Does This PR Do?
This change reduces the likelihood of a SAMHSA incident by automatically disallowing any client from seeing SAMHSA data unless they use a certificate that is explicitly allowed. We do this by dynamically loading in the serial numbers for each allowed certificate at startup. The allowed certificates are determined by a new configuration parameter
samhsa_allowed_certificate_aliases_json. The values in that parameter directly match the names underclient_certificates/*, which are used to populate the trust store on deployment. Serial numbers are not globally unique, but they are unique per CA, which should be sufficient enough to verify identity for our purposes.On each request, we extract the serial number from the provided client certificate and compare it with the list of allowed certificates that we generated on startup. The
excludeSAMHSAparameter is still honored for any clients that are allowed to see SAMHSA info.I've added the list of certificates that should have SAMHSA access and created a new certificate per environment for internal testing.
What Should Reviewers Watch For?
If you're reviewing this PR, please check for these things in particular:
What Security Implications Does This PR Have?
Please indicate if this PR does any of the following:
Adds any new software dependencies
Modifies any security controls
Adds new transmission or storage of data
Any other changes that could possibly affect security?
I have considered the above security implications as it relates to this PR. (If one or more of the above apply, it cannot be merged without the ISSO or team security engineer's (
@sb-benohe) approval.)This change does affect security controls.
Validation
Have you fully verified and tested these changes? Is the acceptance criteria met? Please provide reproducible testing instructions, code snippets, or screenshots as applicable.