[GitHub] [geronimo-metrics] diuis opened a new pull request #4: Accepted hosts ip range

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [geronimo-metrics] diuis opened a new pull request #4: Accepted hosts ip range

GitBox
diuis opened a new pull request #4: Accepted hosts ip range
URL: https://github.com/apache/geronimo-metrics/pull/4
 
 
   Hi @rmannibucau ,
   can you take a look at this pull request?
   I changed the validation predicate: now it's possible to set an IP address range.
   The format is [10.10.10.0..10.10.10.255].

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [geronimo-metrics] diuis commented on issue #4: Accepted hosts ip range

GitBox
diuis commented on issue #4: Accepted hosts ip range
URL: https://github.com/apache/geronimo-metrics/pull/4#issuecomment-592194101
 
 
   Hi @rmannibucau ,
   thank you for your review.
   I think that a syntax like [ip1..ip2] is easier for our use case (serve the metrics api to prometheus in a k8s cluster) then the 10.[10..13].10.[0..255] syntax: sysadmins/devs/devops that manage the k8s cluster, generally express IP addresses with the CIDR notation (where, if I remember correctly the host IPs are contiguous).
   Do you see some use case where only the second octect of the IP address is a range?
   In relation to the other question, I used the InetAddress class because is imho the easier and more readable way to convert the IP address to a number that I can use to check if the client IP is in the range; and with a little patch, in this way, we can support also IPv6 format.
   Later I will do a commit to remove the BigInteger to long conversion.
   What do you think about? If you prefer, I can also implement your 10.[10..13].10.[0..255] proposed syntax, but, really, do we need it?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [geronimo-metrics] rmannibucau commented on issue #4: Accepted hosts ip range

GitBox
In reply to this post by GitBox
rmannibucau commented on issue #4: Accepted hosts ip range
URL: https://github.com/apache/geronimo-metrics/pull/4#issuecomment-592204867
 
 
   Second range not sure but last two yes.
   Main issue is to ensure it is not just becoming a wildcard which break all security mecanism.
   Im not an expert but thought cidr was related to subnet masks so opening the door to foebidden calls (typically only prometheus should be able to call and not other services of the sqme network).
   
   Using a custom impl was really the way to enable that network security but clean security setup was to use a real authentication - even just adding tomcat basic auth using a tomcat-users.xml and configuring geronimo-metrics roles.
   
   What I mean is we shouldnt enble to relax too much the enforcement at network level. Localhost relaxing is ok cause you have access to the binaries anyway, others assume env setup.
   
   So here what I'd do:
   
   1. Document how to override the validator if not clear enough (i can take this point)
   2. Potentially add a cdi event to plug a custom decider trivially if present (would enable you to plug any impl you want) - i cna do it too if needed
   3. Maybe try role based security (works not bad with prometheus and avoids any network whitelisting)
   4. If none works, use a range support forcing explicit ip but not just submasks which are almost wildcards and often leaks foebidden hosts
   
   Hope it makes sense, wdyt?
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [geronimo-metrics] rmannibucau edited a comment on issue #4: Accepted hosts ip range

GitBox
In reply to this post by GitBox
rmannibucau edited a comment on issue #4: Accepted hosts ip range
URL: https://github.com/apache/geronimo-metrics/pull/4#issuecomment-592204867
 
 
   Second range not sure but last two yes.
   Main issue is to ensure it is not just becoming a wildcard which break all security mecanism.
   Im not an expert but thought cidr was related to subnet masks so opening the door to forbidden calls (typically only prometheus should be able to call and not other services of the sqme network).
   
   Using a custom impl was really the way to enable that network security but clean security setup was to use a real authentication - even just adding tomcat basic auth using a tomcat-users.xml and configuring geronimo-metrics roles.
   
   What I mean is we shouldnt enble to relax too much the enforcement at network level. Localhost relaxing is ok cause you have access to the binaries anyway, others assume env setup.
   
   So here what I'd do:
   
   1. Document how to override the validator if not clear enough (i can take this point)
   2. Potentially add a cdi event to plug a custom decider trivially if present (would enable you to plug any impl you want) - i cna do it too if needed
   3. Maybe try role based security (works not bad with prometheus and avoids any network whitelisting)
   4. If none works, use a range support forcing explicit ip but not just submasks which are almost wildcards and often leaks foebidden hosts
   
   Hope it makes sense, wdyt?
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [geronimo-metrics] rmannibucau commented on issue #4: Accepted hosts ip range

GitBox
In reply to this post by GitBox
rmannibucau commented on issue #4: Accepted hosts ip range
URL: https://github.com/apache/geronimo-metrics/pull/4#issuecomment-612372065
 
 
   Hello @diuis , did you manage to move forward on that issue? We are working forward the microprofile metrics 3 spec support so a release can come in the following weeks. I'd like to ensure this issue is covered as well if needed.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [geronimo-metrics] diuis commented on issue #4: Accepted hosts ip range

GitBox
In reply to this post by GitBox
diuis commented on issue #4: Accepted hosts ip range
URL: https://github.com/apache/geronimo-metrics/pull/4#issuecomment-612444392
 
 
   Hi @rmannibucau .
   If you like I can implement a different approach to solve the issue: there is something that I can do to help?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [geronimo-metrics] rmannibucau commented on issue #4: Accepted hosts ip range

GitBox
In reply to this post by GitBox
rmannibucau commented on issue #4: Accepted hosts ip range
URL: https://github.com/apache/geronimo-metrics/pull/4#issuecomment-612447405
 
 
   Hi @diuis , the issue - if I remember ;) - was that the range can become a kind of wildcard and break the whole goal of the validator. Therefore it seems I was more about making it easy to customize the validator (replace its impl by another one). For CDI app an easy solution can be a CDI event fired before the default validation with a Boolean and if an observer set it to true|false then we use that result as validation result otherwise we use the default impl. It would enable you to plug whatever impl fits your need (or we can have it as an extension/other module).
   
   wdyt?
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services