Make sure certificate is verified when required by RestApiClientAsync

[mdazam1942]

No description provided.

[codecov]

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (c455ab2) 86.04% compared to head (aadcc83) 86.00%.

Files	Patch %	Lines
...tils/stix_transmission/utils/RestApiClientAsync.py	80.48%	8 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1620      +/-   ##
===========================================
- Coverage    86.04%   86.00%   -0.05%     
===========================================
  Files          572      572              
  Lines        48653    48676      +23     
===========================================
- Hits         41865    41863       -2     
- Misses        6788     6813      +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Rory-Bray]

Where are the certification verification tests/testcases?

[Rory-Bray]

Please provide a valid reason why this is False. Certificate verification must never be disabled by default.

[benjamin-craig]

@mdazam1942 Can you send me the error message that you get when you try to talk to stix bundles using https

[delliott90]

STIX bundles are public URLs, the connector doesn't allow you to even pass a certificate.

[mdazam1942]

this is a special case. this is basically a demo connector. if anyone wants to use a url that is behind some kind of server that requires ssl cert validation then they can implement in their own. we have test cases using public url that doesn't require any ssl cert validation and in fact it rejects the call if the connector enforces it. here's error:

stix_shifter_utils.utils.error_response:error_response.py:91 unsupplied connector name connector error occurred: client_connector_error: (Cannot connect to host raw.githubusercontent.com:443 ssl:<ssl.SSLContext object at 0x7fd323d38540> [Network is unreachable])

[mdazam1942]

ok so we dont set the cert verify the false anymore. we made sure default ssl check is performed is certificate is not presented.

[mdazam1942]

Where are the certification verification tests/testcases?

Every connector has functional unittest that run through RestApiClientAsync class. The requests/responses are basically mocked in the unittests. For example:

stix-shifter/stix_shifter_modules/qradar/tests/stix_transmission/test_qradar.py

Line 21 in c455ab2

def test_ping_endpoint(self, mock_ping_response, mock_api_client):

Can you suggests what kind of test you are looking for? we can add functional unittests just for this class which will basically mock the ssl verification.

[mdazam1942]

Tested this changes on some connector that uses saas datasources api. They are returning SSLCertVerificationError. The reason is, we are enforcing custom SSL certificate validation for all cases even though some datasource doesn't require that. For those api, we need to do default SSL check because the connector doesn't supply certificate.

We need to revert back few of those changes that require to have certificate

if self.ssl_context:
            self.ssl_context = ssl.SSLContext(ssl.PROTOCOL_TLS)
            self.ssl_context.verify_mode = ssl.CERT_REQUIRED
            self.ssl_context.check_hostname = True

[Rory-Bray]

verify_mode and check_hostname cannot be conditional they must be applied even in the case of using a default ssl context but I don't see a call to ssl.create_default_context() any where.

[mdazam1942]

updated the code to remove ambiguity around certificate validation. now certificate is verified in both custom or default cases. also remove the option to force validation to false.

[Rory-Bray]

Where are the certification verification tests/testcases?

Can you suggests what kind of test you are looking for? we can add functional unittests just for this class which will basically mock the ssl verification.

I want to see both positive and negative tests for: trusted and untrusted certificate chains, valid and invalid hostnames, current and expired certs as minimums. Any time you override the default validation you have to explicitly make sure it's actually correctly validating certificates.

[benjamin-craig]

Where are the certification verification tests/testcases?

Can you suggests what kind of test you are looking for? we can add functional unittests just for this class which will basically mock the ssl verification.

I want to see both positive and negative tests for: trusted and untrusted certificate chains, valid and invalid hostnames, current and expired certs as minimums. Any time you override the default validation you have to explicitly make sure it's actually correctly validating certificates.

@Rory-Bray how do we handle mocking the server side functionality to do these tests?