go.mod (2)

Line range hint 3-3: Fix invalid Go version.

go 1.23 specified in the go.mod file is invalid as Go 1.23 hasn't been released yet. The latest stable version is Go 1.22.

-go 1.23
+go 1.22

---

Line range hint 18-18: Remove old JWT library to prevent conflicts.

Both v3 and v5 of the JWT library are present which could lead to dependency conflicts and confusion. Since you're implementing new JWT functionality, standardize on v5.

-	github.com/golang-jwt/jwt v3.2.2+incompatible

Also applies to: 25-25

pkg/lib/license/license_validator.go (5)

43-62: Simplify JWKS validation in NewLicenseValidatorFromJSON

The manual parsing and validation of the JWKS JSON may be redundant since keyfunc.NewJWKSetJSON already handles parsing and structure validation of the JWKS. Consider removing the explicit JSON unmarshalling and checks to simplify the code.

Apply this diff to streamline the function:

 func NewLicenseValidatorFromJSON(jwksJSON json.RawMessage) (*LicenseValidator, error) {
-	if len(jwksJSON) == 0 {
-		return nil, fmt.Errorf("empty JWKS JSON")
-	}
-
-	// Parse the JSON first to validate structure
-	var jwks struct {
-		Keys []interface{} `json:"keys"`
-	}
-	if err := json.Unmarshal(jwksJSON, &jwks); err != nil {
-		return nil, fmt.Errorf("invalid JWKS JSON: %w", err)
-	}
-
-	// Check if keys array exists and is not empty
-	if jwks.Keys == nil {
-		return nil, fmt.Errorf("missing 'keys' array in JWKS")
-	}
-	if len(jwks.Keys) == 0 {
-		return nil, fmt.Errorf("empty 'keys' array in JWKS")
-	}

 	// Create the JWKS key function
 	keyFunc, err := keyfunc.NewJWKSetJSON(jwksJSON)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create JWKS key function: %w", err)
 	}

 	return &LicenseValidator{
 		keyFunc: keyFunc.Keyfunc,
 	}, nil
 }

---

81-83: Enhance error handling for token parsing

Currently, when token parsing fails, the error message is wrapped with "failed to parse token" using %w. To provide clearer error messages without exposing sensitive information, consider using %v or adding more context to the error.

Apply this diff to adjust the error reporting:

 	if err != nil {
-		return nil, fmt.Errorf("failed to parse token: %w", err)
+		return nil, fmt.Errorf("failed to parse token: %v", err)
 	}

---

85-87: Remove redundant token validity check

The jwt.ParseWithClaims function already validates the token and returns an error if the token is invalid. Checking if !token.Valid immediately after parsing is redundant. Consider removing this check to simplify the code.

Apply this diff to eliminate the redundant code:

 	if err != nil {
 		return nil, fmt.Errorf("failed to parse token: %v", err)
 	}
-
-	if !token.Valid {
-		return nil, fmt.Errorf("invalid token")
-	}

---

97-113: Utilize built-in claims validation

The jwt.RegisteredClaims type provides a Valid() method that performs standard validation of registered claims like ExpiresAt and NotBefore. Replace the manual checks with this method to ensure consistent and thorough validation.

Apply this diff to use the built-in validation:

 func (v *LicenseValidator) validateAdditionalConstraints(claims *LicenseClaims) error {
-	now := time.Now()
-
-	// Check if token is expired
-	if claims.ExpiresAt != nil && claims.ExpiresAt.Before(now) {
-		return fmt.Errorf("license has expired")
-	}
-
-	// Check if token is not yet valid
-	if claims.NotBefore != nil && claims.NotBefore.After(now) {
-		return fmt.Errorf("license is not yet valid")
-	}
+	if err := claims.RegisteredClaims.Valid(); err != nil {
+		return fmt.Errorf("token validation error: %v", err)
+	}

 	// Add any other custom validation logic here
 	return nil
 }

---

9-10: Consider implementing JWKS refresh for key rotation

If the public keys in the JWKS might change over time due to key rotation, consider implementing a mechanism to refresh the JWKS periodically. The keyfunc package supports fetching JWKS from a URL with auto-refresh capabilities.

This enhancement improves security by ensuring that the validator uses the most recent set of keys.

pkg/lib/license/license_validator_test.go (1)

96-157: Expand test coverage for token validation

Currently, the TestValidateToken function does not include tests for expired or not-yet-valid tokens. Adding these test cases will ensure that the validateAdditionalConstraints method correctly handles tokens outside their valid time window.

Would you like assistance in adding these test cases?

🧰 Tools

🪛 Gitleaks (8.21.2)

130-130: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

136-136: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

cspell.yaml (1)

36-36: Re-evaluate the need to ignore the test file in spell checking

Adding pkg/lib/license/license_validator_test.go to ignorePaths excludes it from spell checking, which might allow typos to slip through. Consider removing this entry and instead add any necessary words to the custom dictionary or address specific spelling errors within the test file.

This approach maintains code quality by ensuring all files are checked for spelling errors.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

pkg/lib/license/license_validator_test.go (1)

260-264: Consider using a cross-platform approach to test invalid file permissions

The test case uses the hardcoded file path /root/test.json, which may not behave consistently across different operating systems or environments. On non-Unix systems or when tests are run with elevated permissions, this path might exist or be accessible, causing the test to produce unreliable results.

Consider creating a temporary file and programmatically setting its permissions to an inaccessible state using os.Chmod. This approach ensures the test reliably simulates the scenario of invalid file permissions across all environments.

Example:

import (
	"io/ioutil"
	"os"
)

// In your test function
file, err := ioutil.TempFile("", "test.json")
require.NoError(t, err)
defer os.Remove(file.Name())

// Set file permissions to 000 to simulate invalid permissions
err = os.Chmod(file.Name(), 0000)
require.NoError(t, err)

validator, err := NewLicenseValidatorFromFile(file.Name())
// Continue with your assertions

pkg/lib/license/README.md (3)

1-1: Remove trailing colons from headings

The headings on lines 1 and 21 have trailing colons, which is against Markdown style guidelines. Removing the colons will improve the readability and adhere to standard Markdown practices.

Apply this diff:

-## How to verify a license:
+## How to verify a license

...

-## How to generate JWKS and JWT tokens for tests:
+## How to generate JWKS and JWT tokens for tests

Also applies to: 21-21

🧰 Tools

🪛 Markdownlint (0.37.0)

1-1: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)

---

1-1: null
First line in a file should be a top-level heading

(MD041, first-line-heading, first-line-h1)

---

1-1: Add a top-level heading at the beginning of the file

Markdown linting suggests that the first line should be a top-level heading (#). Adding a main heading will provide a clear title for the README and improve navigation.

Example:

+# License Validation Guide
+
 ## How to verify a license

🧰 Tools

🪛 Markdownlint (0.37.0)

1-1: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)

---

1-1: null
First line in a file should be a top-level heading

(MD041, first-line-heading, first-line-h1)

---

32-36: Align the comment with the code

The comment states "Generate four RSA key pairs," but the code only generates one key pair (rsa_key_1). To avoid confusion, either update the comment to reflect the actual code or modify the code to generate four key pairs as indicated.

Option 1: Update the comment:

-    # Generate four RSA key pairs
+    # Generate an RSA key pair

Option 2: Update the code to generate four key pairs (if intended):

# Generate four RSA key pairs
rsa_keys = []
for i in range(4):
    rsa_key = jwk.JWK.generate(kty='RSA', size=2048, alg='RS256', use='sig', kid=f'{key_name}_{i+1}')
    rsa_keys.append(rsa_key)
    # Save keys and tokens as needed

pkg/lib/license/license_validator.go (1)

110-140: Define constants for expected values to enhance maintainability

Hardcoding expected values like the license version "v1" and issuer "https://expanso.io/" can introduce risks of typos and make future updates more cumbersome. Consider defining these values as constants to improve code maintainability and readability.

Example:

const (
	ExpectedLicenseVersion = "v1"
	ExpectedIssuer         = "https://expanso.io/"
)

// Then use:
if claims.LicenseVersion == ExpectedLicenseVersion {
	// ...
	if claims.Issuer != ExpectedIssuer {
		return fmt.Errorf("invalid issuer: expected '%s', got '%s'", ExpectedIssuer, claims.Issuer)
	}
} else {
	return fmt.Errorf("unsupported license version: %s", claims.LicenseVersion)
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: jamlo
PR: bacalhau-project/bacalhau#4799
File: pkg/lib/license/license_validator_test.go:34-35
Timestamp: 2025-01-07T05:49:03.470Z
Learning: In the context of testing JWT functionality, it is acceptable and preferred to use hardcoded JWT tokens with test data for better clarity and maintainability, even if static analysis tools flag them as potential security issues.

pkg/lib/license/license_validator_test.go (4)

112-127: Consider using constants for common test values.

While the JWT tokens are well organized, there are repeated values like "customer_1", "license_1", etc. Consider extracting these as constants for better maintainability.

+const (
+    testCustomerID1 = "customer_1"
+    testLicenseID1  = "license_1"
+    testIssuer      = "https://expanso.io/"
+    testProduct     = "Bacalhau"
+)

🧰 Tools

🪛 Gitleaks (8.21.2)

113-113: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

114-114: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

115-115: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

119-119: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

120-120: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

122-122: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

123-123: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

124-124: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

125-125: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

126-126: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

250-280: Add positive test case for file-based validator.

The test only covers error cases. Consider adding a positive test case with a temporary file containing valid JWKS data.

 func TestNewLicenseValidatorFromFile(t *testing.T) {
+    // Create a temporary file with valid JWKS
+    tmpFile, err := os.CreateTemp("", "jwks_*.json")
+    require.NoError(t, err)
+    defer os.Remove(tmpFile.Name())
+    
+    _, err = tmpFile.Write(testJWKS)
+    require.NoError(t, err)
+    require.NoError(t, tmpFile.Close())
+
     tests := []struct {
         name     string
         filename string
         wantErr  bool
     }{
+        {
+            name:     "Valid JWKS file",
+            filename: tmpFile.Name(),
+            wantErr:  false,
+        },
         {

---

339-347: Consider adding more time-based test cases.

While the test verifies expired tokens, consider adding cases for:

• Tokens that are not yet valid (before NBF)
• Tokens that are about to expire
• Tokens with invalid time formats

---

1-424: Consider adding test coverage for edge cases.

While the test coverage is comprehensive, consider adding tests for:

1. Very large tokens that might exceed size limits
2. Tokens with unsupported algorithms
3. Tokens with invalid claim types (e.g., numbers instead of strings)
4. Concurrent validation of multiple tokens

🧰 Tools

🪛 Gitleaks (8.21.2)

113-113: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

114-114: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

115-115: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

119-119: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

120-120: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

122-122: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

123-123: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

124-124: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

125-125: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

126-126: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

221-221: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

---

227-227: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: jamlo
PR: bacalhau-project/bacalhau#4799
File: pkg/lib/license/license_validator_test.go:34-35
Timestamp: 2025-01-07T05:49:03.470Z
Learning: In the context of testing JWT functionality, it is acceptable and preferred to use hardcoded JWT tokens with test data for better clarity and maintainability, even if static analysis tools flag them as potential security issues.