Added Contrast Security framework support #446
Conversation
|
Hey donniepropst! Thanks for submitting this pull request! All pull request submitters and commit authors must have a Contributor License Agreement (CLA) on-file with us. Please sign the appropriate CLA (individual or corporate). When sending signed CLA please provide your github username in case of individual CLA or the list of github usernames that can make pull requests on behalf of your organization. If you are confident that you're covered under a Corporate CLA, please make sure you've publicized your membership in the appropriate Github Org, per these instructions. Once you've publicized your membership, one of the owners of this repository can close and reopen this pull request, and dreddbot will take another look. |
|
@donniepropst I'll start a review on this today, but please get started on the CLA so that we merge as soon as the technical changes are complete. |
config/contrast_security_agent.yml
Outdated
|
|
||
| # Configuration for the ContrastSecurity framework | ||
| --- | ||
| enabled: true |
There was a problem hiding this comment.
There should be no need for an enabled flag on the component. Enable/Disable is achieved by either binding or not binding a service to the application.
|
|
||
| <table> | ||
| <tr> | ||
| <td><strong>Detection Criterion</strong></td><td>Existence of a single bound Contrast Security service. The existence of an Contrast Security service defined by the <a href="http://docs.cloudfoundry.org/devguide/deploy-apps/environment-variable.html#VCAP-SERVICES"><code>VCAP_SERVICES</code></a> payload containing a service name, label or tag with <code>contrast-security</code> or <code>contrastsecurity</code> as a substring. |
There was a problem hiding this comment.
Is there a reason you're supporting two forms of the service name/label/tag? Since this the first time we've ever integrated, you should pick a single, canonical, representation that you'd like to search for and that should become the truth. Everywhere else you might see a pair in existing integrations, it's a historical artifact where changes have been made.
| Tags are printed to standard output by the buildpack detect script | ||
|
|
||
| ## User-Provided Service | ||
| When binding ContrastSecurity using a user-provided service, it must have name or tag with `contrast-security` or `contrastsecurity` in it. The credential payload can contain the following entries: |
| | Name | Description | ||
| | ---- | ----------- | ||
| | `teamserver_url` | (Optional) The base URL in which your user has access to and the URL to which the Agent will report. ex: https://app.contrastsecurity.com | ||
| | `username` | (Required) The account name to use when downloading the agent |
There was a problem hiding this comment.
No need for (Required). The absence of (Optional) is sufficient.
| | ---- | ----------- | ||
| | `teamserver_url` | (Optional) The base URL in which your user has access to and the URL to which the Agent will report. ex: https://app.contrastsecurity.com | ||
| | `username` | (Required) The account name to use when downloading the agent | ||
| | `org_uuid` | (Required) The org uuid to send app information to, this is the org that your bound application will appear within |
There was a problem hiding this comment.
No need for (Required). The absence of (Optional) is sufficient.
| java_opts.add_system_property('contrast.dir', '/tmp') | ||
| java_opts.add_system_property('contrast.override.appname', app_name) | ||
| path = java_opts.qualify_path(@droplet.sandbox) | ||
| java_opts.add_preformatted_options("-javaagent:#{path}/#{boot_class_name}=#{path}/contrast.config") |
There was a problem hiding this comment.
Hmm... We really need to get an add_javaagent_with_props added. Not your responsibility.
|
|
||
| # (see JavaBuildpack::Component::VersionedDependencyComponent#supports?) | ||
| def supports? | ||
| @application.services.one_service?(CONTRAST_FILTER, [TEAMSERVER_URL, USERNAME, API_KEY, SERVICE_KEY]) |
There was a problem hiding this comment.
This is a bit unclear, but putting these keys in an array means "or". If you want "and" (and I I'm pretty sure you do), it should just be comma delimited. @application.services.one_service?(CONTRAST_FILTER, TEAMSERVER_URL, USERNAME, API_KEY, SERVICE_KEY).
| private | ||
|
|
||
| CONTRAST_FILTER = /contrast[-]?security/ | ||
| private_constant :CONTRAST_FILTER |
There was a problem hiding this comment.
All of these constants should be alphabetized and added to private_constants.
|
|
||
| private | ||
|
|
||
| CONTRAST_FILTER = /contrast[-]?security/ |
There was a problem hiding this comment.
Since this the first time we've ever integrated, you should pick a single, canonical, representation that you'd like to search for and that should become the truth.
| "contrast-engine-#{version}.jar" | ||
| end | ||
|
|
||
| def build_contrast_configuration |
There was a problem hiding this comment.
I'll leave this one to you to decide, but making a real live XML document might not be the easiest or clearest way to do this. Another option is to use extended literals and just write clear text that you know to be valid XML. An example. It's up to you though, and if you think this is easier, I'm happy to leave it as-is.
|
@nebhale thanks for the quick review, I'm hoping to work on the fixes you mentioned later today or early tomorrow, and will touch base with product/legal teams in regard to the CLA. |
|
@nebhale I believe I resolved all your comments with the exception of 1.) How I built the xml In our repo the jar is packaged with a I believe the CLA should be submitted today if it wasn't yesterday. |
|
@donniepropst Just trying to get a better understanding of the JAR naming requirement. Why does the agent care what the name of the JAR is? Is it used for more than just an opaque file path passed to |
|
@nebhale After talking with one of our Java agent engineers they said the name has to match because the JVM inspects the manifest file and the This was a requirement I didn't realize when I was working on the buildpack and calling download_jar without any parameters. The jar would get downloaded, but would have a bunch of class not found exceptions from our agent upon startup because the name of the file as stored in our repo is The unzipped |
|
OK, understood. We'll just wait on the CLA and then everything should be good to go. |
|
Checked in with our product team and it looks like the CLA was accepted last Wednesday so we should be good |
|
Hey donniepropst! Thanks for submitting this pull request! All pull request submitters and commit authors must have a Contributor License Agreement (CLA) on-file with us. Please sign the appropriate CLA (individual or corporate). When sending signed CLA please provide your github username in case of individual CLA or the list of github usernames that can make pull requests on behalf of your organization. If you are confident that you're covered under a Corporate CLA, please make sure you've publicized your membership in the appropriate Github Org, per these instructions. Once you've publicized your membership, one of the owners of this repository can close and reopen this pull request, and dreddbot will take another look. |
This change adds the Contrast Security Framework which detects a service with name/label/tag of contrast-security. [#446]
|
Hey donniepropst! Thanks for submitting this pull request! All pull request submitters and commit authors must have a Contributor License Agreement (CLA) on-file with us. Please sign the appropriate CLA (individual or corporate). When sending signed CLA please provide your github username in case of individual CLA or the list of github usernames that can make pull requests on behalf of your organization. If you are confident that you're covered under a Corporate CLA, please make sure you've publicized your membership in the appropriate Github Org, per these instructions. Once you've publicized your membership, one of the owners of this repository can close and reopen this pull request, and dreddbot will take another look. |
|
@donniepropst When you get a moment can you please ensure that you've got the proper GitHub organization publicized? |
|
@nebhale that was probably the issue. It is public now. |
|
Hey donniepropst! Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA. |
|
Looks good. Closing as merged now. |
This commit adds support for using Contrast Security's Java agent.
A bound service will be used to create a configuration file for the agent, as well as set some Java system properties.