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

[feature] Adds support for wildcard and regex match ip addresses for API keys. … #3932

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

[feature] Adds support for wildcard and regex match ip addresses for API keys. … #3932

wants to merge 6 commits into from

Conversation

rmawatson
Copy link

@rmawatson rmawatson commented Aug 24, 2017

…Also adds hostname options. Introduces "hostname:" and "regex:" syntax to the ipaddr field.

IP ( 1.2.3.4 )
compares with source ip

IP Wildcard ( 1.2.*.* )
wildcard matched against source ip

IP Regex ( regex:/^123.\d{1,3}.789.0$/ )
regex matched against source ip

Hostname ( hostname:some.example.domain.com )
dns look up domain, comapres with source ip ( this option works for dynamic dns setups )

Hostname Wildcard ( hostname:*.*.example.domain.com )
reverse dns lookup on source ip, wilcard matched against looked up domain

Hostname Regex ( hostname:regex:/\w+?\.example(\.domain)?\.com/i )
reverse dns lookup on source ip, then regex matched against looked up domain

example

…Also adds hostname options. Introduces "hostname:" and "regex:" syntax to the ipaddr field.

IP ( 1.2.3.4 )
compares with source ip

IP Wildcard ( 1.2.*.* )
wildcard compare with source ip

IP Regex ( regex:/^123.\d{1,3}.789.0$/
applies regex directly to source ip

Hostname ( hostname:some.example.domain.com )
dns look up domain, comapres with source ip

Hostname Wildcard ( hostname:*.*.example.domain.com )
reverse dns lookup on source ip wilcard compared with domain

Hostname Regex ( hostname:regex:/\w+?\.example(\.domain)?\.com/i )
reverse dns lookup on source ip, then regex applied to domain
@rmawatson rmawatson changed the title Adds support for wildcard and regex match ip addresses for API keys. … [feature] Adds support for wildcard and regex match ip addresses for API keys. … Aug 24, 2017
Copy link

@jasonhoward7 jasonhoward7 left a comment

Choose a reason for hiding this comment

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

Line 133 of class.api.php has an extra close parenthesis at the end.
Aside from that, this works great! thanks for this.

removed extra parenthesis as pointed out by jasonhoward7
@lubo
Copy link

lubo commented Feb 19, 2019

Hi @rmawatson. From what I can tell, there're at least two problems with your implementation:

  1. Invalid IPv4 addresses can be entered (e.g. 678.678.678.678). Using regex to validate IP addresses is not the way to go.
  2. There's no support for IPv6.

Personally, I'd rather see this feature removed altogether than extending it. There's plenty of existing tools, with which you can achieve the same results. For example, one can use Nginx as a front-end server and do the client validation there.

@rmawatson
Copy link
Author

rmawatson commented Feb 19, 2019

  1. Disagree with this. You could similarly enter hostname that doesn't exist, and validation would fail, or even a valid ip address that is incorrect (someone elses ip), or a nonsense regex that wont match. If I remember correctly the original 'full ip' option that existed before this PR didn't validate the ip address either. For those cases where a full ip is provided you could use the validator that exists in this project.

For any partial matches the user should be responsible for entering the correct expression in the field. Validation is unnecessary. As this PR exposes the the regex expression entry directly to the user, what do you do when they are matching using regex ip, or a wildcard ip? partial validation? If this was even possible to do, it would add additional complication to something that is otherwise a simple implementation. The regex: hostname: prefixes are only meant to direct what is input to the regex, and the type of match performed, not validate the users expression.

  1. Someone could add this if they need it - we didn't.

@giacomoarru
Copy link

Is this going to be merged into mainstream? This is an important one.

@geo782
Copy link

geo782 commented Nov 19, 2021

After changing the code, im getting the error from
if (!preg_match($match_addr,$remote_ip_or_hostname))
return $this->exerr(401, __('API key not found/active or source IP/hostname not authorized'));
}

I changed the message to get the output and I at the osticket log I get
API Error (401)
API key not found/active or source IP/hostname not authorized match_addr:152.12.34.34 remote_ip_or_hostname:152.12.34.34

Date: Παρ, Νοε 19 2021 16:13 IP Adress: 152.12.34.34

The key is an old one (just an IP) and enabled. If I rollback the code, it works fine, any idea?
Osticket 1.10

EDIT
Works fine with :
if ($is_wc_match && !preg_match($match_addr,$remote_ip_or_hostname))
return $this->exerr(401, __('API key not found/active or source IP/hostname not authorized') . ' match_addr:' . $match_addr . ' remote_ip_or_hostname:' . $remote_ip_or_hostname);
else if (!$is_wc_match && strcmp($match_addr,$remote_ip_or_hostname) !== 0)
return $this->exerr(401, __('API key not found/active or source IP/hostname not authorized') . ' match_addr:' . $match_addr . ' remote_ip_or_hostname:' . $remote_ip_or_hostname);

@HairyDuck
Copy link

@geo782 Are you able to update the PR or provide the entier function?

@dmd2222
Copy link

dmd2222 commented Dec 17, 2021

Der Fi**er vom Dienst. Nice. Probs gehen raus.

@joeldeteves
Copy link

Please merge this @protich !

@giacomoarru
Copy link

Any update in this?
Disappointed that by design osticket expects the API client to have a static IP and not a DNS hostname?

@JediKev
Copy link
Contributor

JediKev commented May 21, 2023

@giacomoarru

No, there is no update otherwise you’d see it here. Yes, by design we restrict the API Keys to specific IP Addresses. We will not be changing this behavior in the current (legacy) osTicket codebase; we are focusing most of our efforts into v2.0 (full codebase rewrite). This behavior will be changing in v2.0 as we hope to add support for IP Addresses, IP Ranges, hostnames (static and wildcard), etc. so please stay tuned.

Cheers.

@joeldeteves
Copy link

@JediKev I agree with lubo this feature is useless and should be removed. It is not a function of the software to handle IP whitelisting when that is something that is already handled by NGINX / Traefik etc. it’s one less thing for the team to support and right now only causes problems for those of us running in a clustered environment.

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.

None yet

9 participants