Fix ignored GnuTLS options when SYSTEM priorities are unavailable - #1701
Shubham-Padkonde wants to merge 4 commits into
Conversation
Signed-off-by: Shubham Padkonde <shubhampadkonde12@gmail.com>
|
|
||
| #ifdef HAVE_GNUTLS_PRIORITY_SET_DIRECT | ||
| gnutls_priority_set_direct(http->tls, priority_string, NULL); | ||
| status = gnutls_priority_set_direct(http->tls, priority_string, NULL); |
There was a problem hiding this comment.
IMO it would be better to have the check for @System before we assign it into priority string, instead of handling it here, although it adds new set of HAVE_GNUTLS_PRIORITY_SET_DIRECT ifdef.
Just do not forget to initialize the string with NULL terminator in case of error, as I did :( .
There was a problem hiding this comment.
Done in db6e871: @SYSTEM is now probed before the priority string is built (with set_direct or priority_init/deinit depending on HAVE_GNUTLS_PRIORITY_SET_DIRECT), priority_string is initialized to an empty string first, and the retry after failure is removed. test/testssloptions.py passes all 8 cases with GnuTLS 3.7.3 on both the set_direct and the priority_init code paths.
Probe @System first and only prepend it when it is configured, instead of retrying without it after the combined priority string fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@Shubham-Padkonde @michaelrsweet I'm not sure if we want python script as part of our testsuite, when it is mostly written in C and Bash. IMHO if possible, it would be great if the tests were included into testsuite (now they are run only in Github CI as separate test), and ideally in languages which are used in current testsuite. Having the test in the same language is nice to have, but IMHO if we write a test, we should be able to run it from source - which means any new deps needed by the test should be checked during configure. It would mean there could be option |
|
Integrated the checks into Validation: the full build and all eight local TLS cases pass; simulated unavailable Python TLS support produces the required configure error with The test remains Python for now; this addresses dependency detection and source-suite integration. Prepared and tested with Codex assistance. |
|
The latest build-linux failure occurs before compilation: the Ubuntu Firefox package installation fails because api.snapcraft.io returns HTTP 408 while fetching the mesa-2404 assertion. This is an external package-download failure, rather than a test result for the patch. I tried rerunning failed jobs but GitHub requires repository admin rights. Could a maintainer rerun the failed job when convenient? Run: https://github.com/OpenPrinting/cups/actions/runs/35859071218 |
|
Yeah, I don't understand why it downloads firefox for default server image... restarted. |
No worries, can we look forward to merge it. |
michaelrsweet
left a comment
michaelrsweet
left a comment
There was a problem hiding this comment.
FWIW, I'm not keen on the Python additions directly to the build system since a) we don't otherwise have a Python dependency and b) the test only applies to GNU TLS builds on Linux vs. tests that are done for all platforms/libraries.
Before this all gets merged I would like to see the configure script and makefile changes removed, with the corresponding usage of the test script only in the GitHub CI build.yml file for the GNU TLS job.
|
Removed the configure and Makefile changes in ac71bd3, including the optional Python dependency detection and Validation: reconfigured with |
This branch has not been deployed
3 participants
When GnuTLS has no named
SYSTEMpriority,@SYSTEM,NORMAL:...is rejected: both names are interpreted as configured priorities, soNORMALis not a built-in fallback there. CUPS discards that error and can negotiate TLS 1.3 despiteMaxTLS1.2, or accept TLS 1.2 despiteMinTLS1.3.Probe whether the named
SYSTEMpriority is available before constructing the priority string. Include@SYSTEM,only when that probe succeeds, retaining the requested protocol and cipher options. Check the result in both the direct and legacy GnuTLS API paths and release the session/credentials if priority setup still fails. A valid named system policy andNoSystemretain their existing behavior.Add eight local TLS handshake checks and run them in the GnuTLS CI job. The tests create temporary certificates/configuration and compile a small client against the built CUPS library; they require Python 3, a C compiler, and the
opensslcommand. No external server or printer is contacted.Validation on Ubuntu 22.04, GCC 11.4 and GnuTLS 3.7.3:
Before the fix, the missing-system maximum/minimum TLS checks fail (three failures); the fixed implementation passes all eight cases.
Full build and eight checks pass with
gnutls_priority_set_directand withac_cv_func_gnutls_priority_set_direct=noto exercise the older API path.Full
make testis not green in this environment: the external Google OIDC certificate-validation check fails and the scheduler reports 35 errors instead of 33 because Avahi is not running. The unchanged-source legacy-API build reproduces the same failures. Other scheduler command, restart and job-history checks pass.Python formatting/lint and
git diff --checkpass.This addresses the missing-system-policy part of #1677. The separate observation about the default maximum re-enabling protocols restricted by a valid system policy remains outside this patch, so this PR does not close the entire issue. macOS, Windows, physical printers, and older GnuTLS releases were not tested locally; the legacy API was exercised on GnuTLS 3.7.3.
Assisted by Codex/GPT-6 for investigation, coding, and validation.
Latest review follow-up: removed the configure/Makefile test integration and optional Python dependency detection. Only the Linux GnuTLS CI job installs the test dependencies and invokes the standalone TLS script. Reconfigured and rebuilt successfully; all eight local TLS checks pass. The full scheduler/OIDC suite was not rerun for this build-system-only update.