Skip to content

feat: add mTLS support for secure client-server communication (#3172) - #4571

Open
zs-gabor wants to merge 2 commits into
crowdsecurity:masterfrom
zs-gabor:appsec-allow-mtls-auth-3172
Open

feat: add mTLS support for secure client-server communication (#3172)#4571
zs-gabor wants to merge 2 commits into
crowdsecurity:masterfrom
zs-gabor:appsec-allow-mtls-auth-3172

Conversation

@zs-gabor

Copy link
Copy Markdown

Kérlek ellenőrizd és ha kell javíts a szövegen, amit a Description mezőbe írtam:

📝 Description

This PR implements mutual TLS (mTLS) support to enhance security for communication channels within CrowdSec, addressing the feature request in #3172

With these changes, clients and servers can now authenticate each other using X.509 certificates, ensuring both encryption and mutual identity verification.

✨ Changes Introduced

  • Configuration Updates:
      * Added configuration options for CA certificate (ca_cert_file), enable TLS authentication (tls_auth) .

  • Core Functionality:
      * In case of TLS auth, checking the existence of the necessary certificates in the "UnmarshalConfig" function.
      * I have highlighted the HTTP server settings in a new function "configureHTTPServer" so that the unit test is easier to check for mTLS.
      * In the "appsecHandler" in case of TLS authentication, the apiKey check has been excluded.

  • Testing:
      * Added unit/integration tests for mTLS handshake success and failure scenarios (e.g., trusted and untrusted client cert).

🔗 Related Issue

Fixes #3172

🧪 How to Test

  1. Generate Certificates:
       Generate a test CA, server certificate, and client certificates using renew-test-certs.sh (I took the script from the http module and expanded it).

  2. Update Configuration:
       Enable mTLS in your configuration file (acquis.d/appsec.yaml):

tls_auth: true
ca_cert_file: /etc/crowdsec/ssl/ca.crt
cert_file: /etc/crowdsec/ssl/server.crt
key_file: /etc/crowdsec/ssl/server.key
  1. Verify Connection:
  • Valid Client Cert: Connection should succeed.
  • Invalid / Missing Client Cert: Connection should be rejected with a TLS handshake error.

✅ Checklist

  • Code follows project style guidelines
  • Tests added/updated and passing
  • Verified backwards compatibility when mTLS is disabled

@github-actions

Copy link
Copy Markdown

@zs-gabor: There are no 'kind' label on this PR. You need a 'kind' label to generate the release automatically.

  • /kind feature
  • /kind enhancement
  • /kind refactoring
  • /kind fix
  • /kind chore
  • /kind dependencies
Details

I am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository.

@github-actions

Copy link
Copy Markdown

@zs-gabor: There are no area labels on this PR. You can add as many areas as you see fit.

  • /area agent
  • /area local-api
  • /area cscli
  • /area appsec
  • /area security
  • /area configuration
Details

I am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository.

@zs-gabor

Copy link
Copy Markdown
Author

/kind feature

/area appsec
/area configuration

@xhon-pelushi xhon-pelushi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Read this against master. The TLS plumbing itself is set up correctly — tls.RequireAndVerifyClientCert with a ClientCAs pool built from ca_cert_file is the right starting point, and pulling the server construction out into configureHTTPServer does make it testable. Two things below though, the first of which I think is a security regression rather than a nit.

1. tls_auth disables authentication instead of replacing it.

if !w.config.TLSAuth {
    apiKey := r.Header.Get(appsec.APIKeyHeaderName)
    ...
    if err := w.checkAuth(ctx, apiKey); err != nil { ... 401 ... }
}

RequireAndVerifyClientCert proves only that the peer holds a certificate chaining to the configured CA. It does not say which client this is, and it is not a substitute for checkAuth, which calls validateKey against LAPI — that is where a bouncer being unregistered or revoked actually gets caught. As written, once tls_auth: true:

  • any certificate issued by that CA is accepted as a fully authorized appsec client. If ca_cert_file is an existing internal/corporate CA — which is the common case for anyone who already has a PKI and is why they want mTLS — every certificate that CA has ever signed, for any purpose, is now an appsec client.
  • a revoked bouncer certificate keeps working indefinitely. There is no CRL and no OCSP here, and the API-key path that used to provide revocation is switched off.
  • the request is no longer attributable to a bouncer at all, so AuthCache and the LAPI validation side of checkAuth are simply gone.

CrowdSec already solves this for LAPI in pkg/apiserver/middlewares/v1/tls_auth.go, and that implementation does considerably more than this one: checkAllowedOU() against AllowedAgentsOU/AllowedBouncersOU, isExpired(), and checkRevocationPath() covering both OCSP (ocsp.go) and CRL (crl.go), wired up in pkg/apiserver/apiserver.go:562-572. It would be worth either reusing v1.NewTLSAuth here or mirroring the same three checks, and treating the client certificate as identifying a bouncer that still has to be known to LAPI, rather than as a blanket bypass of the auth path. An allowed_ou setting in particular seems like table stakes given the rest of the codebase has one.

If keeping the two mechanisms independent is deliberate, it would be good to say so in the config documentation, spelling out that tls_auth: true means "anyone with a cert from this CA", so operators know to use a dedicated CA rather than their org-wide one.

2. The Unix socket listener silently gains TLS.

startServer loses its canTLS parameter, and the two call sites change from startServer(listener, false) / startServer(listener, true) to plain startServer(listener). On master the socket path passes false explicitly, so listen_socket is always plaintext regardless of whether cert_file/key_file are set. After this change, the condition is:

if w.config.TLSAuth || w.config.CertFilePath != "" || w.config.KeyFilePath != "" {

which is shared by both listeners, so any existing deployment that configures listen_socket and a TLS cert for the TCP listener will find its Unix socket has become TLS — and with tls_auth on, demanding client certificates from local clients too. That looks unintended, and it is the kind of change that surfaces as "my local appsec socket stopped working" after an upgrade. If the intent was only "tls_auth implies TLS on the TCP listener", keeping the parameter and passing false for the socket preserves that.

Smaller notes:

  • The tls_auth prerequisites are validated twice, in UnmarshalConfig and again at the top of configureHTTPServer, with different error strings. The second set is unreachable given the first, so it may as well go.
  • run_test.go covers trusted vs. untrusted client certs, which is the right shape — worth adding a case for a certificate that is signed by the configured CA but is not a legitimate bouncer, since that is exactly the gap in point 1 and a test would pin the intended behaviour once it is decided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[APPSEC] Allow MTLS auth

2 participants