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

Allow to use client authentication with certificate in freshclam #955

Conversation

jedrzejj
Copy link
Contributor

This PR adds the possibility to define certificate and private key for client authentication in freshclam when using private repo. Certificate and key paths are read from environment variables and passed on to libcurl, similarly to the CA bundle.

@jedrzejj jedrzejj force-pushed the feature/freshclam-client-cert-authentication branch from 08ae1bc to 13e3b23 Compare June 15, 2023 22:28
@micahsnyder micahsnyder force-pushed the feature/freshclam-client-cert-authentication branch from 13e3b23 to ca335f9 Compare August 2, 2023 21:12
@micahsnyder
Copy link
Contributor

Apologies for the delay in reviewing this. This change look very straightforward.

Some issues that I've taken on with the commit I just pushed:

  • I found some missing documentation.
  • I found no reason why this feature couldn't work on mac and windows.
  • I renamed the variables to prefix with FRESHCLAM_ instead of CURL_ because they do not piggyback on existing curl environment variables and are unique to clamav. I specifically I chose FRESHCLAM_ over CLAM_ or CLAMAV_ because it may make sense to someday support certificate-based authentication between clamd and other scan clients -- and I figure I don't want confusion with the names in that hypothetical future.
  • I was looking at the libcurl documentation and noticed that it supports password protected key files, so I added one more variable to support that.

I believe this works, and could be tested like so, if your freshclam.conf file is set up to talk to a private mirror that enforces client certificate authentication...
Linux/Unix:

FRESHCLAM_CLIENT_CERT="/home/micah/client_certificate.pem" FRESHCLAM_CLIENT_KEY="/home/micah/client_private_key.pem"  FRESHCLAM_CLIENT_KEY_PASSWD="testtest" ./install/bin/freshclam

Windows powershell:

$env:FRESHCLAM_CLIENT_CERT="C:\Users\micah\.ssh\client_certificate.pem" & $env:FRESHCLAM_CLIENT_KEY="C:\Users\micah\.ssh\client_private_key.pem" & .\install\freshclam.exe

I don't have such a server and am uncertain how to quickly set one up.

Also:
- Rename to use FRESHCLAM_CLIENT_CERT, FRESHCLAM_CLIENT_KEY instead
  prefixing with "CURL_". Unlike CURL_CA_BUNDLE, these variable names
  are not used by the `curl` program and so do not piggyback on that
  existing functionality.

- Add FRESHCLAM_CLIENT_KEY_PASSWD environment variable to support
  password protected private key PEM files, as described in:
  https://curl.se/libcurl/c/CURLOPT_SSLCERT.html

- Document the new environment variable options in the manpage and in
  the `freshclam --help` message. Also add missing documentation in the
  freshclam and clamsubmit help-messages for CURL_CA_BUNDLE.

- Update the NEWS.md file to credit jedrzej for the new feature.
@micahsnyder micahsnyder force-pushed the feature/freshclam-client-cert-authentication branch from 5413c54 to 3f3382b Compare August 3, 2023 07:30
Copy link
Contributor

@micahsnyder micahsnyder left a comment

Choose a reason for hiding this comment

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

Had a bit of a hiccup this morning with our Jenkins. It's fixed now and this ran through okay, not that I expected any differently. Perhaps we could set up a test some time that makes use of this feature, but in the meantime I'm going to trust that it works okay base don the limited testing I did, and happy that it isn't causing any problems.

@micahsnyder micahsnyder merged commit eb139c6 into Cisco-Talos:main Aug 4, 2023
22 of 23 checks 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.

2 participants