Skip to content

add mTLS for the salt api#69441

Merged
dwoz merged 3 commits into
saltstack:masterfrom
bochi:api-mtls
Jul 10, 2026
Merged

add mTLS for the salt api#69441
dwoz merged 3 commits into
saltstack:masterfrom
bochi:api-mtls

Conversation

@bochi

@bochi bochi commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds optional mutual TLS (client certificate validation) to the rest_cherrypy API
Three new rest_cherrypy options:

ssl_ca_certs — file or directory with the trust-anchor PEM certificates

  • ssl_cert_reqs — peer verification policy, one of CERT_NONE (default),
    CERT_OPTIONAL or CERT_REQUIRED.
  • ssl_allowed_cn — optional list of allowed Subject CN values.

What issues does this PR fix or reference?

Fixes
No fixes, its a feature

New Behavior

Support for mTLS Auth either in addition to existing auth or seperate.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

No

@bochi bochi requested a review from a team as a code owner June 12, 2026 18:32
@dwoz dwoz added the test:full Run the full test suite label Jun 12, 2026
@dwoz dwoz added this to the Potassium v3009.0 milestone Jun 12, 2026
- Rename changelog/api-mtls.added.md to changelog/69441.added.md
  to match the PR number naming convention enforced by the
  Check Changelog Entries hook.
- Strip trailing whitespace from the ssl_allowed_cn docstring in
  salt/netapi/rest_cherrypy/app.py.
- Add blank line after module docstring in
  tests/pytests/unit/netapi/cherrypy/test_ssl_cn_filter.py per
  black formatting.

Co-authored-by: Stefan Bogner <bochi@users.noreply.github.com>
Comment thread salt/netapi/rest_cherrypy/__init__.py Outdated
"ssl_allowed_cn: rejecting client with CN=%r (not in allow list)",
cn,
)
raise cherrypy.HTTPError(403, "Client certificate CN not allowed")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Issue: Relying entirely on cherrypy.request.wsgi_environ.get("SSL_CLIENT_S_DN_CN") works perfectly when CherryPy is running behind an upstream proxy (like Nginx or Apache) terminating the TLS connection. However, if a user runs salt-api standalone using the builtin SSL module (which this PR explicitly configures in init.py), CherryPy's internal web server does not automatically populate WSGI environment variables with client certificate fields.

Without a fallback to extract the CN from the underlying socket's peer certificate wrapper when running standalone, cn will evaluate to None and reject all valid client certificate connections with a 403 Forbidden.

Suggested Replacement:

        environ = cherrypy.request.wsgi_environ
        cn = environ.get("SSL_CLIENT_S_DN_CN")
        
        # Fallback for standalone 'builtin' SSL engine where WSGI env variables aren't populated automatically
        if not cn:
            try:
                # Extract peer certificate directly from the underlying socket context if available
                conn = cherrypy.request.wsgi_environ.get("cherrypy.connection")
                if conn and hasattr(conn, "ssl_ctx"):
                    # For python ssl contexts, we can fetch the peer cert from the active socket socket
                    peercert = conn.socket.getpeercert()
                    if peercert:
                        # Extract the Common Name component from the subject tuple structure
                        for rdn in peercert.get("subject", []):
                            for rdn_attr in rdn:
                                if rdn_attr[0] == "commonName":
                                    cn = rdn_attr[1]
                                    break
            except Exception as exc:
                logger.debug("Failed to extract client CN from socket context: %s", exc)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair, but cheroot's BuiltinSSLAdapter doesn't seem to have a cherrypy.connection ssl_ctx object so I used SSL_CLIENT_CERT which is exposed when client cert auth is active.

("lowdata_fmt", lowdata_fmt),
("hypermedia_out", hypermedia_out),
("salt_ip_verify", salt_ip_verify_tool),
("salt_ssl_cn_filter", salt_ssl_cn_filter_tool),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Issue: In CherryPy, listing a custom tool function directly inside the _tools tuple list doesn't automatically register it into the cherrypy.tools namespace hierarchy. When the configuration parser hits "tools.salt_ssl_cn_filter.on": True on line 1214, it will throw an internal configuration lookup error and crash during startup because it cannot find salt_ssl_cn_filter registered as an official tool handler.

The tool must be explicitly instantiated as a cherrypy.Tool object and assigned to the namespace before the application starts up.

Suggested Replacement:
Move the tool registration wrapper to a package-level binding block inside app.py instead of passing the raw function via the _tools metadata mapping:

# Place this registration statement outside the function blocks, 
# typically right after the tool definition block around line 813:
cherrypy.tools.salt_ssl_cn_filter = cherrypy.Tool('before_handler', salt_ssl_cn_filter_tool)

Note: If you handle registration globally using the hook above, you should remove line 1182 ("salt_ssl_cn_filter", salt_ssl_cn_filter_tool), entirely so it doesn't conflict with CherryPy's automatic lookup.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It already is registered via tools_config at the bottom of the module abd registered via

tools.salt_ssl_cn_filter.on": True

So there should not be a config lookup error on startup?

@dwoz dwoz merged commit f5f1649 into saltstack:master Jul 10, 2026
951 of 954 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants