Skip to content

Fail fast when mTLS cert / key PEMs are missing or only one is set#94

Merged
jajreidy merged 2 commits intokonflux-ci:mainfrom
jajreidy:add-logging-certs
Mar 30, 2026
Merged

Fail fast when mTLS cert / key PEMs are missing or only one is set#94
jajreidy merged 2 commits intokonflux-ci:mainfrom
jajreidy:add-logging-certs

Conversation

@jajreidy
Copy link
Copy Markdown
Contributor

What
PulpClient now validates mutual-TLS configuration before building HTTP sessions. If either cert or key is set in config, both must be set; resolved paths must exist and be regular files. Otherwise the client raises a clear ValueError (with error logging) instead of creating a TLS context without a client certificate. The same check runs for sync and async session setup. create_session_with_retry also logs an error if it receives a cert tuple but the files are missing, as a defensive layer for callers other than PulpClient.

Why
Misconfigured paths (for example host-only paths in cli.toml inside Konflux/Tekton pods, or unmounted secrets) previously led to silent omission of the client cert and confusing HTTP 403 responses. Failing at startup with an explicit message about missing PEMs and environment path expectations makes the failure obvious and actionable.

How to test

git fetch origin
make lint
make format
pre-commit run --all-files
make test
make test-diff-coverage

Manually: point cert / key at non-existent files (or set only one of them) and confirm the process exits immediately with the new validation error instead of succeeding at client construction and failing later with 403 on the wire.

Notes for reviewers
Tests use openssl in the shared mock_config fixture to generate a short-lived self-signed PEM pair so ssl.load_cert_chain succeeds under the same rules as production. CI runners need openssl on PATH (standard on ubuntu-latest).


CHANGELOG.md under [Unreleased]Fixed now has two lines: the existing PulpClient behavior plus the create_session_with_retry logging note. If you prefer the test harness called out in the changelog too, say so and we can add a single maintainer-oriented bullet.

@jajreidy jajreidy requested a review from a team as a code owner March 30, 2026 13:07
@snyk-io
Copy link
Copy Markdown

snyk-io bot commented Mar 30, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 30, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.09%. Comparing base (0e424ea) to head (04abde9).
⚠️ Report is 116 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #94      +/-   ##
==========================================
+ Coverage   95.53%   97.09%   +1.55%     
==========================================
  Files          70       74       +4     
  Lines        3946     5400    +1454     
==========================================
+ Hits         3770     5243    +1473     
+ Misses        176      157      -19     
Flag Coverage Δ
unit-tests 97.09% <100.00%> (?)
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pulp_tool/api/pulp_client.py 96.99% <100.00%> (+3.64%) ⬆️
pulp_tool/cli/upload.py 100.00% <100.00%> (ø)
pulp_tool/cli/upload_files.py 100.00% <100.00%> (ø)
pulp_tool/utils/error_handling.py 98.70% <100.00%> (+0.42%) ⬆️
pulp_tool/utils/session.py 94.28% <100.00%> (+0.95%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jajreidy jajreidy merged commit f2d0273 into konflux-ci:main Mar 30, 2026
5 checks passed
@jajreidy jajreidy deleted the add-logging-certs branch March 30, 2026 13:34
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