Support ECF_SSL configuration for uw ecflow server#933
Open
AnnaSmoot-NOAA wants to merge 9 commits into
Open
Conversation
…cflow server - Add ECF_SSL_DIR as optional key in the ecflow server JSON schema - Refactor _provision_ssl() to accept optional ssl_dir parameter, falling back to $HOME/.ecflowrc/ssl when None - Update server() to read ECF_SSL_DIR from config and pass it to _provision_ssl(); key passes through to ecflow_server/client env - Update existing SSL tests to pass ssl_dir directly instead of patching the module-level constant - Add tests for custom ssl_dir forwarding and env passthrough - Add Server Configuration section to YAML docs covering ECF_SSL_DIR - Add CLI example for ECF_SSL_DIR usage and update --report JSON example Closes ufs-community#924
AnnaSmoot-NOAA
requested review from
christinaholtNOAA,
elcarpenterNOAA and
maddenp-cu
as code owners
There was a problem hiding this comment.
Pull request overview
Adds support for configuring a custom ecFlow SSL certificate directory for uw ecflow server via the optional ECF_SSL_DIR setting, while preserving the existing default behavior when unset.
Changes:
- Extend the ecFlow server config JSON schema to allow
ECF_SSL_DIR. - Refactor SSL provisioning to accept an optional
ssl_dirand wireECF_SSL_DIRthroughserver()into provisioning + server environment. - Update/add unit tests and user documentation to cover the new configuration option and CLI examples.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/uwtools/ecflow.py |
Adds _SSL_DEFAULT_DIR, refactors _provision_ssl(ssl_dir=...), and passes ECF_SSL_DIR from config into provisioning and env. |
src/uwtools/resources/jsonschema/ecflow.jsonschema |
Adds ECF_SSL_DIR as an optional server configuration key. |
src/uwtools/tests/test_ecflow.py |
Updates SSL provisioning tests to pass ssl_dir directly; adds server tests for forwarding + env passthrough. |
docs/sections/user_guide/yaml/ecflow.rst |
Documents the new ecflow: server: block section and ECF_SSL_DIR. |
docs/sections/user_guide/cli/tools/ecflow.rst |
Adds CLI usage example for ECF_SSL_DIR and updates --report JSON example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
maddenp-cu
reviewed
Jun 17, 2026
…r SSL generation Replace shell-string openssl calls with subprocess.run(list, check=False) to eliminate shell injection risk. Use os.umask(0o077)/restore in a try/finally instead of a shell umask prefix. Resolve the openssl executable via shutil.which() to satisfy S607 and fail fast if openssl is absent. Fix docs wording on --insecure behavior. Add test for _openssl() not-found branch.
maddenp-cu
reviewed
Jun 17, 2026
christinaholtNOAA
approved these changes
Jun 17, 2026
maddenp-cu
requested changes
Jun 17, 2026
- Shorten ECF_SSL_DIR description in JSON schema - Change _openssl() return type from str to Path - Switch SSL subprocess calls from subprocess_run to run_shell_cmd for consistency with uwtools conventions; embed umask 077 in shell command for thread safety - Correct docs: ECF_SSL_DIR is a uwtools config key, not a native ecFlow variable; remove incorrect claims about ecflow_client inheritance - Fix ecflow_client description: ecflow → ecFlow (capitalization) - Update tests to patch run_shell_cmd instead of subprocess_run
maddenp-cu
reviewed
Jun 18, 2026
maddenp-cu
reviewed
Jun 18, 2026
AnnaSmoot-NOAA
changed the title
maddenp-cu
reviewed
Jun 22, 2026
maddenp-cu
left a comment
maddenp-cu
left a comment
Collaborator
There was a problem hiding this comment.
There might be more updates related to the semantics of ECF_SSL needed, but these caught my eye.
maddenp-cu
approved these changes
Jun 22, 2026
maddenp-cu
left a comment
maddenp-cu
left a comment
Collaborator
There was a problem hiding this comment.
Thanks again for your patience and persistence implementing this tricky feature.
This file contains hidden or 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
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.
_check_ssl_named()helper to verify that a named certificate triplet (<host>.<port>.crt/.key/.pem) exists in$HOME/.ecflowrc/sslbefore starting the serverserver()to route to_check_ssl_named()whenECF_SSLis a<host>.<port>string, and to_provision_ssl()for the default case (boolean or absent)ECF_SSL: the string form is a<host>.<port>prefix, not a certificate file pathyaml/ecflow.rst,cli/tools/ecflow.rst): replace incorrect file-path examples with<host>.<port>examples; clarify that all cert files must reside in$HOME/.ecflowrc/ssl<host>.<port>format; add tests for_check_ssl_named()(all files present, missing files)Closes #924
Synopsis
Adds proper support for the
ECF_SSLconfig key inuw ecflow server. WhenECF_SSLis absent or true, the default certificate triplet (server.crt/server.key/dh2048.pem) is auto-provisioned in$HOME/.ecflowrc/ssl. WhenECF_SSLis a<host>.<port>string, the named triplet (<host>.<port>.crt/.key/.pem) is verified to exist there before the server starts. WhenECF_SSLisfalseor--insecureis given, SSL is disabled entirely. All cert files must reside in$HOME/.ecflowrc/ssl; no custom directory is supported by ecflow.Type
Impact
Checklist