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.

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

New HTTPAPI plugin for FortiOS #56870

Merged
merged 10 commits into from Jun 22, 2019

Conversation

Projects
None yet
4 participants
@mamunozgonzalez
Copy link
Contributor

commented May 23, 2019

SUMMARY

Fortinet is adding Ansible support for FortiOS and FortiGate products. This PR includes a new HTTPAPI plugin for FortiOS to handle proper connectivity to FortiGate devices for the new and existing Ansible modules. The goals for this submission are:

  • Comply with Ansible guidelines
  • Use a single connection during the lifetime of the playbook execution
  • Better usage of the inventory.

This is related to other commits that include new modules for FortiOS, such as: #56447

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

Fortios (httpapi plugin)

ADDITIONAL INFORMATION

Fortinet is gradually adding Ansible support to every feature of FortiOS, thus old modules and code which are no longer maintained and were submitted by other authors will be deprecated gradually, as new modules with overlapping functionality are added.

@ansibot

This comment has been minimized.

@mamunozgonzalez

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

@trishnaguha , It seems this job failed because of some internal issue with the infrastructure. Could you please help me triggering it again?
Thanks again.

@trishnaguha trishnaguha requested a review from Qalthos May 28, 2019

mamunozgonzalez added a commit to fortinet-solutions-cse/ansible that referenced this pull request Jun 2, 2019

mamunozgonzalez added a commit to fortinet-solutions-cse/ansible that referenced this pull request Jun 3, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

The test ansible-test sanity --test import --python 2.6 [explain] failed with 4 errors:

lib/ansible/module_utils/network/fortios/fortios.py:8:0: SyntaxError: Non-ASCII character '\xc3' in file /root/ansible/lib/ansible/module_utils/network/fortios/fortios.py on line 8, but no encoding declared; see http://www.python.org/peps/pep-0263.html for details
lib/ansible/modules/network/fortios/fortios_address.py:109:0: SyntaxError: Non-ASCII character '\xc3' in file /root/ansible/test/runner/.tox/import/lib/ansible/module_utils/network/fortios/fortios.py on line 8, but no encoding declared; see http://www.python.org/peps/pep-0263.html for details (fortios.py, line 8)
lib/ansible/modules/network/fortios/fortios_config.py:76:0: SyntaxError: Non-ASCII character '\xc3' in file /root/ansible/test/runner/.tox/import/lib/ansible/module_utils/network/fortios/fortios.py on line 8, but no encoding declared; see http://www.python.org/peps/pep-0263.html for details (fortios.py, line 8)
lib/ansible/modules/network/fortios/fortios_ipv4_policy.py:199:0: SyntaxError: Non-ASCII character '\xc3' in file /root/ansible/test/runner/.tox/import/lib/ansible/module_utils/network/fortios/fortios.py on line 8, but no encoding declared; see http://www.python.org/peps/pep-0263.html for details (fortios.py, line 8)

The test ansible-test sanity --test import --python 2.7 [explain] failed with 4 errors:

lib/ansible/module_utils/network/fortios/fortios.py:8:0: SyntaxError: Non-ASCII character '\xc3' in file /root/ansible/lib/ansible/module_utils/network/fortios/fortios.py on line 8, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details
lib/ansible/modules/network/fortios/fortios_address.py:109:0: SyntaxError: Non-ASCII character '\xc3' in file /root/ansible/test/runner/.tox/import/lib/ansible/module_utils/network/fortios/fortios.py on line 8, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details (fortios.py, line 8)
lib/ansible/modules/network/fortios/fortios_config.py:76:0: SyntaxError: Non-ASCII character '\xc3' in file /root/ansible/test/runner/.tox/import/lib/ansible/module_utils/network/fortios/fortios.py on line 8, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details (fortios.py, line 8)
lib/ansible/modules/network/fortios/fortios_ipv4_policy.py:199:0: SyntaxError: Non-ASCII character '\xc3' in file /root/ansible/test/runner/.tox/import/lib/ansible/module_utils/network/fortios/fortios.py on line 8, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details (fortios.py, line 8)

click here for bot help

@ansibot ansibot added core_review and removed needs_revision labels Jun 3, 2019

@mamunozgonzalez

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Hi @Qalthos ,
Finally I got a successful execution for this. It is ready for review (maybe it is easy to understand together with: #56447).
Thanks!

Show resolved Hide resolved lib/ansible/plugins/httpapi/fortios.py Outdated
Show resolved Hide resolved lib/ansible/plugins/httpapi/fortios.py Outdated
Show resolved Hide resolved lib/ansible/plugins/httpapi/fortios.py

@mamunozgonzalez mamunozgonzalez force-pushed the fortinet-solutions-cse:new_httpapi_fortios branch from af76415 to d641bd6 Jun 11, 2019

Miguel A. Munoz
@mamunozgonzalez

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

Hi @Qalthos,
if you agree, I would leave the code as it is. Do you think more changes are needed or can we merge it?

@mamunozgonzalez

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

ready_for_review

@ansibot ansibot added core_review and removed needs_revision labels Jun 17, 2019

@thomnico

This comment has been minimized.

Copy link

commented Jun 18, 2019

shipit

method = message_kwargs.get('method', 'GET')

if self._ccsrftoken == '' and not (method == 'POST' and 'logincheck' in url):
raise Exception('Not logged in. Please login first')

This comment has been minimized.

Copy link
@Qalthos

Qalthos Jun 20, 2019

Contributor

Perhaps I'm missing something, but I just don't see how this check should ever trigger, nor why the user should get this message.

  • Your login() method gets called automatically as the first request to the device. The response from that will pass through update_auth() and set 'x-csrftoken'
  • Requests are made to the API. Because you returned the csrf token in update_auth() previously, that is automatically added to the request headers without having to add them below.
  • At some point I guess you clear the token? This is the part I don't understand. Furthermore, as you indicate that handle_httperror() is unneeded, I'm presuming that the API returns its errors in 200 responses for some reason.
  • So you can parse the access denied error from the response to send() below, and if you need to login again, you call login() again before retrying the call and failing if it doesn't work a second time. Or you could let it pass all the way back to the module_utils, reset the connection and retry, depending on where this sort of thing is likely to happen.

Do I have something wrong here? The whole point of httpapi is to abstract the authentication dance away from the user, otherwise we might as well go back to calling open_url() in module_utils. I'm just struggling to see why telling the user they have to login is ever a thing that should happen.

This comment has been minimized.

Copy link
@mamunozgonzalez

mamunozgonzalez Jun 20, 2019

Author Contributor

Hi Qalthos,
thanks for your answer. If the system guarantees that the first call is a login then I think you are right and the exception will never raise. I can remove that part, thanks for the clarification. I just added it as an extra 'safety' measure.

Regarding the csrf token lost, our device sends the csrf token and the cookie in the first login, but in the later request it may send only the cookie, or nothing. There is no rule about that. However whenever we send a PUT or POST we must add both the token and the cookie always. And here is where I found that if I don't store the csrf token it will be lost in later requests. I have run some tests, and the ansible system do not keep the csrftoken after the first request (even if I return "None" in update_auth). Therefore, I see no other way to make it work than storing the csrf token in the class. Please let me know if I am doing something wrong. This is the code I am testing according to your comments but it does not work bcs later requests do not keep the csrf token:

    def update_auth(self, response, response_text):

        headers = {}

        for attr, val in response.getheaders():
            if attr == 'Set-Cookie' and 'APSCOOKIE_' in val:
                headers['Cookie'] = val

            elif attr == 'Set-Cookie' and 'ccsrftoken=' in val:
                csrftoken_search = re.search('\"(.*)\"', val)
                if csrftoken_search:
                    headers['x-csrftoken'] = self._ccsrftoken

        if len (headers) == 2:
            return headers
        else:
            return None

    def send_request(self, **message_kwargs):

        url = message_kwargs.get('url', '/')
        data = message_kwargs.get('data', '')
        method = message_kwargs.get('method', 'GET')

        try:
            response, response_data = self.connection.send(url, data, #headers=headers,
                                                           method=method)

            return response.status, to_text(response_data.getvalue())
        except Exception as err:
            raise Exception(err)

Please let me know if I am doing something wrong or if I misunderstood something.
Thanks!

This comment has been minimized.

Copy link
@Qalthos

Qalthos Jun 20, 2019

Contributor

I have run some tests, and the ansible system do not keep the csrftoken after the first request (even if I return "None" in update_auth).

This is what I was alluding to in my previous comments- if you need to keep the token for that purpose, then I have no problem with that. Set ccsrftoken when you see it, and just always return it in the dictianry regardless. My main goal was to impress that you are doing a bunch of things you don't have to inside of send_request().

Multiple required tokens is a thing I admit I hadn't considered before. It's possible that instead of overwriting _auth in the connection, we should instead update the dictionary. I'm not immediately sure if there are any problems that would cause, but that would also fix your issue, I think.

This comment has been minimized.

Copy link
@mamunozgonzalez

mamunozgonzalez Jun 21, 2019

Author Contributor

Ok, thanks for the clarification @Qalthos.
I have added a commit with the changes, hoping to reflect your comments as much as possible. I have reduced to a minimum the things done in 'send_request' and now I return always the csrftoken as headers in update_auth. Do you think it could be ok this way?

@Qalthos Qalthos merged commit 64c3b4f into ansible:devel Jun 22, 2019

1 check passed

Shippable Run 128803 status is SUCCESS.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.