-
Notifications
You must be signed in to change notification settings - Fork 1k
Add Ivanti Secure Connect model #3677
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
base: master
Are you sure you want to change the base?
Add Ivanti Secure Connect model #3677
Conversation
robertcheramy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR. I'm not too familiar with the http input, but your PR looks good to me.
Could you please take a look at my comments and address them where applicable?
lib/oxidized/input/http.rb
Outdated
| req_class = case method.to_s.downcase.to_sym | ||
| when :post then Net::HTTP::Post | ||
| else Net::HTTP::Get | ||
| end | ||
| req = req_class.new(uri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Maybe you could document allowed methods in docs/Inputs.md (
:getand:post), so there is no need to to_s.downcase.to_sym? - Using if method == :post / else seems simplier to me than case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, just added if-elsif-else with raise for greater clarity and to avoid potential difficulties during debugging since only two methods are currently supported
lib/oxidized/model/ivanti.rb
Outdated
| login_user = vars(:username) || @node.auth[:username] | ||
| login_pass = vars(:password) || @node.auth[:password] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this redundant to just using @node.auth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I also simplified this block
|
Hello @robertcheramy, |
|
Hello @robertcheramy, |
|
I will review this PR before the Release of 0.36. I'm currently spending my available ressources on closing the issues on the roadmap. |
Hi @ytti,
As a follow-up to the issue #3672 I opened earlier, here’s a PR that adds support for Ivanti Secure Connect (ICS) devices and extends the HTTP input with POST support.
Pre-Request Checklist
rubocop --auto-correct)rake test)Description
1. Added new Ivanti Secure Connect model
The model is as thin as possible and delegates all HTTP mechanics to the transport layer as mentioned in related issue. The model implements the standard ICS API workflow:
/api/v1/realm_authto obtain a temporaryapi_key/api/v1/system/binary-configuration2. Added POST support in
input/http.rbICS requires a POST before any config can be fetched, so adding
post_httpmethod felt like the right long-term solution. So the transport now:3. Tests
Added tests for:
post_http(success, custom headers, nil body)What it was tested on
Hardware: ISA6000-V
ICS version: Ivanti Secure Connect 22.x
(should also work across other ICS hardware/virtual platforms because API is consistent, but I haven't tested that cases)
Logs while testing on real device
Here are logs with enabled
debugsetting in config and vulnerable data replaced with<its-description>:Closes issue #3672