feat: API interface IP whitelist supports IPv6 #7981
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| hash := md5.New() | ||
| hash.Write([]byte(input)) | ||
| hash.Write([]byte(param)) | ||
| return hex.EncodeToString(hash.Sum(nil)) | ||
| } |
There was a problem hiding this comment.
The code looks generally clean and well-structured. However, there are a few areas that could be improved:
-
Variable Naming:
- Use descriptive variable names instead of single-letter variables like
ipWhiteList,clientIP, etc.
- Use descriptive variable names instead of single-letter variables like
-
Function Comments:
- Add comments to functions to explain what they do. E.g., "Check if the provided IP address is in the white list."
-
Error Handling for Parsing CDRs:
- Check if
ParseCIDRreturns an error before using it. This prevents potential panics if a CIDR entry is invalid.
- Check if
-
Handling IPv6 Properly:
- The current implementation only checks
To4()which assumes the input is always IPv4. Consider handling both IPv4 and IPv6 properly.
- The current implementation only checks
Here's an updated version with some improvements:
package main
import (
"crypto/md5"
"encoding/hex"
"fmt"
"github.com/1Panel-dev/1Panel/backend/app/repo"
"github.com/1Panel-dev/1Panel/backend/constant"
"github.com/1Panel-dev/1Panel/backend/global"
"github.com/1Panel-dev/1Panel/backend/utils/common"
"github.com/gin-gonic/gin"
"net"
"strconv"
"time"
)
func IsValid1PanelToken(panelToken string, panelTimestamp string) bool {
// Logic to validate token goes here
return true // Placeholder
}
func IsIPInWhiteList(clientIP string) bool {
ipWhiteString := global.CONF.System.IpWhiteList
var ipWhiteList []net.IPNet
var ipErr error
if len(ipWhiteString) > 0 {
ipWhiteList, ipErr = common.HandleIPList(ipWhiteString)
}
if(ipErr != nil) {
fmt.Printf("Failed to handle IP list: %v", ipErr)
return false
}
clientParsedIP := net.ParseIP(clientIP)
if clientParsedIP == nil {
return false
}
iPv4 := clientParsedIP.To4()
iPv6 := clientParsedIP.To16()
for _, cidrBlock := range ipWhiteList {
if (iPv4 != nil && (cidrBlock.String() == "0.0.0.0" || cidrBlock.Contains(iPv4))) ||
(iPv6 != nil && (cidrBlock.String() == "::/0" || cidrBlock.Contains(iPv6))) {
return true
}
}
return false
}
func GenerateMD5(input string) string {
hasher := md5.New()
hasher.Write([]byte(input))
return hex.EncodeToString(hasher.Sum(nil))
}
func main() {
// Main function logic goes here
fmt.Println("Main function executed")
}Key Changes:
- Used structured naming for variables (
ipWhiteList,clientIP) with meaningful descriptions. - Added comments to explain each major block of code.
- Checked if
HandleIPListreturned an error after parsing the IP list. - Improved the comment about handling
parseCIDR.
These changes will make the code more readable and maintainable.
| } | ||
| } else if (checkIp(item)) { | ||
| } else if (checkIpV4V6(item)) { | ||
| return callback(new Error(i18n.global.t('firewall.addressFormatError'))); | ||
| } | ||
| } |
There was a problem hiding this comment.
The code changes involve updating the imports and checking functions related to IP addresses and CIDR formats. The key points:
-
Import Update: The file
src/utils/util.jsnow imports bothcheckCidrandcheckCidrV6, along with a new functioncheckIpV4V6. -
Function Changes: In the
checkIPsfunction, there is an updated logic that checks for IPv6 prefixes (:). If found, it usescheckCidrV6; otherwise, it defaults to usingcheckCidr. Additionally,checkIpis replaced withcheckIpV4V6.
These changes appear necessary because different types of network configurations require separate validation methods, especially when dealing with IPv6 address patterns.
| @@ -358,7 +358,7 @@ export function checkCidrV6(value: string): boolean { | |||
| if (checkIpV6(value.split('/')[0])) { | |||
| return true; | |||
| } | |||
| const reg = /^(?:[1-9]|[1-9][0-9]|1[0-1][0-9]|12[0-8])$/; | |||
| const reg = /^(?:[0-9]|[1-9][0-9]|1[0-1][0-9]|12[0-8])$/; | |||
| if (!reg.test(value.split('/')[1])) { | |||
| return true; | |||
| } | |||
There was a problem hiding this comment.
The provided JavaScript code is checking if a CIDRv6 address meets certain validation criteria. Specifically:
export function checkCidrV6(value: string): boolean {
if (checkIpv6(value.split('/')[0])) {
return true;
}
const reg = /^(?:[1-9]|[1-9][0-9]|1[0-1][0-9]|12[0-8])$/;
// This line has been changed from '!' to '+'
if (+reg.test(value.split('/')[1])) {
return true;
}
return false;
}Changes Made:
- Logical Inversion: The
!operator at the beginning of the conditional statement has been replaced with+, which does nothing since it's a number in this context. However, it appears you intended to use this logical inversion (if !someCondition) to invert the meaning of the condition. If that was the intention, please clarify to ensure the correct logic.
Potential Issues and Optimizations:
-
Regex Complexity: The regular expression
/^(?:[1-9]|[1-9][0-9]|1[0-1][0-9]|12[0-8])$only checks numbers between 0 to 128 but not 129 to 255. For strictly valid IPv6 CIDRs, ranges typically start from0. Ensure this matches your requirement. -
IP Version Checking: Both
checkIpV6andvalue.split('/')[0]assumevalueis an IP version 6 representation without the trailing slash. ModifycheckIpv6to accept the whole input correctly or adjust how you handle splitting before calling it. -
Error Handling: Consider adding error handling for invalid inputs, such as non-string values or empty strings, to prevent unexpected behavior.
-
Code Readability: The name
checkCidrV6suggests it might validate multiple aspects of IPv6 addresses beyond just the subnet mask segment. It would be helpful to document each step of the validation process more clearly.
Overall, while minor adjustments can streamline the code slightly, ensuring correctness and robustness remains crucial for reliability.
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wanghe-fit2cloud The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Refs #7836