Skip to content

Conversation

@netgab
Copy link
Contributor

@netgab netgab commented Feb 5, 2025

Added the option to control certificate validation, to the device connection as:

devices:
  eWLC:
        os: iosxe
        type: eWLC
        custom:
          abstraction:
            order: [os]
        connections:
          rest:
            class: rest.connector.Rest
            ip: 198.51.100.3
            #verify: True                  # <-- Enable certificate validation (default)
            verify: False                  # <-- Disable certificate validation

This setting is applied to the requests.session object once, which is applied to all following operations like GET, POST etc.

@netgab netgab requested a review from a team as a code owner February 5, 2025 14:26
@netgab netgab requested review from Taarini and omehrabi February 5, 2025 14:26
@netgab
Copy link
Contributor Author

netgab commented Feb 5, 2025

I guess the pipeline fails, because Python 3.8 is EoL
https://devguide.python.org/versions/

The runners don't support Python 3.8

Error: Version 3.8 with arch x64 not found
Available versions:

3.10.16 (x64)
3.11.11 (x64)
3.12.8 (x64)
3.13.1 (x64)
3.9.21 (x64)

Should the file CiscoTestAutomation-rest/.github/workflows/run_tests.yml also be modified, to exclude python-version 3.8?

log.debug("Timeout: %s" % timeout)
self.content_type = default_content_type

self.verify = self.connection_info.get('verify', True)
Copy link

Choose a reason for hiding this comment

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

shouldn't this default to False?

Suggested change
self.verify = self.connection_info.get('verify', True)
self.verify = self.connection_info.get('verify', False)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As of today, it defaults to True for the get requests (as no verify parameter is passed there).
Furthermore, I guess a good security practice is, to enable SSL/TLS certificate validation by default and only disable it, if there are specific reasons.

Copy link

Choose a reason for hiding this comment

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

I don't argue, even if test tools often interact with lab devices where certificate discipline is often, err, sloppy..
but in the PR description you mentioned

            verify: False                  # <-- Disable certificate validation (default)            

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry @oboehmer .. my bad. I updated the PR description above

Copy link
Contributor

@omehrabi omehrabi left a comment

Choose a reason for hiding this comment

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

Please add a changelog and also merged latest code into your branch

@netgab
Copy link
Contributor Author

netgab commented Sep 23, 2025

Hi @omehrabi ,
did that - hope it's ok

@omehrabi
Copy link
Contributor

@netgab
Copy link
Contributor Author

netgab commented Sep 25, 2025

Sorry @omehrabi, that I forgot the changelog.
What is the correct and desired process for this?

Just create a new file (e.g. docs/changelog/2025/sept.rst) and add my content there?
Or just edit the file docs/changelog/undistributed/template.rst ?

I guess my next issue and PR will be a CONTRIBUTION.MD file :)

@netgab
Copy link
Contributor Author

netgab commented Sep 25, 2025

Hey @omehrabi ,
thanks for helping me with the changelog
Added a new file docs/changelog/undistributed/issue138_iosxe_cert_validation.rst with my change. Hope it's ok.
I'm off to vacation now ;)

Copy link
Contributor

@lsheikal lsheikal left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. code looks good.

@Taarini Taarini merged commit 49dc9d2 into CiscoTestAutomation:main Sep 25, 2025
5 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.

5 participants