From c2e221cccf7cba25eb010d7d58217d11961e79c9 Mon Sep 17 00:00:00 2001 From: Olivia Kotsopoulos Date: Mon, 1 Apr 2024 11:02:01 -0400 Subject: [PATCH] [WOR-1560] Upgrade to TCL 1.1.4 for MDC-aware Stairway (#427) * Revert "Revert "[WOR-1560] Upgrade to TCL 1.1.1 for MDC-aware Stairway (#418)" (#425)" This reverts commit 9220e781c194d3de1cc536cccc8eb1d0cf14adef. Reason: we are going to upgrade TCL to version k8s client at 20.0.1-legacy rather than 20.0.1. The rest of the commit that we originally reverted we'd like to apply. * Upgrade TCL to 1.1.4 This upgrade versions our transitive dependency on k8s client at 20.0.1-legacy -- the latest legacy version which moves past a number of vulnerabilities, but does not introduce breaking changes. * Do not useMavenLocal by default --- README.md | 35 +++++ buildSrc/build.gradle | 1 + ...rra.profile.java-common-conventions.gradle | 26 ++-- ...rra.profile.java-spring-conventions.gradle | 5 +- service/build.gradle | 5 +- .../app/common/MDCHandlingException.java | 27 ---- .../bio/terra/profile/app/common/MdcHook.java | 136 ------------------ .../terra/profile/service/job/JobBuilder.java | 11 +- .../terra/profile/service/job/JobService.java | 9 +- .../profile/ProfileServiceUnitTest.java | 6 +- 10 files changed, 59 insertions(+), 202 deletions(-) delete mode 100644 service/src/main/java/bio/terra/profile/app/common/MDCHandlingException.java delete mode 100644 service/src/main/java/bio/terra/profile/app/common/MdcHook.java diff --git a/README.md b/README.md index ae3c0379..be29cc69 100644 --- a/README.md +++ b/README.md @@ -76,5 +76,40 @@ Automatically fix linting issues: ./gradlew generateSwaggerCode ``` + +## SourceClear + +[SourceClear](https://srcclr.github.io) is a static analysis tool that scans a project's Java +dependencies for known vulnerabilities. If you are working on addressing dependency vulnerabilities +in response to a SourceClear finding, you may want to run a scan off of a feature branch and/or local code. + +### Github Action + +You can trigger BPM's SCA scan on demand via its +[Github Action](https://github.com/broadinstitute/dsp-appsec-sourceclear-github-actions/actions/workflows/z-manual-terra-billing-profile-manager.yml), +and optionally specify a Github ref (branch, tag, or SHA) to check out from the repo to scan. By default, +the scan is run off of BPM's `main` branch. + +High-level results are outputted in the Github Actions run. + +### Running Locally + +You will need to get the API token from Vault before running the Gradle `srcclr` task. + +```sh +export SRCCLR_API_TOKEN=$(vault read -field=api_token secret/secops/ci/srcclr/gradle-agent) +./gradlew srcclr +``` + +High-level results are outputted to the terminal. + +### Veracode + +Full results including dependency graphs are uploaded to +[Veracode](https://sca.analysiscenter.veracode.com/workspaces/jppForw/projects/551485/issues) +(if running off of a feature branch, navigate to Project Details > Selected Branch > Change to select your feature branch). +You can request a Veracode account to view full results from #dsp-infosec-champions. + + ## Tech Stack BPM adheres to the [Terra Tech Stack](https://docs.google.com/document/d/1JkTrtaci7EI0TnuR-68zYgTx_mRCNu-u2eV9XhexWTI/edit#heading=h.5z6knaqygr4a). See linked document for relevant technology choices and the rationale behind their inclusion in this service. diff --git a/buildSrc/build.gradle b/buildSrc/build.gradle index d21f1c4f..84457574 100644 --- a/buildSrc/build.gradle +++ b/buildSrc/build.gradle @@ -17,6 +17,7 @@ dependencies { implementation 'org.hidetake.swagger.generator:org.hidetake.swagger.generator.gradle.plugin:2.19.2' implementation 'org.springframework.boot:spring-boot-gradle-plugin:3.2.3' implementation 'bio.terra:terra-test-runner:0.2.0-SNAPSHOT' + implementation 'com.srcclr.gradle:com.srcclr.gradle.gradle.plugin:3.1.12' // Transitive dependency constraints due to security vulnerabilities in prior versions. // These are not directly included, they are just constrained if they are pulled in as diff --git a/buildSrc/src/main/groovy/bio.terra.profile.java-common-conventions.gradle b/buildSrc/src/main/groovy/bio.terra.profile.java-common-conventions.gradle index 51c2a617..d4f3a45c 100644 --- a/buildSrc/src/main/groovy/bio.terra.profile.java-common-conventions.gradle +++ b/buildSrc/src/main/groovy/bio.terra.profile.java-common-conventions.gradle @@ -5,6 +5,7 @@ plugins { id 'com.diffplug.spotless' id 'org.hidetake.swagger.generator' + id 'com.srcclr.gradle' } boolean isCiServer = System.getenv().containsKey("CI") @@ -21,7 +22,12 @@ java { } } +// If true, search local repository (~/.m2/repository/) first for dependencies. +def useMavenLocal = false repositories { + if (useMavenLocal) { + mavenLocal() // must be listed first to take effect + } maven { // Terra proxy for maven central url 'https://broadinstitute.jfrog.io/broadinstitute/maven-central/' @@ -33,29 +39,15 @@ repositories { maven { url 'https://broadinstitute.jfrog.io/broadinstitute/libs-snapshot-local/' } - //mavenLocal() } dependencies { compileOnly "com.google.code.findbugs:annotations:3.0.1" implementation 'io.swagger.core.v3:swagger-annotations:2.1.12' + implementation 'io.opentelemetry.instrumentation:opentelemetry-instrumentation-annotations:2.2.0' swaggerCodegen 'io.swagger.codegen.v3:swagger-codegen-cli:3.0.47' implementation 'org.slf4j:slf4j-api' - - testImplementation 'ch.qos.logback:logback-classic' - testImplementation 'org.hamcrest:hamcrest:2.2' - - var openTelemetryVersion = '1.31.0' - implementation "io.opentelemetry:opentelemetry-api:${openTelemetryVersion}" - implementation "io.opentelemetry:opentelemetry-sdk:${openTelemetryVersion}" - implementation "io.opentelemetry:opentelemetry-sdk-metrics:${openTelemetryVersion}" - implementation "io.opentelemetry:opentelemetry-exporter-logging:${openTelemetryVersion}" - implementation "io.opentelemetry.semconv:opentelemetry-semconv:1.21.0-alpha" - implementation "io.opentelemetry.instrumentation:opentelemetry-spring-webmvc-6.0:${openTelemetryVersion}-alpha" - implementation "io.opentelemetry.instrumentation:opentelemetry-instrumentation-annotations:${openTelemetryVersion}" - implementation "io.opentelemetry.instrumentation:opentelemetry-spring-boot:${openTelemetryVersion}-alpha" - implementation "com.google.cloud.opentelemetry:exporter-trace:0.25.2" } // Modify the standard :test task to only include unit-tagged tests. @@ -106,3 +98,7 @@ jacocoTestReport { xml.required = true } } + +srcclr { + scope = "runtimeClasspath" +} diff --git a/buildSrc/src/main/groovy/bio.terra.profile.java-spring-conventions.gradle b/buildSrc/src/main/groovy/bio.terra.profile.java-spring-conventions.gradle index 810f48b6..ead852f7 100644 --- a/buildSrc/src/main/groovy/bio.terra.profile.java-spring-conventions.gradle +++ b/buildSrc/src/main/groovy/bio.terra.profile.java-spring-conventions.gradle @@ -6,5 +6,6 @@ plugins { id 'org.springframework.boot' } -// Spring Boot 3.1.2 pulls in postgresql 42.6.0, vulnerable to CVE-2024-1597 -ext['postgresql.version'] = '42.7.2' +// Spring Boot 3.2.3 pulls in opentelemetry-bom 1.31.0. +// It must have version >= 1.34.1 for compatibility with terra-common-lib 1.0.9: +ext['opentelemetry.version'] = '1.36.0' diff --git a/service/build.gradle b/service/build.gradle index da5fad1a..06a37be1 100644 --- a/service/build.gradle +++ b/service/build.gradle @@ -15,7 +15,7 @@ apply from: 'publishing.gradle' dependencies { implementation 'io.sentry:sentry:7.6.0' - implementation 'bio.terra:terra-common-lib:0.1.9-SNAPSHOT' + implementation 'bio.terra:terra-common-lib:1.1.4-SNAPSHOT' implementation 'org.apache.commons:commons-dbcp2' implementation 'org.springframework.boot:spring-boot-starter-data-jdbc' @@ -27,7 +27,8 @@ dependencies { implementation group: "org.apache.commons", name: "commons-lang3" implementation group: 'jakarta.ws.rs', name: 'jakarta.ws.rs-api', version: '3.1.0' implementation group: 'com.nimbusds', name: 'nimbus-jose-jwt', version: '9.37.3' - implementation 'org.springframework.boot:spring-boot-starter-actuator:2.7.18' + + implementation 'org.springframework.boot:spring-boot-starter-actuator' implementation 'io.micrometer:micrometer-registry-prometheus' // Google dependencies diff --git a/service/src/main/java/bio/terra/profile/app/common/MDCHandlingException.java b/service/src/main/java/bio/terra/profile/app/common/MDCHandlingException.java deleted file mode 100644 index fe0f5443..00000000 --- a/service/src/main/java/bio/terra/profile/app/common/MDCHandlingException.java +++ /dev/null @@ -1,27 +0,0 @@ -package bio.terra.profile.app.common; - -import bio.terra.common.exception.InternalServerErrorException; -import java.util.List; - -public class MDCHandlingException extends InternalServerErrorException { - - public MDCHandlingException(String message) { - super(message); - } - - public MDCHandlingException(String message, Throwable cause) { - super(message, cause); - } - - public MDCHandlingException(Throwable cause) { - super(cause); - } - - public MDCHandlingException(String message, List causes) { - super(message, causes); - } - - public MDCHandlingException(String message, Throwable cause, List causes) { - super(message, cause, causes); - } -} diff --git a/service/src/main/java/bio/terra/profile/app/common/MdcHook.java b/service/src/main/java/bio/terra/profile/app/common/MdcHook.java deleted file mode 100644 index c720b9cb..00000000 --- a/service/src/main/java/bio/terra/profile/app/common/MdcHook.java +++ /dev/null @@ -1,136 +0,0 @@ -package bio.terra.profile.app.common; - -import bio.terra.stairway.FlightContext; -import bio.terra.stairway.FlightMap; -import bio.terra.stairway.HookAction; -import bio.terra.stairway.StairwayHook; -import com.fasterxml.jackson.core.JsonProcessingException; -import com.fasterxml.jackson.core.type.TypeReference; -import com.fasterxml.jackson.databind.ObjectMapper; -import jakarta.annotation.Nullable; -import java.util.Map; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -import org.slf4j.MDC; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.stereotype.Component; - -/** - * A {@link StairwayHook} for propagating MDC context across steps using the input {@link - * FlightMap}. - * - *

This allows steps to have the same MDC context as when their flight was created. Note that any - * modifications to the MDC context within a step are not propagated to other steps. - */ -@Component -public class MdcHook implements StairwayHook { - private static final Logger logger = LoggerFactory.getLogger(MdcHook.class); - private static final String FLIGHT_LOG_FORMAT = "Operation: {}, flightClass: {}, flightId: {}"; - private static final String STEP_LOG_FORMAT = - "Operation: {}, flightClass: {}, flightId: {}, stepClass: {}, stepIndex: {}, direction: {}"; - - /** The key to use in {@link FlightMap} for storing the MDC context. */ - public static final String MDC_FLIGHT_MAP_KEY = "mdcKey"; - - private static final TypeReference> mapType = new TypeReference<>() {}; - - private final ObjectMapper objectMapper; - - @Autowired - public MdcHook(ObjectMapper objectMapper) { - this.objectMapper = objectMapper; - } - - /** - * Returns a serialized version of the current MDC context. - * - *

This is meant to be used to set up the initial {@link FlightMap} when a Flight is being - * created to propagate the context of the flight creation to the steps. - */ - public Object getSerializedCurrentContext() { - return serializeMdc(MDC.getCopyOfContextMap()); - } - - @Override - public HookAction startStep(FlightContext flightContext) { - logger.info( - STEP_LOG_FORMAT, - "startStep", - flightContext.getFlightClassName(), - flightContext.getFlightId(), - flightContext.getStepClassName(), - flightContext.getStepIndex(), - flightContext.getDirection().name()); - String serializedMdc = flightContext.getInputParameters().get(MDC_FLIGHT_MAP_KEY, String.class); - // Note that this destroys any previous context on this thread. - MDC.setContextMap(deserializeMdc(serializedMdc)); - return HookAction.CONTINUE; - } - - @Override - public HookAction endStep(FlightContext flightContext) { - logger.info( - STEP_LOG_FORMAT, - "endStep", - flightContext.getFlightClassName(), - flightContext.getFlightId(), - flightContext.getStepClassName(), - flightContext.getStepIndex(), - flightContext.getDirection().name()); - MDC.clear(); - return HookAction.CONTINUE; - } - - @Override - public HookAction startFlight(FlightContext flightContext) { - logger.info( - FLIGHT_LOG_FORMAT, - "startFlight", - flightContext.getFlightClassName(), - flightContext.getFlightId()); - return HookAction.CONTINUE; - } - - @Override - public HookAction endFlight(FlightContext flightContext) { - logger.info( - FLIGHT_LOG_FORMAT, - "endFlight", - flightContext.getFlightClassName(), - flightContext.getFlightId()); - return HookAction.CONTINUE; - } - - private String serializeMdc(@Nullable Map mdcMap) { - try { - return objectMapper.writeValueAsString(mdcMap); - } catch (JsonProcessingException e) { - throw new MDCHandlingException( - "Error serializing MDC map from string: " - + (mdcMap == null ? "null" : mdcMap.toString())); - } - } - - private Map deserializeMdc(@Nullable String serializedMdc) { - if (serializedMdc == null) { - return Map.of(); - } - Map mdcContext; - try { - mdcContext = objectMapper.readValue(serializedMdc, mapType); - } catch (JsonProcessingException e) { - throw new MDCHandlingException("Error deserializing MDC map from string: " + serializedMdc); - } - if (mdcContext == null) { - return Map.of(); - } - return mdcContext; - } - - @Override - public HookAction stateTransition(FlightContext context) throws InterruptedException { - logger.info( - "Flight ID {} changed status to {}.", context.getFlightId(), context.getFlightStatus()); - return HookAction.CONTINUE; - } -} diff --git a/service/src/main/java/bio/terra/profile/service/job/JobBuilder.java b/service/src/main/java/bio/terra/profile/service/job/JobBuilder.java index 3bd416fa..c666137b 100644 --- a/service/src/main/java/bio/terra/profile/service/job/JobBuilder.java +++ b/service/src/main/java/bio/terra/profile/service/job/JobBuilder.java @@ -4,7 +4,6 @@ import bio.terra.common.iam.AuthenticatedUserRequest; import bio.terra.common.stairway.MonitoringHook; import bio.terra.common.stairway.StairwayComponent; -import bio.terra.profile.app.common.MdcHook; import bio.terra.profile.service.job.exception.InvalidJobIdException; import bio.terra.profile.service.job.exception.InvalidJobParameterException; import bio.terra.stairway.Flight; @@ -17,7 +16,6 @@ public class JobBuilder { private final JobService jobService; private final StairwayComponent stairwayComponent; - private final MdcHook mdcHook; private final FlightMap jobParameterMap; private final OpenTelemetry openTelemetry; @Nullable private Class flightClass; @@ -27,13 +25,9 @@ public class JobBuilder { @Nullable private AuthenticatedUserRequest userRequest; public JobBuilder( - JobService jobService, - StairwayComponent stairwayComponent, - MdcHook mdcHook, - OpenTelemetry openTelemetry) { + JobService jobService, StairwayComponent stairwayComponent, OpenTelemetry openTelemetry) { this.jobService = jobService; this.stairwayComponent = stairwayComponent; - this.mdcHook = mdcHook; this.openTelemetry = openTelemetry; this.jobParameterMap = new FlightMap(); } @@ -109,8 +103,7 @@ private void populateInputParams() { jobId = stairwayComponent.get().createFlightId(); } - // Always add the MDC logging and tracing span parameters for the mdc hook - addParameter(MdcHook.MDC_FLIGHT_MAP_KEY, mdcHook.getSerializedCurrentContext()); + // Always add the tracing span parameters addParameter( MonitoringHook.SUBMISSION_SPAN_CONTEXT_MAP_KEY, MonitoringHook.serializeCurrentTracingContext(openTelemetry)); diff --git a/service/src/main/java/bio/terra/profile/service/job/JobService.java b/service/src/main/java/bio/terra/profile/service/job/JobService.java index 333168ef..51c268ee 100644 --- a/service/src/main/java/bio/terra/profile/service/job/JobService.java +++ b/service/src/main/java/bio/terra/profile/service/job/JobService.java @@ -3,8 +3,8 @@ import bio.terra.common.iam.AuthenticatedUserRequest; import bio.terra.common.stairway.MonitoringHook; import bio.terra.common.stairway.StairwayComponent; +import bio.terra.common.stairway.StairwayLoggingHook; import bio.terra.profile.app.common.ErrorReportUtils; -import bio.terra.profile.app.common.MdcHook; import bio.terra.profile.app.configuration.IngressConfiguration; import bio.terra.profile.app.configuration.JobConfiguration; import bio.terra.profile.app.configuration.StairwayDatabaseConfiguration; @@ -58,7 +58,6 @@ public class JobService { private final IngressConfiguration ingressConfig; private final StairwayDatabaseConfiguration stairwayDatabaseConfiguration; private final ScheduledExecutorService executor; - private final MdcHook mdcHook; private final StairwayComponent stairwayComponent; private final ApplicationContext context; private final Logger logger = LoggerFactory.getLogger(JobService.class); @@ -71,7 +70,6 @@ public JobService( JobConfiguration jobConfig, IngressConfiguration ingressConfig, StairwayDatabaseConfiguration stairwayDatabaseConfiguration, - MdcHook mdcHook, StairwayComponent stairwayComponent, ApplicationContext context, ObjectMapper objectMapper, @@ -80,7 +78,6 @@ public JobService( this.ingressConfig = ingressConfig; this.stairwayDatabaseConfiguration = stairwayDatabaseConfiguration; this.executor = Executors.newScheduledThreadPool(jobConfig.maxThreads()); - this.mdcHook = mdcHook; this.stairwayComponent = stairwayComponent; this.context = context; this.objectMapper = objectMapper; @@ -89,7 +86,7 @@ public JobService( // Fully fluent style of JobBuilder public JobBuilder newJob() { - return new JobBuilder(this, stairwayComponent, mdcHook, openTelemetry); + return new JobBuilder(this, stairwayComponent, openTelemetry); } // submit a new job to stairway @@ -165,7 +162,7 @@ public void initialize() { .newStairwayOptionsBuilder() .dataSource(stairwayDatabaseConfiguration.getDataSource()) .context(context) - .addHook(mdcHook) + .addHook(new StairwayLoggingHook()) .addHook(new MonitoringHook(openTelemetry)) .exceptionSerializer(new StairwayExceptionSerializer(objectMapper))); } diff --git a/service/src/test/java/bio/terra/profile/service/profile/ProfileServiceUnitTest.java b/service/src/test/java/bio/terra/profile/service/profile/ProfileServiceUnitTest.java index 38ea4363..fe5a84af 100644 --- a/service/src/test/java/bio/terra/profile/service/profile/ProfileServiceUnitTest.java +++ b/service/src/test/java/bio/terra/profile/service/profile/ProfileServiceUnitTest.java @@ -27,7 +27,6 @@ import bio.terra.policy.model.TpsPaoGetResult; import bio.terra.policy.model.TpsPolicyInput; import bio.terra.policy.model.TpsPolicyInputs; -import bio.terra.profile.app.common.MdcHook; import bio.terra.profile.common.BaseUnitTest; import bio.terra.profile.common.ProfileFixtures; import bio.terra.profile.db.ProfileDao; @@ -544,10 +543,7 @@ void createProfileFlightSetup() { when(billingCow.testIamPermissions(any())).thenReturn(iamPermissionsResponse); StairwayComponent stairwayComponent = mock(StairwayComponent.class); - var builder = - spy( - new JobBuilder( - jobService, stairwayComponent, mock(MdcHook.class), OpenTelemetry.noop())); + var builder = spy(new JobBuilder(jobService, stairwayComponent, OpenTelemetry.noop())); when(jobService.newJob()).thenReturn(builder); var profileDescription = ProfileFixtures.createGcpBillingProfileDescription("ABCD1234");