feat(system): add session check for agent api#8258
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. DetailsInstructions 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. |
| func checkSession(c *gin.Context) bool { | ||
| _, err := global.SESSION.Get(c) | ||
| return err == nil | ||
| } |
There was a problem hiding this comment.
-
handlenotroutehas two calls toCheckSecurity, one inside another. This could lead to unnecessary overhead if multiple requests need to pass through this function without being redirected. -
Some functions like
HandleNotSecurity,IsEntrancePath, etc., perform similar checks onc.Request.URL.Path. These duplicate logic can be extracted into utility functions or reused directly instead of repeating them. -
The function
toindexhtmlwrites raw HTML which should always be sanitized before rendering to prevent XSS attacks. -
Function names are too short, making it hard to understand their responsibilities and usage within different context.
-
Comments should be clearer about what each function does and why certain conditions were chosen.
-
The error handling within
HandleNoTSecurityseems redundant since there's no way to return from that function if an error occurs while reading the file contents. -
Using constants for hardcoded values like
"err_domain"and"200_200"would make your code cleaner and easier to maintain. -
Checking IP addresses against CIDR ranges needs a dedicated helper method for better readability and clarity. Instead of using regular expressions for such comparisons.
-
In some places where checking against empty strings (
len(status.value) == 0) it might indicate that these config variables aren't properly configured at initialization stage but haven't been validated yet leading to incorrect behavior during runtime. Adding more verbose log statements can help diagnose these issues earlier.
Overall this implementation looks good overall aside mostly from the few optimizations described above.
| security.HandleNotSecurity(c, "") | ||
| }) | ||
|
|
||
| return Router |
There was a problem hiding this comment.
There are several changes made and improvements suggested in this code snippet:
-
Imports: Some imports have been added back to fix compilation errors.
-
Function Moves:
toIndexHtmlwas moved into thesecuritypackage since it handles index HTML rendering which is security-related now.handleNoRoute,checkEntrance,checkFrontendPath,checkBindDomain,checkIPLimit, andcheckSessionwere all converted tosecurity.*.
-
Logic Changes:
- The authentication logic has been consolidated under
middleware.ApiAuth()for API routes. - No-route handling functions (
handleNoRouteand related checks) have been deprecated and replaced with corresponding methods in thesecuritypackage likeHandleNotSecurityandHandleNotRoute.
- The authentication logic has been consolidated under
-
Documentation Update:
- Comments mentioning
isFrontendPath,checkFrontendPath,checkSession, etc., have been removed as these are no longer needed after consolidation.
- Comments mentioning
-
Variable Renames:
- Certain variables like
entrancewere renamed within their respective packages to avoid conflicts or misidentification.
- Certain variables like
-
Consistent Function Calls:
- Most function calls across the file now use lowercase letters unless necessary (e.g.,
CheckSecurityinstead ofcheckSecurity) to adhere to Go coding standards.
- Most function calls across the file now use lowercase letters unless necessary (e.g.,
These changes aim to improve consistency, maintainability, and clarity, especially in areas dealing with security-related paths and functionality.
| hash := md5.New() | ||
| hash.Write([]byte(param)) | ||
| return hex.EncodeToString(hash.Sum(nil)) | ||
| } |
There was a problem hiding this comment.
The provided code has several issues and areas for improvement:
Issues:
-
Magic Numbers: The use of constants like
toleranceand60across multiple places could lead to magic numbers. Consider defining these as named variables within functions where they're used. -
Error Handling in Logs: The logging calls can be optimized by using formatted logs instead of printing raw errors directly to logs.
-
Repetition in Code: Functions like
isValid1PanelTimestamp,isValid1PanelToken, andGenerateMD5have similar logic repeated in different methods. This redundancy can be streamlined. -
Security Concerns:
- The MD5 hashing function should be replaced with stronger cryptographic algorithms that resist collision attacks, such as SHA-256.
-
Documentation: Comments are present but not detailed enough to explain how each part of the code works.
-
Performance: Ensure that parsing IP addresses and validating CIDRs efficiently.
Optimization Suggestions:
- Use Named Variables for Tolerances: For example, instead of writing
int64(60)repeatedly, define them at the top level or in local contexts.
Example refactored version:
const (
defaultTolerance = 60 // seconds tolerance allowed
)
// Define other constants similarly
func ValidateAPITokenAndPermissionMiddleware() gin.HandlerFunc {
return func(c *gin.Context) {
if strings.HasPrefix(c.Request.URL.Path, "/api/v2/core/auth") {
c.Next()
return
}
panelToken := c.GetHeader("1Panel-Token")
apiKeyTimestamp := c.GetHeader("1Panel-Timestamp")
// Check API interface status and timestamps etc...
}
}
func IsValidAPIKeyTimestamp(timestampStr string, validTimeInSeconds int, currentTimeOffsetSeconds int) bool {
currentUnixTime := getCurrentUnixTime(currentTimeOffsetSeconds)
parsedTimestamp, err := strconv.ParseInt(timestampStr, 10, 64)
if err != nil {
log.Printf("Failed to parse timestamp: %v\n", err)
return false
}
thresholdTime := currentUnixTime + time.Duration(validTimeInSeconds)*time.Second
if parsedTimestamp > thresholdTime {
log.Println("Invalid API key timestamp.")
return false
}
return true
}
func GetCurrentUnixTime(offsetSeconds int) int64 {
current := time.Now().Unix()
// Apply offset if needed
current += int64(offsetSeconds)
return current
}
func GenerateSecureHash(message string) string {
hash := sha256.New()
hash.Write([]byte(message))
return hex.EncodeToString(hash.Sum(nil))
}This refactoring reduces repetition, improves logging quality by formatting messages properly, and enhances security by utilizing a stronger encryption method (sha256) and ensuring proper error handling and parameter validation.
|
|
/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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
No description provided.