WIP: Single-init for Gw.NotificationVerifier

[titpetric]

User description

This PR fixes some issues around bundles:

• NotificationVerifier was created in two places, moved to *gateway.afterConfSetup
• Updates MockBundleGetter to FileBundleGetter, load contents from disk, support file://
• Add full regression test with e2e request for the testing bundle
• Delete bundle cache after each test run, fix some bundle tests to invoke StartTest correctly
• Fix backoff settings for test making TestBundle functions fast due to no sleep
• Rename NotificationVerifier to SignatureVerifier (or more appropriate)

---

Type

enhancement, bug_fix

---

Description

• Centralized the initialization of NotificationVerifier to the afterConfSetup method in gateway/server.go, ensuring it's only done once and improving maintainability.
• Simplified the bundle and signature verification processes by removing redundant checks and dependencies, making the code cleaner and more efficient.
• Removed the dependency on goverify from the bundle verification logic, relying instead on the pre-initialized NotificationVerifier.

---

Changes walkthrough

Relevant files

Enhancement

coprocess_bundle.go Simplify Bundle Verification Process                                          gateway/coprocess_bundle.go Removed the dependency on goverify from the bundle verification process. Simplified the signature verification logic by using the already initialized NotificationVerifier. Removed redundant checks for public key path and bundle signature presence. +4/-15    redis_signals.go Streamline Signature Verification in Redis Signals              gateway/redis_signals.go Removed the dynamic loading of public key for signature verification in isPayloadSignatureValid. Streamlined the signature verification process by relying on the pre-initialized NotificationVerifier. +0/-16    server.go Centralize NotificationVerifier Initialization                      gateway/server.go Added initialization of NotificationVerifier in afterConfSetup if a public key path is provided. Centralized the public key loading process to improve maintainability. +8/-0

---

✨ PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[github-actions]

PR Description updated to latest commit (cc33cea)

[github-actions]

PR Review

⏱️ Estimated effort to review [1-5]	2, because the changes are straightforward and localized to specific areas in the codebase. The removal of goverify dependency and centralization of NotificationVerifier initialization are clear and concise. However, understanding the impact of these changes on the system's behavior and ensuring no side effects require some domain knowledge.
🧪 Relevant tests	No
🔍 Possible issues	Possible Bug: The removal of the check for PublicKeyPath in isPayloadSignatureValid function might lead to bypassing signature validation if NotificationVerifier is not properly initialized.
🔒 Security concerns	No

Code feedback:

---

relevant file	gateway/coprocess_bundle.go
suggestion	Consider adding a log message before returning an error when the bundle isn't signed. This will help in debugging and understanding why a bundle verification failed. [medium]
relevant line	return errors.New("Bundle isn't signed")

---

relevant file	gateway/redis_signals.go
suggestion	Reinstate the check for PublicKeyPath in isPayloadSignatureValid or ensure NotificationVerifier is always correctly initialized to avoid bypassing signature validation. [important]
relevant line	- if notification.Gw.GetConfig().PublicKeyPath != "" && notification.Gw.NotificationVerifier == nil {

---

relevant file	gateway/server.go
suggestion	Add error handling for the scenario where PublicKeyPath is set but the file is not found or is inaccessible, to prevent the gateway from starting with invalid configuration. [important]
relevant line	mainLog.WithError(err).Fatalf("Failed loading public key from path: %s", err)

---

relevant file	gateway/coprocess_bundle.go
suggestion	To improve code clarity, consider extracting the signature verification logic into a separate method. This will make Verify method more readable and maintainable. [medium]
relevant line	useSignature := bundleVerifier != nil

---

---

✨ Review tool usage guide:

---

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

• When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

• With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

[github-actions]

PR Code Suggestions

Category	Suggestions
Enhancement	Simplify the condition by removing the unnecessary boolean flag. Instead of using a boolean flag useSignature to check if bundleVerifier is not nil, directly check if bundleVerifier is not nil in the if condition. This makes the code more straightforward and eliminates the need for an extra variable. gateway/coprocess_bundle.go [49-52] -useSignature := bundleVerifier != nil -if useSignature && b.Manifest.Signature == "" { +if bundleVerifier != nil && b.Manifest.Signature == "" {
	Enhance error messages for better debugging. Consider adding detailed error information for the case when the bundle isn't signed. This could include the bundle name or identifier to help with debugging. gateway/coprocess_bundle.go [54] -return errors.New("Bundle isn't signed") +return fmt.Errorf("bundle '%s' isn't signed", b.Spec.CustomMiddlewareBundle)
Best practice	Use structured logging for better log management. When logging errors, consider using structured logging instead of concatenating strings. This approach makes it easier to query logs and extract meaningful insights. gateway/redis_signals.go [211] -pubSubLog.Error("Could not verify notification: ", err, ": ", notification) +pubSubLog.WithError(err).WithField("notification", notification).Error("Could not verify notification")
	Propagate errors instead of directly logging fatal errors. When loading the public key, if an error occurs, it might be more appropriate to return the error to the caller instead of directly logging a fatal error. This allows the caller to decide how to handle the error, which is especially important in a library or when testing. gateway/server.go [1457-1459] gw.NotificationVerifier, err = goverify.LoadPublicKeyFromFile(conf.PublicKeyPath) if err != nil { - mainLog.WithError(err).Fatalf("Failed loading public key from path: %s", err) + return fmt.Errorf("failed loading public key from path: %w", err) }
	Use constants for repeated error messages. It's good practice to define error messages as constants or variables if they are used in multiple places. This makes it easier to manage and update error messages. gateway/coprocess_bundle.go [54] -return errors.New("Bundle isn't signed") +var ErrBundleNotSigned = errors.New("bundle isn't signed") +return ErrBundleNotSigned

---

✨ Improve tool usage guide:

---

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

• When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

• With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.