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.

Sign up for GitHub

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

Fix assertion for test_katello_certs_check_output_invalid_input #17002

Merged
merged 1 commit into from
Dec 3, 2024
Merged

Fix assertion for test_katello_certs_check_output_invalid_input #17002

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion tests/foreman/sys/test_katello_certs_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,12 @@ class TestKatelloCertsCheck:
CA cert (a.k.a cacert.crt or rootCA.pem) can be used as bundle file.
"""

error_message = f"error 26 at 0 depth lookup: {'unsupported' if settings.repos.rhel_major_version == '8' else 'unsuitable'} certificate purpose"
Copy link
Contributor

Choose a reason for hiding this comment

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

This error surprises me a bit. If we don't assert the explicit message we emit ourselves:
https://github.com/theforeman/foreman-installer/blob/7c2b54267a933bcf0261396c0950d51c8d5bec5c/bin/katello-certs-check#L157-L169

Perhaps that's not needed and we can simply assert the right code path is used by checking for exit code 4?

Copy link
Contributor Author

@jameerpathan111 jameerpathan111 Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this is what I see in logs, no exit code 4

    
    Checking CA bundle against the certificate file: 
    [FAIL]
    
    The /root/cacert.crt does not verify the /root/certs/invalid.crt
    CN=foreman.example.com
    error 20 at 0 depth lookup: unable to get local issuer certificate
    error /root/certs/invalid.crt: verification failed
    
    Checking CA bundle size: 1
    [OK]
    
    Checking if CA bundle has trust rules: 0
    [OK]
    
    Checking Subject Alt Name on certificate 
    [FAIL]
    
    The /root/certs/invalid.crt does not contain a Subject Alt Name. Common Name is deprecated, use Subject Alt Name instead. See: https://tools.ietf.org/html/rfc2818#section-3.1
    Checking Key Usage extension on certificate for Key Encipherment 
    [OK]
    
    Checking for use of shortname as CN
    
    stderr:
    
    status: 1

Copy link
Contributor

Choose a reason for hiding this comment

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

That signals something is broken. The error function sets the exit code:
https://github.com/theforeman/foreman-installer/blob/7c2b54267a933bcf0261396c0950d51c8d5bec5c/bin/katello-certs-check#L65-L70

Then It's also surprising the command exits unexpectedly, without showing [OK] or [FAIL]. That looks like check-shortname isn't following an expected code path:
https://github.com/theforeman/foreman-installer/blob/7c2b54267a933bcf0261396c0950d51c8d5bec5c/bin/katello-certs-check#L229-L250

Given there was already a warning that it doesn't contain a SAN I think we can conclude it's missing an else statement with success.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it's a bug😅. DO you want me to file an issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to have it covered in our tests: https://github.com/theforeman/foreman-installer/blob/develop/spec/katello_certs_check_spec.rb is what we use in CI.

Do I read it well that these certs are generated using tests/foreman/data/certs.sh? In particular:

robottelo/tests/foreman/data/certs.sh

Lines 48 to 49 in fdad16a

# generate invalid certs with hostname as 'foreman.example.com'
generate_certs "invalid" "/CN=foreman.example.com" "client_extensions"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

invalid_inputs = [
(
{
'check': 'Checking CA bundle against the certificate file',
'message': 'error 26 at 0 depth lookup: unsupported certificate purpose',
'message': error_message,
},
'certs/invalid.crt',
'certs/invalid.key',
Expand Down
Loading