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.

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 official Docker image and optional CORS filter #225

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

srstsavage
Copy link
Contributor

@srstsavage srstsavage commented Nov 1, 2024

Description

  • Added official ERDDAP Dockerfile, adapted from existing Dockerfiles provided by @roje-bodc and @MattHopsonNOC and Axiom Data Science, which builds local source code by default but can also check out and build branches/tags from arbitrary git remotes
  • Added example docker-compose.yml file
  • Added optional CORS filter (disabled by default) with configurable allowed origins and headers (settings enableCors, corsAllowHeaders, corsAllowOrigin)
  • Added deployment info setting and set value to docker in the Dockerfile (setting deploymentInfo, settable by user like any other seting). deployment_info is viewable in the json format response of the ERDDAP version endpoint /erddap/version which can be requested by setting query parameter format=json or header Accept: application/json.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist before requesting a review

  • I have performed a self-review of my code
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@srstsavage
Copy link
Contributor Author

Needs further testing but is ready for a draft review.

One thing not addressed here is SSL/TLS/HTTPS. Users wanting to configure Tomcat for HTTPS directly will need to mount an alternate server.xml file, and relevant configurations are well covered in the Tomcat documentation. Also it seems much more common these days to handle TLS in a web proxy layer (Apache, nginx, etc), and the configuration is much more ergonomic in those systems. The Docker Compose stack could potentially be extended to include nginx-proxy and acme-companion as examples of how to add automated letsencrypt certificates.

@srstsavage srstsavage force-pushed the docker branch 2 times, most recently from 803852b to f369605 Compare November 1, 2024 16:09
@srstsavage
Copy link
Contributor Author

Another piece to figure out: on the Axiom docker-erddap repo, @ocefpaf asked that some public indication that an ERDDAP is running in Docker be added for auditing purposes.

axiom-data-science/docker-erddap#90

Since the plan is to move to an official ERDDAP docker image, that should be implemented in this repo. If the Axiom image continues to be maintained with experimental features it will inherit from the official base image.

@ChrisJohnNOAA Any thoughts on adding such an indicator? It could only live on /erddap/status.html, or on every footer version string, etc. It would be trivial to add a step to the Dockerfile adding (Docker) or similar to the end of the version string (e.g. ERDDAP, Version 2.25 (Docker).

@ChrisJohnNOAA
Copy link
Contributor

Another piece to figure out: on the Axiom docker-erddap repo, @ocefpaf asked that some public indication that an ERDDAP is running in Docker be added for auditing purposes.

axiom-data-science/docker-erddap#90

Since the plan is to move to an official ERDDAP docker image, that should be implemented in this repo. If the Axiom image continues to be maintained with experimental features it will inherit from the official base image.

@ChrisJohnNOAA Any thoughts on adding such an indicator? It could only live on /erddap/status.html, or on every footer version string, etc. It would be trivial to add a step to the Dockerfile adding (Docker) or similar to the end of the version string (e.g. ERDDAP, Version 2.25 (Docker).

I think having the (Docker) in the footer version string would be good.

@ChrisJohnNOAA
Copy link
Contributor

Needs further testing but is ready for a draft review.

One thing not addressed here is SSL/TLS/HTTPS. Users wanting to configure Tomcat for HTTPS directly will need to mount an alternate server.xml file, and relevant configurations are well covered in the Tomcat documentation. Also it seems much more common these days to handle TLS in a web proxy layer (Apache, nginx, etc), and the configuration is much more ergonomic in those systems. The Docker Compose stack could potentially be extended to include nginx-proxy and acme-companion as examples of how to add automated letsencrypt certificates.

This (SSL/https configuration) should definitely be at least explained in a readme. I want to look more into it, but I'm wondering if the default should be a setup with ssl/https (one of my goals is to make it easy for folks to get a well configured secure server up and running), and then an advanced configuration for setups with a web proxy layer that handles the ssl (even if "advanced" is what 90% of people end up using).

Also just for clarification, is the cors configuration here needed because it's assumed to be behind a web proxy or is there another reason it's needed?

@rmendels
Copy link
Collaborator

rmendels commented Nov 1, 2024

I haven't looked at the cors configuration in the Docker image, but I can tell you that any site scanned by either Qualys or Nessus scans, which include at least most NOAA sites, will have that flagged immediately, and if memory serves as a high-risk vulnerability with only a short time-line to remedy it. I think the best default setup is CORS disabled, with instructions on how to enable it if needed and/or desired, and those instructions should also point out that it can create a vulnerability and failure to pass security scans.

As an aside, the test implementation of ERDDAP 2.25 passed both sets of scans.

@srstsavage
Copy link
Contributor Author

wondering if the default should be a setup with ssl/https

@ChrisJohnNOAA Are you thinking of having a self-signed cert be used as the default if an external cert isn't provided? I think the easiest way to get users to proper SSL if they're not handling it in a web proxy or buying/generating a cert out of band is to provide a letsencrypt example using an HTTP-01 challenge. That should be doable in a docker-compose.yml example using nginx-proxy and acme-companion. And maybe the fallback is a self signed cert?

@rmendels Absolutely CORS will be disabled by default. I think it's helpful to have it available because there are a growing number of in-browser apps which pull data from ERDDAP APIs directly and require CORS to be configured. I will add some options to limit the allowed domains which will hopefully appease the scanners somewhat when it's enabled.

@ChrisJohnNOAA
Copy link
Contributor

@ChrisJohnNOAA Are you thinking of having a self-signed cert be used as the default if an external cert isn't provided? I think the easiest way to get users to proper SSL if they're not handling it in a web proxy or buying/generating a cert out of band is to provide a letsencrypt example using an HTTP-01 challenge. That should be doable in a docker-compose.yml example using nginx-proxy and acme-companion. And maybe the fallback is a self signed cert?

I hadn't thought through the how to do it yet. Your idea to provide a letsencrypt example sounds good. If the goal is to be as simple to setup as possible, it'd be better to not require the users provide a cert.

@srstsavage
Copy link
Contributor Author

@ChrisJohnNOAA If possible I'd like to split the SSL/letsencrypt example to a separate issue. I have the Docker image and CORS support in a fairly good place (pending a few small updates) and IMO the cert management is related but enough of a separate concern to handle in its own PR. Thoughts?

Copy link
Contributor

@ChrisJohnNOAA ChrisJohnNOAA left a comment

Choose a reason for hiding this comment

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

A separate change for ssl/let's encrypt sounds good.

@ChrisJohnNOAA
Copy link
Contributor

I believe you need to make EDStatic.enableCors not final so that you can set it in tests.

@srstsavage
Copy link
Contributor Author

srstsavage commented Jan 17, 2025 via email

@srstsavage srstsavage force-pushed the docker branch 12 times, most recently from 8457375 to 17985df Compare January 25, 2025 17:41
@srstsavage srstsavage marked this pull request as ready for review January 25, 2025 17:43
@srstsavage
Copy link
Contributor Author

Ok, this is ready for final review/merging.

Summary of changes:

  • Added official ERDDAP Dockerfile, adapted from existing Dockerfiles, which builds local source code by default but can also check out and build branches/tags from arbitrary git remotes
  • Added example docker-compose.yml file
  • Added optional CORS filter (disabled by default) with configurable allowed origins and headers (settings enableCors, corsAllowHeaders, corsAllowOrigin)
  • Added deployment info setting and set value to docker in the Dockerfile (setting deploymentInfo, settable by user like any other seting). deployment_info is viewable in the json format response of the ERDDAP version endpoint /erddap/version which can be requested by setting query parameter format=json or header Accept: application/json.

@srstsavage
Copy link
Contributor Author

@ocefpaf Please see previous comment for a description of a new deployment metadata setting which the official Dockerfile will set to docker by default. Pending approval of this MR, the setting can be viewed by requesting a new json format of the ERDDAP version response:

$ curl "http://estuaries03.axiomptk:8080/erddap/version"
ERDDAP_version=2.25
$ curl "http://estuaries03.axiomptk:8080/erddap/version?format=json"
{
  "version": "2.25",
  "version_full": "2.25_1",
  "deployment_info": "docker"
}
$ curl -H "Accept: application/json" "http://estuaries03.axiomptk:8080/erddap/version"
{
  "version": "2.25",
  "version_full": "2.25_1",
  "deployment_info": "docker"
}

…setting

Adapts development Dockerfile from @roje-bodc and @MattHopsonNOC
and Axiom Dockerfile to a new official ERDDAP Docker image.

Also adds an optional internally managed CORS filter (enabled
by setting `enableCors=true` in settings).

CORS filter behavior can be modified using settings
variables corsAllowHeaders and corsAllowOrigin.

Also adds deploymentInfo setting and a json format of ERDDAP
version information, which can be used to add
deployment metadata (e.g. docker) viewable in
an ERDDAP version json response (set header
"Accept: application/json" or query parameter "format=json"
to /erddap/version
@ChrisJohnNOAA
Copy link
Contributor

@ocefpaf Please see previous comment for a description of a new deployment metadata setting which the official Dockerfile will set to docker by default. Pending approval of this MR, the setting can be viewed by requesting a new json format of the ERDDAP version response:

$ curl "http://estuaries03.axiomptk:8080/erddap/version"
ERDDAP_version=2.25
$ curl "http://estuaries03.axiomptk:8080/erddap/version?format=json"
{
  "version": "2.25",
  "version_full": "2.25_1",
  "deployment_info": "docker"
}
$ curl -H "Accept: application/json" "http://estuaries03.axiomptk:8080/erddap/version"
{
  "version": "2.25",
  "version_full": "2.25_1",
  "deployment_info": "docker"
}

I'm also planning to add this information as a Prometheus info metric.

@ChrisJohnNOAA ChrisJohnNOAA merged commit fe58ddb into ERDDAP:main Jan 27, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants