Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add metadata parameter to CodeArtifact api #109

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

add metadata parameter to CodeArtifact api #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c532d3e
Add metadata parameter to CodeArtifact api
seschis Nov 4, 2021
1e28f20
fix formatting and interface from PR comments
seschis Nov 17, 2021
f376c92
formatting
seschis Nov 17, 2021
08db808
fix stream pattern in test
seschis Nov 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/main/java/com/contrastsecurity/sdk/scan/CodeArtifact.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
* #L%
*/

import com.contrastsecurity.sdk.internal.Nullable;
import java.time.Instant;

/**
Expand All @@ -40,6 +41,10 @@ public interface CodeArtifact {
/** @return filename */
String filename();

/** @return metadata filename */
@Nullable
String metadata();

/** @return time at which the code artifact was uploaded to Contrast Scan */
Instant createdTime();
}
20 changes: 19 additions & 1 deletion src/main/java/com/contrastsecurity/sdk/scan/CodeArtifactClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
interface CodeArtifactClient {

/**
* Transfers a file from the file system to Contrast Scan to create a new code artifact for
* Transfers an artifact from the file system to Contrast Scan to create a new code artifact for
* analysis.
*
* @param projectId ID of the project to which the code artifact belongs
Expand All @@ -51,4 +51,22 @@ interface CodeArtifactClient {
* @throws ServerResponseException when Contrast API returns a response that cannot be understood
*/
CodeArtifactInner upload(String projectId, Path file) throws IOException;

/**
* Transfers artifact and prescan metadata from the file system to Contrast Scan to create a new
* code artifact for analysis.
*
* <p>Prescan metadata will allow the scanner to produce more detailed finding reports.
*
* @param projectId ID of the project to which the code artifact belongs
* @param file the file to upload
* @param metadata the prescan metadata to upload with the file artifact.
* @return new {@link CodeArtifactInner} from Contrast API
* @throws IOException when an IO error occurs while making the request to the Contrast API
* @throws UnauthorizedException when Contrast rejects the credentials used to send the request
* @throws ResourceNotFoundException when the requested resource does not exist
* @throws HttpResponseException when Contrast rejects this request with an error code
* @throws ServerResponseException when Contrast API returns a response that cannot be understood
*/
CodeArtifactInner upload(String projectId, Path file, Path metadata) throws IOException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API asks that the user provide the metadata JSON as a file. Maybe passing the metadata as a file is the only use case we have now (i.e. the Maven plugin), but it doesn't feel like the most flexible assumption for the SDK. I would expect the SDK to accept the metadata as a Java object that describes the data structure, and I would expect the SDK to marshal that object to JSON for me.

Are the fields in the metadata JSON object well known, or is this more of an open-ended bag of key-value pairs? If it's the former, then we should define a new class for it. If it's the latter, then maybe all we need is a Map.

Copy link
Author

@seschis seschis Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the format of the data is defined here: https://github.com/Contrast-Security-OSS/contrast-scan-prescan/blob/master/src/main/resources/schema/scan-input-metadata-schema-1.0.0.json

I don't anticipate it changing from json, but I can't say it never will. I also don't know how the content of the data will be required to change over time. The main purpose of the prescan metadata is to allow the engine to generate physical absolute paths to files in its sarif report which GitHub uses as part of preview renderings when displaying the sarif findings.
It also allows local sarif viewers, like the one in VSCode to naturally find the right file locally and show code annotated with the data flow path in the editor directly.

Copy link
Contributor

@gilday gilday Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. Knowing that has a well-defined schema, I feel more strongly that we should make a corresponding Java class to hold this data, but I don't fully understand what expectation you have for users who wan to use this API. How do they know how to generate this file?

}
48 changes: 42 additions & 6 deletions src/main/java/com/contrastsecurity/sdk/scan/CodeArtifactClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,17 @@ final class CodeArtifactClientImpl implements CodeArtifactClient {

@Override
public CodeArtifactInner upload(final String projectId, final Path file) throws IOException {
return sendRequest(projectId, file, null);
}

@Override
public CodeArtifactInner upload(final String projectId, final Path file, final Path metadata)
throws IOException {
return sendRequest(projectId, file, Objects.requireNonNull(metadata));
}

private CodeArtifactInner sendRequest(
final String projectId, final Path file, final Path metadata) throws IOException {
final String uri =
contrast.getRestApiURL()
+ new URIBuilder()
Expand All @@ -66,9 +77,9 @@ public CodeArtifactInner upload(final String projectId, final Path file) throws
"code-artifacts")
.toURIString();
final String boundary = "ContrastFormBoundary" + ThreadLocalRandom.current().nextLong();
final String header =
"--"
+ boundary
final String boundaryMarker = CRLF + "--" + boundary;
final String filenameSection =
boundaryMarker
+ CRLF
+ "Content-Disposition: form-data; name=\"filename\"; filename=\""
+ file.getFileName().toString()
Expand All @@ -80,8 +91,28 @@ public CodeArtifactInner upload(final String projectId, final Path file) throws
+ "Content-Transfer-Encoding: binary"
+ CRLF
+ CRLF;
final String footer = CRLF + "--" + boundary + "--" + CRLF;
final long contentLength = header.length() + Files.size(file) + footer.length();
final String metadataSection =
metadata != null
? boundaryMarker
+ CRLF
+ "Content-Disposition: form-data; name=\"metadata\"; filename=\""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we know this form is of type application/json, shouldn't we send it as such instead of sending it as a file? This relates to my question in CodeArtifactClient which is "how does the user know how to build this file?"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this gets back to the API design of the REST endpoint which I did not design. Let me see if I can provide some clarity on a few of your questions for trying to figure the best way to proceed.

The JSON spec I linked to (https://github.com/Contrast-Security-OSS/contrast-scan-prescan/blob/master/src/main/resources/schema/scan-input-metadata-schema-1.0.0.json) is an ingest specification for the scanner engine itself. It's saying, that if you want to provide the scanner engine some "prescan data" this is the format it will accept it in. Its published this way because I just wanted a specification for the input format rather than telling someone, "go read the code", and I also wanted to use code generators like jsonschema2pojo, to automatically generate the libraries to handle the data models of the scanner's prescan ingest format.

  • The scanner makes no promises that this format will always be supported, though we will try to keep it backwards compat best as possible, in all future versions of the scanner engine. The current format represents the simplest format to satisfy the current requirements given the current knowledge of the domain, which we know is not complete.
  • The REST API in front of the scanner engine doesn't do anything with this data specifically and merely passes it along to the scanner engine. From its point of view, it's opaque.
  • If the REST api wanted to validate the data before passing to the engine since it knows what engine version it bundles, its free to do that with the publish json schema, but the engine makes no assumptions that will occur.
  • For "contrast tools" created to generate prescan data as part of the scan request, they have a well defined json-schema to use to get the right data to the engine. But once again, the engine makes no assumptions that it is guarded behind a REST api or anything thing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For "contrast tools" created to generate prescan data as part of the scan request

Is this SDK an example of such a "contrast tool", or does the SDK user have to use another tool to generate the file first before they can use the SDK to include in their new code artifacts?

+ metadata.getFileName().toString()
+ '"'
+ CRLF
+ "Content-Type: "
+ determineMime(metadata)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we determine the MIME this way if we always know that it is JSON?

+ CRLF
+ "Content-Transfer-Encoding: binary"
+ CRLF
+ CRLF
: "";

final String footer = boundaryMarker + "--" + CRLF;
long contentLength = filenameSection.length() + Files.size(file);
if (metadata != null) {
contentLength += metadataSection.length() + Files.size(metadata);
}
contentLength += footer.length();

final HttpURLConnection connection = contrast.makeConnection(uri, "POST");
connection.setDoOutput(true);
Expand All @@ -91,9 +122,14 @@ public CodeArtifactInner upload(final String projectId, final Path file) throws
try (OutputStream os = connection.getOutputStream();
PrintWriter writer =
new PrintWriter(new OutputStreamWriter(os, StandardCharsets.US_ASCII), true)) {
writer.append(header).flush();
writer.append(filenameSection).flush();
Files.copy(file, os);
os.flush();
if (metadata != null) {
writer.append(metadataSection).flush();
Files.copy(metadata, os);
os.flush();
}
writer.append(footer).flush();
}
final int code = connection.getResponseCode();
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/com/contrastsecurity/sdk/scan/CodeArtifactImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ public String filename() {
return inner.filename();
}

@Override
public String metadata() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the metadata is a String, does that imply that the SDK user is not intended to parse this structured data; rather, they should treat it as an opaque box? Specifically, is this JSON-encoded JSON inside this JSON metadata property and it's not meant to be decoded?

Copy link
Author

@seschis seschis Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I always considered the generation of prescan data something that was external to the SDK.... but it probably would be more user-friendly to allow the SDK user to call a function that generated prescan data for them.... or perhaps just do it transparently.
In either case, it seems like an opaque thing to the SDK user.

return inner.metadata();
}

@Override
public Instant createdTime() {
return inner.createdTime();
Expand Down
8 changes: 8 additions & 0 deletions src/main/java/com/contrastsecurity/sdk/scan/CodeArtifactInner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
* #L%
*/

import com.contrastsecurity.sdk.internal.Nullable;
import com.google.auto.value.AutoValue;
import java.time.Instant;

Expand All @@ -44,6 +45,10 @@ static Builder builder() {
/** @return filename */
abstract String filename();

/** @return metadata filename */
@Nullable
abstract String metadata();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this imply that the user can retrieve the metadata filename, but not the metadata? What would the user do with the filename?


/** @return time at which the code artifact was uploaded to Contrast Scan */
abstract Instant createdTime();

Expand All @@ -63,6 +68,9 @@ abstract static class Builder {
/** @see CodeArtifactInner#filename() */
abstract Builder filename(String value);

/** @see CodeArtifactInner#metadata() */
abstract Builder metadata(String value);

/** @see CodeArtifactInner#createdTime() */
abstract Builder createdTime(Instant value);

Expand Down
32 changes: 32 additions & 0 deletions src/main/java/com/contrastsecurity/sdk/scan/CodeArtifacts.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,23 @@ interface Factory {
*/
CodeArtifact upload(Path file, String name) throws IOException;

/**
* Transfers a file from the file system to Contrast Scan to create a new code artifact for static
* analysis.
*
* @param file the code artifact to upload
* @param name the name of the code artifact
* @param metadata the path of the prescan data file to upload
* @param metaname the name of the prescan data file
* @return new {@link CodeArtifact} from Contrast
* @throws IOException when an IO error occurs while making the request to the Contrast API
* @throws UnauthorizedException when Contrast rejects the credentials used to send the request
* @throws ResourceNotFoundException when the requested resource does not exist
* @throws HttpResponseException when Contrast rejects this request with an error code
* @throws ServerResponseException when Contrast API returns a response that cannot be understood
*/
CodeArtifact upload(Path file, String name, Path metadata, String metaname) throws IOException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is metaname here? I don't think it's used.

Copy link
Author

@seschis seschis Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

metaname is analagous to the name argument the way metadata is analogous to the file argument. As to if that is even needed, I'm not sure. I don't see why we'd give the user the capability to change the "filename" on the multipart upload... but I figured it was something I didn't understand so I just mirrored the behavior when allowing the prescan metadata to be added to the multipart file upload. It's your call if you want me to take it out as I don't have a valid argument for or against it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iirc the filename of the upload is reflected in the UI, and that's why we give the user the capability to set that. I don't see a use case for allowing the user to set the name of the metadata file. I'd argue we take that capability out. Also, I'm still questioning whether the metadata should be sent as a file vs as JSON, but I'll continue that discussion in another thread.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I see now. I can take it out. Before I do any work on it yet though I'll wait until we resolve the fundamental API design questions you have in this area.


/**
* Transfers a file from the file system to Contrast Scan to create a new code artifact for static
* analysis.
Expand All @@ -75,4 +92,19 @@ interface Factory {
* @throws ServerResponseException when Contrast API returns a response that cannot be understood
*/
CodeArtifact upload(Path file) throws IOException;

/**
* Transfers a file from the file system to Contrast Scan to create a new code artifact for static
* analysis.
*
* @param file the code artifact to upload
* @param metadata the path of the prescan data file to upload
* @return new {@link CodeArtifact} from Contrast
* @throws IOException when an IO error occurs while making the request to the Contrast API
* @throws UnauthorizedException when Contrast rejects the credentials used to send the request
* @throws ResourceNotFoundException when the requested resource does not exist
* @throws HttpResponseException when Contrast rejects this request with an error code
* @throws ServerResponseException when Contrast API returns a response that cannot be understood
*/
CodeArtifact upload(Path file, Path metadata) throws IOException;
}
13 changes: 13 additions & 0 deletions src/main/java/com/contrastsecurity/sdk/scan/CodeArtifactsImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,14 @@ public CodeArtifacts create(final String projectId) {
this.projectId = projectId;
}

@Override
public CodeArtifact upload(
final Path file, final String name, final Path metadata, final String metaname)
throws IOException {
final CodeArtifactInner inner = client.upload(projectId, file, metadata);
return new CodeArtifactImpl(inner);
}

@Override
public CodeArtifact upload(final Path file, final String name) throws IOException {
final CodeArtifactInner inner = client.upload(projectId, file);
Expand All @@ -60,4 +68,9 @@ public CodeArtifact upload(final Path file, final String name) throws IOExceptio
public CodeArtifact upload(final Path file) throws IOException {
return upload(file, file.getFileName().toString());
}

@Override
public CodeArtifact upload(final Path file, final Path metadata) throws IOException {
return upload(file, file.getFileName().toString(), metadata, metadata.getFileName().toString());
}
}
1 change: 1 addition & 0 deletions src/test/java/com/contrastsecurity/sdk/scan/CodeArtifactAssert.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ public CodeArtifactAssert hasSameValuesAsInner(final CodeArtifactInner inner) {
Assertions.assertThat(actual.projectId()).isEqualTo(inner.projectId());
Assertions.assertThat(actual.organizationId()).isEqualTo(inner.organizationId());
Assertions.assertThat(actual.filename()).isEqualTo(inner.filename());
Assertions.assertThat(actual.metadata()).isEqualTo(inner.metadata());
Assertions.assertThat(actual.createdTime()).isEqualTo(inner.createdTime());
return this;
}
Expand Down
35 changes: 35 additions & 0 deletions src/test/java/com/contrastsecurity/sdk/scan/CodeArtifactsImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,23 @@ void upload(@TempDir final Path tmp) throws IOException {
assertThat(codeArtifact).hasSameValuesAsInner(inner);
}

@Test
void upload_with_metadata(@TempDir final Path tmp) throws IOException {
// GIVEN stubbed code artifacts client
final CodeArtifactClient client = mock(CodeArtifactClient.class);
final CodeArtifactInner inner = builder().metadata("prescan.json").build();
final Path file = tmp.resolve(inner.filename());
final Path meta = tmp.resolve(inner.metadata());
when(client.upload(inner.projectId(), file, meta)).thenReturn(inner);

// WHEN upload file,meta
final CodeArtifacts codeArtifacts = new CodeArtifactsImpl(client, inner.projectId());
final CodeArtifact codeArtifact = codeArtifacts.upload(file, meta);

// THEN returns expected code artifact
assertThat(codeArtifact).hasSameValuesAsInner(inner);
}

@Test
void upload_custom_filename(@TempDir final Path tmp) throws IOException {
// GIVEN stubbed code artifacts client
Expand All @@ -69,6 +86,24 @@ void upload_custom_filename(@TempDir final Path tmp) throws IOException {
assertThat(codeArtifact).hasSameValuesAsInner(inner);
}

@Test
void upload_custom_metaname(@TempDir final Path tmp) throws IOException {
// GIVEN stubbed code artifacts client
final CodeArtifactClient client = mock(CodeArtifactClient.class);
final CodeArtifactInner inner = builder().metadata("prescan.json").build();
final Path file = tmp.resolve(inner.filename());
final Path meta = tmp.resolve("other-prescan.json");
when(client.upload(inner.projectId(), file, meta)).thenReturn(inner);

// WHEN upload file,meta
final CodeArtifacts codeArtifacts = new CodeArtifactsImpl(client, inner.projectId());
final CodeArtifact codeArtifact =
codeArtifacts.upload(file, inner.filename(), meta, inner.metadata());

// THEN returns expected code artifact
assertThat(codeArtifact).hasSameValuesAsInner(inner);
}

@Test
void delegates_to_inner() {
final CodeArtifactInner inner = builder().build();
Expand Down
Loading